Fix BEGIN block placeholder affecting file return value#341
Merged
Conversation
When a file ends with a BEGIN block followed by __END__, the file
should return the value of the last non-compile-time statement
(e.g., VERSION assignment). Previously, the BEGIN block placeholder
(OperatorNode undef) was being used as the last statement value.
The fix adds the compileTimeOnly annotation to the BEGIN block
placeholder, matching the behavior of other compile-time constructs.
This allows EmitBlock to correctly skip it when finding the last
meaningful statement for return value purposes.
This fixes modules like Package::Stash.pm that end with:
BEGIN { ... }
__END__
Note: Bare block return value at file level still needs a separate
fix - that issue requires distinguishing file-level RUNTIME context
from subroutine body RUNTIME context.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Add unit test documenting Perl context behavior for different block types - Document findings: bare blocks always return their value regardless of context - Update design doc with analysis and solution options Generated with Devin - https://cli.devin.ai/docs Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Both approaches (register spilling and direct RUNTIME context) cause JVM verification errors in tests with complex control flow (tie, local, typeglob). The root cause is fundamental: changing how the bare block body is compiled affects the entire bytecode structure, causing verifier failures at merge points. Generated with Devin - https://cli.devin.ai/docs Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Tests for bare block return values in RUNTIME context are marked TODO since this feature is not yet implemented. This includes: - Test 5: Bare block as last stmt in sub - Test 7: File loaded via do with bare block - Test 8: eval string with bare block - Test 12: File ending with bare block Generated with Devin - https://cli.devin.ai/docs Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This fixes bare blocks (like { 42; }) to return their last expression
value when used in SCALAR or LIST context. This is needed for patterns
like my $val = do { my $x = 99; $x; } and my @vals = do { (1,2,3); }.
Key changes:
- Move result register load to after endLabel to ensure consistent stack
state across all code paths (normal, last, next)
- Only apply to UNLABELED bare blocks (not labeled blocks like TODO:)
to avoid breaking Test::More TODO handling
- RUNTIME context is NOT yet fixed due to Test2 context handling issues
The fix uses register spilling: allocate a local variable, tell the
block to store its last element value there, then load after endLabel.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Fixes bare blocks (like `{ 42; }`) to return their last expression value
when used in SCALAR/LIST context or as the last statement in a file
(RUNTIME context for do/require).
Implementation:
- EmitStatement: Add register spilling for bare blocks in SCALAR/LIST context
- Parser: Add isFileLevelBlock annotation for do/require files
- EmitBlock: Handle bare blocks in RUNTIME context using SCALAR visitation
This fixes Package::Stash::PP and similar modules that end with a bare
block containing lexical variables.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The subroutine bare block case (sub foo { { 99 } }) cannot be fixed without
breaking Test2 context handling. This comment documents the limitation.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Documents the root cause analysis showing two issues: 1. Bare block return value in subroutines 2. Indirect object syntax bug that drops values The indirect object syntax bug must be fixed first before enabling blockIsSubroutine in EmitBlock.java. All tests must pass in both JVM and interpreter mode. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
In Perl, `(release $ctx, "V")` should return two elements: 1. The result of $ctx->release() 2. The string "V" Previously PerlOnJava consumed all arguments when detecting indirect object syntax, then only used the first one and dropped the rest. Fix: When detecting `method $object, args` syntax, only consume the object (using prototype "$" instead of "@"), leaving trailing args for the outer list context. This fixes Test::Builder's `release $ctx, $self->cmp_ok(...)` pattern. Also updates bare block return fix design doc with findings about why the simple `blockIsSubroutine` approach breaks Test2. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
All bare block return value tests now pass since the fix in EmitBlock.java has been applied. Remove the TODO markers that were marking these tests as expected failures. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
All three related issues have been fixed: 1. Indirect object syntax bug (SubroutineParser.java) 2. Hash literal detection bug (StatementResolver.java) 3. Bare block return value (EmitBlock.java) All tests pass including Test2-based tests. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Fixes Windows CI failure - the test was using /tmp/ which does not exist on Windows. Now uses File::Temp which handles cross-platform temp file creation correctly. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Two fixes to enable bare blocks returning their value:
1. EmitBlock.java: Add blockIsSubroutine to the condition that handles
bare blocks in RUNTIME context. This allows sub { { 99 } } to return 99.
2. StatementResolver.java: Detect hash literals that start with % or @
sigils (e.g., { %{hash} }, { @array }). This prevents patterns like
map {{ %{$_} }} @DaTa from being misidentified as bare blocks,
which would break Test2.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a file ends with a BEGIN block followed by
__END__, the file should return the value of the last non-compile-time statement (e.g.,$VERSIONassignment). Previously, the BEGIN block placeholder (OperatorNode "undef") was being used as the last statement's value, causing modules to fail the "did not return a true value" check.Changes
compileTimeOnlyannotation to the BEGIN block placeholder inSpecialBlockParser.javaTesting
makepassesour $VERSION = '1.0'; BEGIN { } __END__now correctly return the VERSION valueKnown Limitations (WIP)
{ 42; }as the last statement) still needs workPackage::Stash::PP.pmwhich end with a bare block containing sub definitionsRelated
dev/design/cpan_client.mdPhase 11 for details on the DateTime dependency chainGenerated with Devin