Improvements to B::Deparse handling of signatured subs #23710

leonerd · 2025-09-15T15:38:38Z

Added ->ambient_pragmas_from_caller method for simplicity
Add best-effort fallback deparse of OP_MULTIPARAM to prior pure-perl behaviours
Add unit test of such
Add documentation that points out what it does and alerts users to the fact this may not always work in future

Currently no perldelta, but I could be convinced to add one if considered significant. Do we normally include changes to bundled modules in perldelta?

book · 2025-09-15T16:33:50Z

I really appreciate the (best) effort put into this.

I only use B::Deparse to get a better understanding of code, and (more often than not) to show people who don't know Perl very well how some code excerpt is understood by Perl itself. Usually, by doing something like:

perl -MO=Deparse -e '...'

I don't think this kind of usage will trigger the behaviour described with regard to "ambient pragmas", though.

leonerd · 2025-09-15T20:09:42Z

Indeed so. In the usual -MO=Deparse scenario, it prints entire packages or at least entire subs, complete with the surrounding context:

$ perl -MO=Deparse,func -E 'sub func($x, $y, $z = 2) {}'
use feature 'current_sub', 'bitwise', 'evalbytes', 'fc', 'isa', 'module_true', 'postderef_qq', 'say', 'signatures', 'state', 'try', 'unicode_strings', 'unicode_eval';
use builtin (':5.40');
sub func ($x, $y, $z = 2) {
    ;
}
-e syntax OK

bram-perl

How are you planning to merge this?

Branch as is?
Squashing some commits and fast-forward merge (i.e. no merge commit)
With a merge commit?

My personal view: there are definitely some commits that I would keep separate (i.e. the tidying up) .

bram-perl · 2025-09-18T17:46:00Z

lib/B/Deparse.t

-    warning_bits => $warning_bits,
-    '%^H'        => $hinthash,
-);
+$deparse->ambient_pragmas_from_caller;


Commit tests for ->ambient_pragmas_from_caller

This commit is slightly confusing; that it: when seeing the commit title I would've expected tests for the ambient_pragmas_from_caller function but what it is doing is using it inside the tests.

(If that is all that was intended then fine; I'm not asking for explicit tests for the ambient_pragmas_from_caller it's just what I expected to see in the commit)

Oh yeah that was more of a note to myself to remind me. I'll just squash it in to the previous commit.

bram-perl · 2025-09-18T18:13:36Z

lib/B/Deparse.pm

+    foreach my $parami ( 0 .. $#param_padix ) {
+        my $argix = $parami;
+
+        $code .= "my $param_padname[$parami] = ";


Commit Best-effort deparse of OP_MULTIPARAM when feature 'signatures' is disabled

Reading up a bit on signatures, in perlsub, it contains:

An optional parameter can be nameless just like a mandatory parameter. For example,

sub foo ($thing, $ = 1) { print $thing; }

The parameter's default value will still be evaluated if the corresponding argument isn't supplied, even though the value won't be stored anywhere. This is in case evaluating it has important side effects. However, it will be evaluated in void context, ...

It's not yet clear to me how/if this is handled.

(But I'm not that familiar yet with this code and/or signatures so I might be overlooking something)

It's not yet clear to me how/if this is handled.

Ooh, good catch. I don't have a test for that yet, and in fact I'm not sure the code will even handle it. Looks like more work to do there.

bram-perl · 2025-09-18T18:16:21Z

lib/B/Deparse.pm

+as possible.  This is performed on a B<best-effort> basis.  It is not
+guaranteed to perfectly capture the semantics of the signature's behaviour,
+only to offer a human-readable suggestion as to what it might do.
+Furthermore. it is not guaranteed to be able to reproduce every possible


Commit Add docs to B::Deparse about signatures pure-perl fallback

Furthermore. --> Furthermore, ?

leonerd · 2025-09-19T14:02:33Z

Squashing some commits and fast-forward merge (i.e. no merge commit)

Yeah I'll squash some of them out and preserve the identity of others. I'll have a hack at that now.

leonerd · 2025-09-19T14:32:02Z

Rebased, squashed some commits, fixed a few more little docs issues, added code + tests for anonymous signature params.

PTAL.

bram-perl · 2025-09-19T17:26:46Z

lib/B/Deparse.t

+    }
+}


Commit Unit tests for B::Deparse coderef2text on signatured sub with signatures feature enabled

Slightly wondering: should $deparse->ambient_pragmas_from_caller; be called after the test(s) to reset the state?
In the earlier block of code (line 96) it does happen explicitely.

There's probably no huge need to reset it after some tests, as any newly-added test block that gets added after this may want to set it back to something else for its own purposes anyway.

bram-perl · 2025-09-19T17:33:11Z

lib/B/Deparse.pm

+        else {
+            # anonymous params without defaulting expressions can be entirely ignored
+            $param_padix[$parami] or next;
+
+            $stmt .= "\$_[$argix]";
+        }


Commit Best-effort deparse of OP_MULTIPARAM when feature 'signatures' is disabled

While the code works it was a bit confusing at first.
I would probably write this as:

