parse_subsignature() gets confused about empty parens `()`

[leonerd]

The newly-added (perl 5.31.3) core function of parse_subsignature() is for parsing the inside contents of a sub signature; for example it will extract the $x, $y part of

sub foo($x, $y) { $x + $y }

When facing an empty paren () it however fails; it increments the value of PL_parser->error_count despite yielding a valid op tree for the empty sig. This is currently causing a CPAN module failure:

https://rt.cpan.org/Ticket/Display.html?id=132284

[leonerd]

Further analysis by a bunch of different test cases reveals a curious behaviour. By using test code like

      fprintf(stderr, "ENTER parse_subsignature bufptr: %s", PL_parser->bufptr);
      sigop = parse_subsignature(0);
      fprintf(stderr, "LEAVE parse_subsignature bufptr: %s", PL_parser->bufptr);

we can see the buffer pointer just before and just after the function runs. For any correctly-parsed signature, the buffer pointer is left facing the closing paren ) :

parsing signature for <withparam>
ENTER parse_subsignature bufptr: $x) { return $x + 1 }
LEAVE parse_subsignature bufptr: ) { return $x + 1 }

parsing signature for <sum>
ENTER parse_subsignature bufptr: @x) {
LEAVE parse_subsignature bufptr: ) {

parsing signature for <firstandthird>
ENTER parse_subsignature bufptr: $x, $, $z) {
LEAVE parse_subsignature bufptr: ) {

parsing signature for <withoptparam>
ENTER parse_subsignature bufptr: $one = 1) { return $one + 2 }
LEAVE parse_subsignature bufptr: ) { return $one + 2 }

parsing signature for <has_whitespace>
ENTER parse_subsignature bufptr: $x
LEAVE parse_subsignature bufptr: ) {

Yet when given a sub with the empty parens () it skips over that close paren and reports an error:

parsing signature for <noparams>
ENTER parse_subsignature bufptr: ) { return "constant" }
LEAVE parse_subsignature bufptr:  { return "constant" }
syntax error at t/02func-signatures.t line 56, near "func noparams()"
Expected ')', found buf: { return "constant" }

[leonerd]

I've just put a new version of XS-Parse-Sublike on CPAN which contains a workaround for this core issue:
https://metacpan.org/source/PEVANS/XS-Parse-Sublike-0.07/lib/XS/Parse/Sublike.xs#L76-93

As such I think I can remove the BBC tag. The underlying issue remains however.

[leonerd]

Further investigation by injecting lots of warn() calls into the action blocks of perly.y reveals this paren-skipping behaviour to be at some awkward point I can't quite narrow down.

diff --git a/perly.y b/perly.y
index c90b8b41b8..1677caa459 100644
--- a/perly.y
+++ b/perly.y
@@ -763,9 +763,9 @@ siglist:
 
 /* () or (....) */
 siglistornull:		/* NULL */
-			{ $$ = NULL; }
+			{ warn("PARSE siglistornull empty buf: %s", parser->bufptr); $$ = NULL; }
 	|	siglist
-			{ $$ = $1; }
+			{ warn("PARSE siglistornull siglist\n"); $$ = $1; }
 
 /* optional subroutine signature */
 optsubsignature:	/* NULL */
@@ -780,6 +780,7 @@ subsignature:	'(' subsigguts ')'
 subsigguts:
                         {
                             ENTER;
+			    warn("ENTER subsigguts buf: %s", parser->bufptr);
                             SAVEIV(parser->sig_elems);
                             SAVEIV(parser->sig_optelems);
                             SAVEI8(parser->sig_slurpy);
@@ -787,6 +788,7 @@ subsigguts:
                             parser->sig_optelems = 0;
                             parser->sig_slurpy   = 0;
                             parser->in_my        = KEY_sigvar;
+			    warn("INITed subsigguts buf: %s", parser->bufptr);
                         }
                 siglistornull
 			{
@@ -794,6 +796,12 @@ subsigguts:
                             struct op_argcheck_aux *aux;
                             OP            *check;
 
+			    warn("MIDWAY subsigguts buf: %s", parser->bufptr);
+			    if(sigops)
+			      op_dump(sigops);
+			    else
+			      warn("NO OPS\n");
+
 			    if (!FEATURE_SIGNATURES_IS_ENABLED)
 			        Perl_croak(aTHX_ "Experimental "
                                     "subroutine signatures not enabled");
@@ -841,6 +849,7 @@ subsigguts:
                              */
                             parser->expect = XATTRBLOCK;
                             parser->sig_seen = TRUE;
+			    warn("LEAVE subsigguts buf: %s", parser->bufptr);
                             LEAVE;
 			}
 	;

If we inspect a correct case; for example the API test case (subsignature $one = 1) we get:

ENTER subsigguts buf:  $one = 1);
INITed subsigguts buf:  $one = 1);
PARSE siglistornull siglist
MIDWAY subsigguts buf: );
LEAVE subsigguts buf: );

This is all much as we expected. The subsigguts setup code didn't move the pointer, the middle part parsed the entire contents and stopped while looking at the close paren, which didn't then move for the finishing code to generate the optree. All good.

Now for the next test case of empty parens:

ENTER subsigguts buf: ); # empty
INITed subsigguts buf: ); # empty
PARSE siglistornull empty buf: ; # empty
MIDWAY subsigguts buf: ; # empty
LEAVE subsigguts buf: ; # empty

Curiously the INITed message is still looking at the close paren but by the time we get to the PARSE message the buffer pointer has already stepped forward over it. Looking at the structure of perly.y I can't see any other code locations where things would happen, but I'm not sufficiently expert in this area to know if there's other subtle bits of logic hiding in nonobvious places.

[iabyn]

On Thu, Apr 02, 2020 at 10:03:56AM -0700, Paul Evans wrote: Further investigation by injecting lots of `warn()` calls into the action blocks of `perly.y` reveals this paren-skipping behaviour to be at some awkward point I can't quite narrow down.

I've only skimmed this thread, but are you aware of running a debugging perl with-DpTv which shows in detail what the toker is grabbing from input and returning to the parser, and how the parser is interpreting those rules (and what its pushing on or popping off the parse stack). You could run it against a normal parse with the src containing an empty sig, and compare it with what happens when parse_subsignature() is used instead.

…

-- Little fly, thy summer's play my thoughtless hand has terminated with extreme prejudice. (with apologies to William Blake)

[xsawyerx]

@iabyn What should parse_subsignature do when the signature is empty? If it should fail, is there a way to introspect and see that the signature is empty? (!= NULL, empty string check, etc.)

[ilmari]

@xsawyerx It should return a standard arg-checking optree with a zero-argument OP_ARGCHECK.

[xsawyerx]

@ilmari Thank you. :)

Considering this is no longer a BBC, I'm happy to see this conversation continue but I'm removing it from a blocker.