Open
Conversation
Recent work to optimise away empty branches of if/else etc,
unfortunately broke the deparsing of
if (C1)
X;
}
elsif (C2) {
Y;
}
I.e. where there is no final else after the elsif. The new code
assumed that the 'elsif' branch of the if/else was itself
an OP_COND; but in the absence of a final else, and OP_AND is used
instead.
Better explain how the optimised away if/else blocks are handled and the difference between ?: and if/else. Also, add a few blank lines for readability.
this:
for $foo (...) {}
was no longer being deparsed, since a recent change optimised away the
'stub' op making up the body.
This caused Deparse to not spot a valid block top op and mistake it
instead for a 'for' statement modifier. It would then panic because the
loop variable wasn't '$_'.
Also fix the panic - it was just doing confess() without any message.
Do a normal die() instead, with a helpful error message.
'for' was recently enhanced to support
for my ($x, \$y, $z) (...) { ... }
where the OP_ITER's targ contains both a count of the number of index
variables and a mask indicating which vars are refaliases.
This commit makes Deparse catch up.
Before this commit,
cd t; ./TEST -deparse op/for-many.t
was failing.
Recent changes optimised away the stub op in empty blocks.
This was causing code like this:
{
f(234);
}
continue { }
to break the deparser:
Can't call method "name" on an undefined value at lib/B/Deparse.pm line 6987.
This was due to a long-standing thinko in is_lexical_subs() which was
doing:
my (@ops) = shift;
when it (presumably) should have been doing:
my (@ops) = @_;
The pre-existing effect of this thinko was that potentially it might not
spot a lexical sub declaration and thus change whether to wrap the block
in 'do {}'.
The effect after stub become ex-stub was that @_ was now empty and so
$ops[0] was now an undef value.
Handle the recent change where stub ops are optimised out
Since v5.43.1-82-gc0053197fb,
Reapply "op.c: re-enable coderef-in-stash optimization"
t/op/caller.t no longer passes a round trip through Deparse. This commit
adds it to the list of test files expected to fail.
The issue is that when storing a CV directly in a stash rather than as
part of a new GV, the 'reify GV' code, which later converts the stash
entry from a CV to a GV-to-CV, will set the line/file of the GV to the
location where/when the reification took place, rather than to where the
sub was defined. This doesn't matter 99.999% of the time - I temporarily
hacked Perl_cvgv_from_hek() to always set the line number to 999999 and
no tests failed - but it fails a caller.t deparse round-trip test which
is using '#line' to modify what a BEGIN sub sees as its caller. The
deparsed file doesn't have the correct values in the '#line'.
I can't see any easy way to fix this - I don't think the original
line/file info is stored anywhere - so this commit just skips caller.t
for now.
I've also added some code comments to Perl_cvgv_from_hek() to explain
the implementation leakage.
The same ops, AND/OR/COND_EXPR, are used both for statements such
as:
if (...) { ... }
if (...) { ... } else { ... }
... if ...;
and expressions, such as:
... && ...
... ? ... : ...
This commit adds a new private OP flag, OPpSTATEMENT, and makes the
parser set it on such ops when they are derived from if/else etc rather
than from ?:,&&,|| etc.
The flags are currently unused after being set on the ops, but they will
be used by Deparse shortly to stop it from having to guess what form to
use. There are lots of spare private flag bits on those logops, so using
one of them just for a non-essential reason seems reasonable.
Use the OPpSTATEMENT flag, added by the previous commit, to simplify
the deparsing of
if ($c) { foo() }
if ($c) { foo() } else { bar() }
foo() unless $c
etc
versus
$c && foo()
$c ? foo() : bar()
$c || foo()
etc
Previously it had to guess which of those two forms to use; now it uses
that flag, which simplifies the code and makes a couple of failing
round-trip test files pass under './TEST -deparse'.
This commit: - updates the POD in t/test_pl.pod to improve the documentation for the refcount_is() test function. It makes it clear that the first argument is a *reference* to the thing which is having it's reference count compared. - makes the example in that pod simpler. - updates the tests in t/test_pl/examples.t which are supposed to test that example code snippet in test_pl.pod. It's changed because: first, the example code didn't match the test code, second, the example code has changed; third, the old test code didn't survive a round-trip through './TEST -deparse', since \(1+2) was being constant-folded to \1, with a different reference count.
Update the list of skipped files in
Porting/deparse-skips.txt
New entries have been added due to:
- named params in signatures and classes not yet being deparsed correctly
- constant blessed hash ref not being correctly deparsed
Two entries have been removed since they now pass:
dist/Data-Dumper/t/trailing_comma.t
due to better deparsing of '&&' vs 'unless'
cpan/autodie/t/exceptions-smartmatch.t
I'm not sure why that file passes.
Contributor
|
Is this "annual fixup" something which our docs indicate needs to be done on an annual basis? If not, should it be documented? If so, perhaps it should be done in connection with the annual production release? |
't/TEST -deparse' - which uses Porting/deparse-skips.txt to decide which files are expected to fail when run after a round-trip through Deparse - normally lists any expected-fail test files which unexpectedly succeeded. However, it only lists them if at least one other test file failed. Or to put it another way, if all files expected to pass actually pass, it doesn't mention that some expected to fail passed too. This commit changes it so that it *always* lists such unexpected successes.
iabyn
force-pushed
the
davem/deparse_fixes
branch
from
64fb988 to
5cafb2b
Compare
Contributor
Author
Fixed, rebased and pushed. |
Contributor
Author
Ideally it should be done from time to time, especially after commits which add new features or alter the optree. Similarly to running Porting/bench.pl from time to time to see whether anything has slowed down; reducing stderr noise on build; reducing smoke failures etc. It's something that all porters might do from time to time rather than a specific step to be done by the release manager - who would be unlikely to be in a position to fix the issues anyway, and would likely be looking at the issues to late in the release cycle. |
Contributor
Should we have a CI step that runs TEST -deparse? |
Contributor
Author
|
On Tue, Feb 03, 2026 at 02:09:00PM -0800, Tony Cook wrote:
Should we have a CI step that runs TEST -deparse?
I don't know much about our GH CI environment, or what restrictions
there are on total CPU etc.
IIUC, './TEST -deparse' can't be parallelised and takes about 30 mins to
run single-CPU on my laptop. Not sure whether that would slow down the CI.
|
Contributor
Apologies for that, Dave. Thanks for fixing it up. I go through spells of remembering about |
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
4 participants
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.
This branch fixes up various things so that './TEST -deparse' again runs cleanly. It reflects the accumulated breakage over the past year, as things in the op tree change, new tests and test files are added etc.
In particular, there was quite a bit of fall-out from the recent optimising away of 'stub' ops from empty blocks.