if (my $defmode = $param_defmode[$parami]) { ... } elsif (not $param_padix[$parami]) { # anonymous params without defaulting expressions can be entirely ignored next; } else { $stmt .= "\$_[$argix]"; }

The check is also inconsistent with the earlier check. Earlier check (on line 1130):

if( length $param_padname[$parami] > 1 ) {

while here it just checks for a true value.

While the code works it was a bit confusing at first. I would probably write this as:
...

I wanted to structure it in two branches, for optional vs. mandatory params, and within each branch then have a condition for anonymous versions of those. It's two binary choices, not a threeway split.

The check is also inconsistent with the earlier check. Earlier check (on line 1130):
...
while here it just checks for a true value.

Ohyes, hmm. That's quite subtle.

Anonymous mandatory params don't even get allocate a pad slot, so their padname is undefined. Anonymous optional params need a pad slot to detect if it's been set or not, and therefore have a name that is simply '$'. So the conditions for detecting named vs. anonymous params are different depending if they're optional or mandatory.

I have explained further in some more comment.

While the code works it was a bit confusing at first. I would probably write this as:
...

I wanted to structure it in two branches, for optional vs. mandatory params, and within each branch then have a condition for anonymous versions of those. It's two binary choices, not a threeway split.

The thing that bothers me most with:

else { # Mandatory anonymous params can be entirely ignored. Their pad # index will be zero. $param_padix[$parami] or next; $stmt .= "\$_[$argix]"; }

is that the comment is at the beginning of the block and it can easily be read as applying to the entire block but it only applies to the next line.
(I think my initial thought was something like: the comment says they can be ignored but it still does $stmt .= ... which felt conflicting)

Alternative (untested):

if (my $defmode = $param_defmode[$parami]) { ... } else { if (not $param_padix[$parami]) { # Mandatory anonymous params can be entirely ignored. Their pad # index will be zero. next; } $stmt .= "\$_[$argix]"; }

or (untested):

if (my $defmode = $param_defmode[$parami]) { ... } else { $param_padix[$parami] or next; # Mandatory anonymous params can be entirely ignored. Their pad # index will be zero. $stmt .= "\$_[$argix]"; }

Ahyes, I see how the positioning of some of the comments is rather misleading. I've had a go at adjusting it somewhat more, see new push.

bram-perl · 2025-09-19T17:34:39Z

lib/B/Deparse.pm

+    foreach my $parami ( 0 .. $#param_padix ) {
+        my $argix = $parami;
+
+        my $stmt = "my $param_padname[$parami] = ";


Commit Best-effort deparse of OP_MULTIPARAM when feature 'signatures' is disabled

Can $param_padname[$parami] be undef? (If so then it will trigger a warning)

No, the name is always at least '$'

bram-perl · 2025-09-19T17:36:10Z

lib/B/Deparse.pm

+
+            # anonymous params with defaults don't create or assign a variable but
+            # still evaluate the defaulting expression for side-effects
+            if( length $param_padname[$parami] > 1 ) {


Commit Best-effort deparse of OP_MULTIPARAM when feature 'signatures' is disabled

nit-picking: style seems appears inconsistent?
Earlier if-blocks are all if (...) { while this is if( ... ) {

(if/when intentional then feel free to ignore)

Ahyes, fixed.

Add C<...> wrapping around the name of the module, also a few other places. Also add L<...> wrapping around names of other modules.

…res feature enabled

…abled

leonerd mentioned this pull request Sep 15, 2025

B::Deparse->coderef2text does not subs with signatures #23699

Open

leonerd force-pushed the gh23699 branch from 8595f1a to 2871228 Compare September 18, 2025 18:05

bram-perl reviewed Sep 18, 2025

View reviewed changes

leonerd force-pushed the gh23699 branch from 2871228 to d371cc7 Compare September 19, 2025 14:31

bram-perl reviewed Sep 19, 2025

View reviewed changes

leonerd force-pushed the gh23699 branch 2 times, most recently from 4868d81 to 94d4a7e Compare September 19, 2025 20:04

leonerd added 3 commits September 23, 2025 14:04

Tidy up Pod formatting in lib/B/Deparse.pm

5659a96

Add C<...> wrapping around the name of the module, also a few other places. Also add L<...> wrapping around names of other modules.

Add B::Deparse->ambient_pragmas_from_caller helper shortcut

ffc9a53

Unit tests for B::Deparse coderef2text on signatured sub with signatu…

da3253e

…res feature enabled

leonerd force-pushed the gh23699 branch from 94d4a7e to 414c7ed Compare September 23, 2025 13:10

bram-perl approved these changes Sep 23, 2025

View reviewed changes

Best-effort deparse of OP_MULTIPARAM when feature 'signatures' is dis…

8c53225

…abled

leonerd force-pushed the gh23699 branch from 414c7ed to 8c53225 Compare September 23, 2025 22:24

tonycoz approved these changes Sep 23, 2025

View reviewed changes

leonerd merged commit a3c9c70 into Perl:blead Sep 24, 2025
33 checks passed

leonerd deleted the gh23699 branch September 24, 2025 10:34

Improvements to B::Deparse handling of signatured subs #23710

Improvements to B::Deparse handling of signatured subs #23710

Uh oh!

Conversation

leonerd commented Sep 15, 2025

Uh oh!

book commented Sep 15, 2025

Uh oh!

leonerd commented Sep 15, 2025

Uh oh!

bram-perl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonerd commented Sep 19, 2025

Uh oh!

leonerd commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants