diff --git a/dev/design/NUMERIC_WARNINGS_IMPLEMENTATION.md b/dev/design/NUMERIC_WARNINGS_IMPLEMENTATION.md new file mode 100644 index 000000000..22b9f68b7 --- /dev/null +++ b/dev/design/NUMERIC_WARNINGS_IMPLEMENTATION.md @@ -0,0 +1,188 @@ +# Numeric Warnings Implementation Plan + +## Problem Statement + +Perl's `use warnings "numeric"` should emit warnings like `Argument "abc" isn't numeric` when non-numeric strings are used in numeric context. Currently: + +1. `use warnings` and `no warnings` are compile-time pragmas +2. Numification happens at runtime via `RuntimeScalar.getDouble()` → `NumberParser.parseNumber()` +3. Runtime code doesn't know the compile-time warning state + +### Current Behavior (broken) +```perl +use warnings; +my $x = 0 + "abc"; # Should warn, but doesn't (or warns incorrectly) +{ + no warnings "numeric"; + my $y = 0 + "def"; # Should NOT warn +} +my $z = 0 + "ghi"; # Should warn +``` + +## Core Design Decision: Warn on Cache Miss + +Key insight: `NumberParser` has a numification cache. Most strings are only parsed once. +We only need to check the warning flag on **cache misses**. + +### Flow +``` +Cache hit path (fast, common): + getDouble() → parseNumber() → cache hit → return + [no warning check needed] + +Cache miss path (rare): + getDouble() → parseNumber() → parse string → + if (isNonNumeric && warningsEnabled()) warn → cache result → return + [one flag check only on cache miss] +``` + +### Behavior Difference from Perl + +- **Perl**: Warns every time a non-numeric string is used +- **Our approach**: Warns only on cache miss (first use of that string) + +This is acceptable. We can later add a warning flag to the cache entry if exact Perl behavior is needed. + +## Open Question: How to Track Warning State at Runtime + +The compile-time symbol table knows if warnings are enabled, but `parseNumber()` runs at runtime and needs to check this. Several options exist with different trade-offs. + +### Option A: Perl Global Variable with `local` + +Use a Perl global variable `$warnings::_numeric_enabled` that: +- Is set to 1 by `use warnings "numeric"` +- Is set to 0 by `no warnings "numeric"` using `local` for automatic scope restore + +```java +public static boolean isNumericWarningsEnabled() { + return getGlobalVariable("warnings::_numeric_enabled").getBoolean(); +} +``` + +**Pros:** +- Automatic block scoping via existing `local`/DynamicVariableManager +- Handles `goto` correctly (DynamicVariableManager already handles this) +- Simple to implement + +**Cons:** +- Hash lookup on every cache miss (slower than ThreadLocal) + +### Option B: ThreadLocal with try/finally + +```java +private static final ThreadLocal numericWarningsEnabled = + ThreadLocal.withInitial(() -> false); + +// Compiler generates for "no warnings" blocks: +boolean _saved = Warnings.isNumericWarningsEnabled(); +Warnings.setNumericWarningsEnabled(false); +try { + // block code +} finally { + Warnings.setNumericWarningsEnabled(_saved); +} +``` + +**Pros:** +- Fast ThreadLocal read +- Proper block scoping + +**Cons:** +- **Conflicts with `goto`** - JVM bytecode issues when goto jumps out of try/finally +- More complex compiler changes + +### Option C: Per-Class Static Field + ThreadLocal on Entry + +Each generated class (subroutine) has a compile-time constant: +```java +public class Sub_foo { + static final boolean NUMERIC_WARNINGS = true; // set at compile time +} +``` + +On subroutine entry, set a ThreadLocal: +```java +// Generated at subroutine entry +Warnings.setNumericWarnings(NUMERIC_WARNINGS); +``` + +**Pros:** +- Fast ThreadLocal read on cache miss +- No try/finally (no goto issues) +- Warning state is per-subroutine (matches Perl's lexical scoping) + +**Cons:** +- One ThreadLocal write per subroutine call +- Nested calls overwrite - need to verify this matches Perl semantics +- Doesn't handle block-level `no warnings` within a subroutine + +### Option D: Simple Global Flag (No Block Scoping) + +Just use a simple flag without block-level scoping: +- `use warnings` → enable globally +- `no warnings` → disable globally + +**Pros:** +- Simplest implementation +- No goto issues +- Correct for 99% of real code (most use file-level `use warnings`) + +**Cons:** +- Block-level `no warnings "numeric"` won't restore on block exit +- Less correct than Perl + +## NumberParser Changes (Common to All Options) + +Regardless of which option is chosen for tracking state, the parseNumber() changes are the same: + +```java +// In parseNumber(), after parsing determines the string is non-numeric: +// (isNonNumeric is already computed during parsing - no extra work) +if (isNonNumeric && Warnings.isNumericWarningsEnabled()) { + WarnDie.warn(new RuntimeScalar("Argument \"" + str + "\" isn't numeric"), + RuntimeScalarCache.scalarEmptyString); +} +``` + +Note: `isNonNumeric` is already determined during parsing (e.g., "abc" → 0). +The only new check is `isNumericWarningsEnabled()`. + +## Files to Modify + +1. `Warnings.java` - add `isNumericWarningsEnabled()` method (implementation depends on option chosen) +2. `NumberParser.java` - check flag on cache miss, emit warning +3. Possibly compiler changes depending on option chosen + +## Testing + +```perl +use warnings; +my $warned = 0; +local $SIG{__WARN__} = sub { $warned++ }; + +my $x = 0 + "abc"; +ok($warned == 1, "warns on non-numeric"); + +$warned = 0; +my $y = 0 + "123"; +ok($warned == 0, "no warning for numeric string"); + +$warned = 0; +{ + no warnings "numeric"; + my $z = 0 + "def"; +} +ok($warned == 0, "no warning in no-warnings block"); + +# After block, warnings should be restored (if block scoping implemented) +$warned = 0; +my $w = 0 + "ghi"; +ok($warned == 1, "warning restored after block"); +``` + +## Current Status + +- [x] Design documented +- [ ] Decision on runtime state tracking option (A, B, C, or D) +- [ ] Implementation +- [ ] Testing with op/numify.t diff --git a/src/main/java/org/perlonjava/core/Configuration.java b/src/main/java/org/perlonjava/core/Configuration.java index 957fe3691..0cc6af171 100644 --- a/src/main/java/org/perlonjava/core/Configuration.java +++ b/src/main/java/org/perlonjava/core/Configuration.java @@ -33,7 +33,7 @@ public final class Configuration { * Automatically populated by Gradle/Maven during build. * DO NOT EDIT MANUALLY - this value is replaced at build time. */ - public static final String gitCommitId = "3f00c2f24"; + public static final String gitCommitId = "c4e439b01"; /** * Git commit date of the build (ISO format: YYYY-MM-DD). diff --git a/src/main/java/org/perlonjava/frontend/parser/PrototypeArgs.java b/src/main/java/org/perlonjava/frontend/parser/PrototypeArgs.java index 39d22889f..59e03c29d 100644 --- a/src/main/java/org/perlonjava/frontend/parser/PrototypeArgs.java +++ b/src/main/java/org/perlonjava/frontend/parser/PrototypeArgs.java @@ -165,9 +165,21 @@ static ListNode consumeArgsWithPrototype(Parser parser, String prototype, boolea // Check for too many arguments without parentheses only if prototype expects 2+ args if (!hasParentheses && countPrototypeArgs(prototype) >= 2) { - // If we see a comma after parsing all required args, there are too many + // If we see a comma after parsing all required args, check if it's a trailing comma if (isComma(TokenUtils.peek(parser))) { - throwTooManyArgumentsError(parser); + // Consume the comma and check what follows + int saveIndex = parser.tokenIndex; + consumeCommas(parser); + LexerToken nextToken = TokenUtils.peek(parser); + // If followed by a statement terminator, it's a trailing comma (allowed) + // Otherwise, it's too many arguments + if (!Parser.isExpressionTerminator(nextToken) && + nextToken.type != LexerTokenType.EOF && + !nextToken.text.equals(")")) { + throwTooManyArgumentsError(parser); + } + // Restore position - the comma will be handled by the caller + parser.tokenIndex = saveIndex; } } } diff --git a/src/main/java/org/perlonjava/frontend/semantic/ScopedSymbolTable.java b/src/main/java/org/perlonjava/frontend/semantic/ScopedSymbolTable.java index 3d550d972..8f799d5a2 100644 --- a/src/main/java/org/perlonjava/frontend/semantic/ScopedSymbolTable.java +++ b/src/main/java/org/perlonjava/frontend/semantic/ScopedSymbolTable.java @@ -36,6 +36,8 @@ public class ScopedSymbolTable { // Stack to manage warning categories for each scope public final Stack warningFlagsStack = new Stack<>(); + // Stack to track explicitly disabled warning categories (for proper $^W interaction) + public final Stack warningDisabledStack = new Stack<>(); // Stack to manage feature categories for each scope public final Stack featureFlagsStack = new Stack<>(); // Stack to manage strict options for each scope @@ -65,6 +67,8 @@ public ScopedSymbolTable() { } } warningFlagsStack.push((BitSet) defaultWarnings.clone()); + // Initialize the disabled warnings stack (empty by default) + warningDisabledStack.push(new BitSet()); // Initialize the feature categories stack with an empty map for the global scope featureFlagsStack.push(0); // Initialize the strict options stack with 0 for the global scope @@ -135,6 +139,8 @@ public int enterScope() { inSubroutineBodyStack.push(inSubroutineBodyStack.peek()); // Push a copy of the current warning categories map onto the stack warningFlagsStack.push((BitSet) warningFlagsStack.peek().clone()); + // Push a copy of the current disabled warnings map onto the stack + warningDisabledStack.push((BitSet) warningDisabledStack.peek().clone()); // Push a copy of the current feature categories map onto the stack featureFlagsStack.push(featureFlagsStack.peek()); // Push a copy of the current strict options onto the stack @@ -159,6 +165,7 @@ public void exitScope(int scopeIndex) { subroutineStack.pop(); inSubroutineBodyStack.pop(); warningFlagsStack.pop(); + warningDisabledStack.pop(); featureFlagsStack.pop(); strictOptionsStack.pop(); } @@ -528,6 +535,10 @@ public ScopedSymbolTable snapShot() { st.warningFlagsStack.pop(); // Remove the initial value pushed by enterScope st.warningFlagsStack.push((BitSet) this.warningFlagsStack.peek().clone()); + // Clone disabled warnings flags + st.warningDisabledStack.pop(); // Remove the initial value pushed by enterScope + st.warningDisabledStack.push((BitSet) this.warningDisabledStack.peek().clone()); + // Clone feature flags st.featureFlagsStack.pop(); // Remove the initial value pushed by enterScope st.featureFlagsStack.push(this.featureFlagsStack.peek()); @@ -631,6 +642,8 @@ public void enableWarningCategory(String category) { Integer bitPosition = warningBitPositions.get(category); if (bitPosition != null) { warningFlagsStack.peek().set(bitPosition); + // Clear the disabled bit when enabling + warningDisabledStack.peek().clear(bitPosition); } } @@ -638,6 +651,8 @@ public void disableWarningCategory(String category) { Integer bitPosition = warningBitPositions.get(category); if (bitPosition != null) { warningFlagsStack.peek().clear(bitPosition); + // Mark as explicitly disabled (for proper $^W interaction) + warningDisabledStack.peek().set(bitPosition); } } @@ -646,6 +661,15 @@ public boolean isWarningCategoryEnabled(String category) { return bitPosition != null && warningFlagsStack.peek().get(bitPosition); } + /** + * Checks if a warning category was explicitly disabled via 'no warnings'. + * This is used to determine if $^W should be overridden. + */ + public boolean isWarningCategoryDisabled(String category) { + Integer bitPosition = warningBitPositions.get(category); + return bitPosition != null && warningDisabledStack.peek().get(bitPosition); + } + // Methods for managing features using bit positions public void enableFeatureCategory(String feature) { if (isNoOpFeature(feature)) { @@ -705,6 +729,10 @@ public void copyFlagsFrom(ScopedSymbolTable source) { this.warningFlagsStack.pop(); this.warningFlagsStack.push((BitSet) source.warningFlagsStack.peek().clone()); + // Copy disabled warnings flags + this.warningDisabledStack.pop(); + this.warningDisabledStack.push((BitSet) source.warningDisabledStack.peek().clone()); + // Copy feature flags this.featureFlagsStack.pop(); this.featureFlagsStack.push(source.featureFlagsStack.peek()); diff --git a/src/main/java/org/perlonjava/runtime/operators/CompareOperators.java b/src/main/java/org/perlonjava/runtime/operators/CompareOperators.java index ef4daeee2..eec84fee7 100644 --- a/src/main/java/org/perlonjava/runtime/operators/CompareOperators.java +++ b/src/main/java/org/perlonjava/runtime/operators/CompareOperators.java @@ -128,9 +128,6 @@ public static RuntimeScalar greaterThan(RuntimeScalar arg1, RuntimeScalar arg2) return getScalarBoolean((int) arg1.value > (int) arg2.value); } - // Check for uninitialized values - checkUninitialized(arg1, arg2, "gt (>)"); - // Prepare overload context and check if object is eligible for overloading int blessId = blessedId(arg1); int blessId2 = blessedId(arg2); @@ -145,6 +142,9 @@ public static RuntimeScalar greaterThan(RuntimeScalar arg1, RuntimeScalar arg2) } } + // Check for uninitialized values (only when using numeric comparison fallback) + checkUninitialized(arg1, arg2, "gt (>)"); + // Convert strings to numbers if necessary arg1 = arg1.getNumber(); arg2 = arg2.getNumber(); diff --git a/src/main/java/org/perlonjava/runtime/operators/FileTestOperator.java b/src/main/java/org/perlonjava/runtime/operators/FileTestOperator.java index 25f909967..ab30dfb84 100644 --- a/src/main/java/org/perlonjava/runtime/operators/FileTestOperator.java +++ b/src/main/java/org/perlonjava/runtime/operators/FileTestOperator.java @@ -59,10 +59,10 @@ static void updateLastStat(RuntimeScalar arg, boolean ok, int errno, boolean was lastStatErrno = errno; lastStatWasLstat = wasLstat; Stat.lastNativeStatFields = null; - if (!ok) { - lastBasicAttr = null; - lastPosixAttr = null; - } + // Always reset BasicFileAttributes - they should only be set by statForFileTest + // for real filesystem paths. JAR resources don't have BasicFileAttributes. + lastBasicAttr = null; + lastPosixAttr = null; } static void updateLastStat(RuntimeScalar arg, boolean ok, int errno) { @@ -128,10 +128,11 @@ private static boolean statForFileTest(RuntimeScalar arg, Path path, boolean lst } catch (UnsupportedOperationException | IOException ignored) { } - lastBasicAttr = basicAttr; - lastPosixAttr = posixAttr; getGlobalVariable("main::!").set(0); updateLastStat(arg, true, 0, lstat); + // Set attributes after updateLastStat (which resets them to null) + lastBasicAttr = basicAttr; + lastPosixAttr = posixAttr; Stat.lastNativeStatFields = Stat.nativeStat(path.toString(), !lstat); return true; } catch (NoSuchFileException e) { @@ -322,6 +323,17 @@ public static RuntimeScalar fileTest(String operator, RuntimeScalar fileHandle) } // JAR resource path (e.g., "jar:PERL5LIB/DBI.pm") if (Jar.exists(filename)) { + // Check if it's a directory entry (not a file) + if (Jar.isResourceDirectory(filename)) { + updateLastStat(fileHandle, true, 0); + return switch (operator) { + case "-d", "-e", "-r", "-x" -> scalarTrue; // It's a readable, executable directory + case "-f", "-l", "-w", "-z" -> scalarFalse; // Not a file, link, writable, or empty + case "-s" -> RuntimeScalarCache.scalarZero; // Size 0 + default -> scalarUndef; + }; + } + // It's a regular file updateLastStat(fileHandle, true, 0); return switch (operator) { case "-e", "-f", "-r" -> scalarTrue; // Exists, is a file, is readable diff --git a/src/main/java/org/perlonjava/runtime/operators/Stat.java b/src/main/java/org/perlonjava/runtime/operators/Stat.java index 6a39f218d..8371b6bc4 100644 --- a/src/main/java/org/perlonjava/runtime/operators/Stat.java +++ b/src/main/java/org/perlonjava/runtime/operators/Stat.java @@ -180,9 +180,6 @@ public static RuntimeList stat(RuntimeScalar arg) { // Not available on Windows } - lastBasicAttr = basicAttr; - lastPosixAttr = posixAttr; - if (nf != null) { statInternalNative(res, nf); } else if (posixAttr != null) { @@ -193,6 +190,9 @@ public static RuntimeList stat(RuntimeScalar arg) { getGlobalVariable("main::!").set(0); updateLastStat(arg, true, 0, false); + // Set attributes after updateLastStat (which resets them to null) + lastBasicAttr = basicAttr; + lastPosixAttr = posixAttr; lastNativeStatFields = nf; } catch (NoSuchFileException e) { getGlobalVariable("main::!").set(2); @@ -242,9 +242,6 @@ public static RuntimeList lstat(RuntimeScalar arg) { // Not available on Windows } - lastBasicAttr = basicAttr; - lastPosixAttr = posixAttr; - if (nf != null) { statInternalNative(res, nf); } else if (posixAttr != null) { @@ -255,6 +252,9 @@ public static RuntimeList lstat(RuntimeScalar arg) { getGlobalVariable("main::!").set(0); updateLastStat(arg, true, 0, true); + // Set attributes after updateLastStat (which resets them to null) + lastBasicAttr = basicAttr; + lastPosixAttr = posixAttr; lastNativeStatFields = nf; } catch (NoSuchFileException e) { getGlobalVariable("main::!").set(2); diff --git a/src/main/java/org/perlonjava/runtime/perlmodule/Version.java b/src/main/java/org/perlonjava/runtime/perlmodule/Version.java index 11795f52b..f35f550b1 100644 --- a/src/main/java/org/perlonjava/runtime/perlmodule/Version.java +++ b/src/main/java/org/perlonjava/runtime/perlmodule/Version.java @@ -319,18 +319,26 @@ public static RuntimeList VCMP(RuntimeArray args, int ctx) { // Check if arguments were swapped (third argument from overload) boolean swapped = args.size() > 2 && args.get(2).getBoolean(); - // Handle non-version objects + // Handle non-version objects - treat undef/empty as version 0 if (!v1.isBlessed() || !NameNormalizer.getBlessStr(v1.blessId).equals("version")) { + String v1Str = v1.toString().trim(); + if (v1Str.isEmpty()) { + v1Str = "0"; + } RuntimeArray parseArgs = new RuntimeArray(); parseArgs.push(new RuntimeScalar("version")); - parseArgs.push(v1); + parseArgs.push(new RuntimeScalar(v1Str)); v1 = parse(parseArgs, RuntimeContextType.SCALAR).scalar(); } if (!v2.isBlessed() || !NameNormalizer.getBlessStr(v2.blessId).equals("version")) { + String v2Str = v2.toString().trim(); + if (v2Str.isEmpty()) { + v2Str = "0"; + } RuntimeArray parseArgs = new RuntimeArray(); parseArgs.push(new RuntimeScalar("version")); - parseArgs.push(v2); + parseArgs.push(new RuntimeScalar(v2Str)); v2 = parse(parseArgs, RuntimeContextType.SCALAR).scalar(); } diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/Jar.java b/src/main/java/org/perlonjava/runtime/runtimetypes/Jar.java index 8747601cd..33badc6e2 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/Jar.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/Jar.java @@ -99,6 +99,55 @@ public static boolean exists(String filename) { return getResource(filename) != null; } + /** + * Checks if a JAR resource exists as a regular file (not a directory). + * JAR directory entries have contentLength of 0 and return -1 on read. + * + * @param filename The JAR path + * @return true if the resource exists and is a regular file + */ + public static boolean isFile(String filename) { + if (isJarDirectory(filename)) { + return false; // Virtual directories are not files + } + URL resource = getResource(filename); + if (resource == null) { + return false; + } + // Check if it's a real file by checking content length + // JAR directory entries have contentLength of 0 + try { + var conn = resource.openConnection(); + return conn.getContentLength() > 0; + } catch (Exception e) { + return false; + } + } + + /** + * Checks if a JAR resource is a directory. + * + * @param filename The JAR path + * @return true if the resource is a directory + */ + public static boolean isResourceDirectory(String filename) { + if (isJarDirectory(filename)) { + return true; // Virtual directories + } + URL resource = getResource(filename); + if (resource == null) { + return false; + } + // Check if it's a directory by checking content length + // JAR directory entries have contentLength of 0 + try { + var conn = resource.openConnection(); + return conn.getContentLength() == 0; + } catch (Exception e) { + return false; + } + } + /** * Opens a JAR resource for reading. * diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeIO.java b/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeIO.java index 75a0d02a5..f4c46e1ea 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeIO.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeIO.java @@ -103,8 +103,15 @@ protected boolean removeEldestEntry(Map.Entry eldest) { public static RuntimeIO stdout = new RuntimeIO(new StandardIO(System.out, true)); /** * Standard error stream handle (STDERR) + * Note: autoFlush is set to true to match Perl's unbuffered stderr behavior */ public static RuntimeIO stderr = new RuntimeIO(new StandardIO(System.err, false)); + + static { + // STDERR should be unbuffered (autoFlush) by default, like in Perl + stderr.autoFlush = true; + } + /** * Standard input stream handle (STDIN) */ diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/WarningFlags.java b/src/main/java/org/perlonjava/runtime/runtimetypes/WarningFlags.java index 457de6d10..548266135 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/WarningFlags.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/WarningFlags.java @@ -142,4 +142,15 @@ public void setWarningState(String category, boolean state) { public boolean isWarningEnabled(String category) { return getCurrentScope().isWarningCategoryEnabled(category); } + + /** + * Checks if a warning category was explicitly disabled via 'no warnings'. + * This is used to determine if $^W should be overridden. + * + * @param category The name of the warning category to check. + * @return True if the category was explicitly disabled, false otherwise. + */ + public boolean isWarningDisabled(String category) { + return getCurrentScope().isWarningCategoryDisabled(category); + } }