From @andk

Sample fail report​:

http​://www.cpantesters.org/cpan/report/a0340894-64b7-11e9-a696-470fc989963f

Xref​:

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

--
andreas
PS​: perl,perl,perl

From @iabyn

On Mon, Apr 22, 2019 at 10​:34​:57AM -0700, (Andreas J. Koenig) (via RT) wrote​:

Sample fail report​:

http​://www.cpantesters.org/cpan/report/a0340894-64b7-11e9-a696-470fc989963f

Xref​:

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

This bisects to the following. I haven't looked further yet.

commit aabfead
Author​: Karl Williamson <khw@​cpan.org>
AuthorDate​: Tue Mar 26 21​:54​:03 2019 -0600
Commit​: Karl Williamson <khw@​cpan.org>
CommitDate​: Sat Apr 13 16​:20​:35 2019 -0600

  Make a few parse fcns accessible to B​::Hooks​::Parser
 
  This module had made copies of three functions from toke.c many releases
  ago, and they stagnated. Most outside code has no business calling
  them, but the least worst choice I believe is to make them accessible,
  but hide that fact.
 
  This commit makes them accessible to modules that have defined PERL_EXT.
  It does not document their API's, and marks them as subject to change,
  so they aren't even listed as available in the docs. In other words,
  you'd have to really go digging to find out you could use them.
 
  And the API of two of the three had changed since the code was
  originally stolen. So that "subject to change" actually has happened.
  We should feel free to change the API as needed, and B​::Hooks​::Parser
  will have to be updated.
 
  Thanks to Tony Cook for advising me on this area unfamiliar to me.

The tests are failing this assertion​:

t/weird.t .............. perl5.29.10​: toke.c​:2538​: S_sublex_done​: Assertion `PL_lex_inwhat == OP_SUBST || PL_lex_inwhat == OP_TRANS' failed.

which is saying that if PL_lex_repl is true (we're toking a
replacement), then the op (PL_lex_inwhat) must be either OP_SUBST or
OP_TRANS)

--
I took leave and went to hear Mrs Turner's daughter play on the harpsicon,
but Lord, it was enough to make any man sick to hear her; yet I was forced
to commend her highly.
  -- The Diary of Samuel Pepys, 1 May 1663

The RT System itself - Status changed from 'new' to 'open'

From @khwilliamson

If I comment out that assert, it thinks things are syntax errors, so it needs further investigation
--
Karl Williamson

From @iabyn

On Wed, Apr 24, 2019 at 10​:26​:44AM -0700, Karl Williamson via RT wrote​:

If I comment out that assert, it thinks things are syntax errors, so it needs further investigation

The proximate cause is that signatures.xs not clearing PL_lex_stuff at a
certain point.

This Proof of Concept fix passes all tests in blead​:

--- signatures.xs-	2019-04-26 21:53:04.430901853 +0100
+++ signatures.xs	2019-04-26 22:07:32.304999812 +0100
@@ -151,6 +151,9 @@
 		return op;
 	}
 
+	assert(PL_lex_stuff == op_sv);
+	PL_lex_stuff = NULL;
+
 	tmp = hook_toke_scan_str (aTHX_ s);
 	tmp2 = hook_parser_get_lex_stuff (aTHX);
 	hook_parser_clear_lex_stuff (aTHX);

What is happening is that perl parses a sub prototype by using scan_str()
to extract the balanced delimiter​: in thise case the '()'s in

  sub foo ($x) { ... }

scan_str() sets PL_lex_stuff to the extracted prototype, i.e. '$x'
It then creates an OP_CONST whose op_sv is set to PL_lex_stuff, and then
PL_lex_stuff is cleared, i.e. this code in yylex()​:

  if (have_proto) {
  NEXTVAL_NEXTTOKE.opval =
  newSVOP(OP_CONST, 0, PL_lex_stuff);
  PL_lex_stuff = NULL;
  force_next(THING);
  }

B-Hooks-Parser / signatures.xs hook into the ck_foo() mechanism, so that
the hook is called for the OP_CONST in the newSVOP() above. The hook
revisits the lexing stream that's just been processed to confirm whether
it is of the form 'sub maybe_a_name (...)', using calls to
hook_toke_skipspace() etc to extract and skip over the bits + pieces and
to confirm that they form a sub declaration rather than anything else.

At this point it calls scan_str() to skip over the prototype. Here it goes
wrong, because PL_lex_stuff is still set, which causes scan_str() to think
it's scanning the second part of a s/// or tr///, and so sets PL_lex_repl
instead of PL_lex_stuff.

So that diff above just ensures that PL_lex_stuff is NULL before calling
scan_str() to skip over the prototype/signature again.

--
Monto Blanco... scorchio!

From @khwilliamson

On 4/26/19 3​:33 PM, Dave Mitchell wrote​:

On Wed, Apr 24, 2019 at 10​:26​:44AM -0700, Karl Williamson via RT wrote​:

If I comment out that assert, it thinks things are syntax errors, so it needs further investigation

The proximate cause is that signatures.xs not clearing PL_lex_stuff at a
certain point.

This Proof of Concept fix passes all tests in blead​:

--- signatures.xs- 2019-04-26 21​:53​:04.430901853 +0100
+++ signatures.xs 2019-04-26 22​:07​:32.304999812 +0100
@​@​ -151,6 +151,9 @​@​
return op;
}

+ assert(PL_lex_stuff == op_sv);
+ PL_lex_stuff = NULL;
+
tmp = hook_toke_scan_str (aTHX_ s);
tmp2 = hook_parser_get_lex_stuff (aTHX);
hook_parser_clear_lex_stuff (aTHX);

but I don't know whether it works with older perls, or why switching
away from the 'stolen' scan_str() to perl's version broke things.

Thanks for doing this. I was dreading having to work on this ticket, as
I don't much understand the lexing process (I can deal with the low
level, but not the grand view).

When run under older perls the stolen chunk is used, so it is lquite
ikely to work, and if not, it's a simple matter to make that code
dependent on the perl version.

What is happening is that perl parses a sub prototype by using scan_str()
to extract the balanced delimiter​: in thise case the '()'s in

 sub foo \($x\) \{ \.\.\. \}

scan_str() sets PL_lex_stuff to the extracted prototype, i.e. '$x'
It then creates an OP_CONST whose op_sv is set to PL_lex_stuff, and then
PL_lex_stuff is cleared, i.e. this code in yylex()​:

    if \(have\_proto\) \{
        NEXTVAL\_NEXTTOKE\.opval =
                     newSVOP\(OP\_CONST\, 0\, PL\_lex\_stuff\);
        PL\_lex\_stuff = NULL;
        force\_next\(THING\);
    \}

B-Hooks-Parser / signatures.xs hook into the ck_foo() mechanism, so that
the hook is called for the OP_CONST in the newSVOP() above. The hook
revisits the lexing stream that's just been processed to confirm whether
it is of the form 'sub maybe_a_name (...)', using calls to
hook_toke_skipspace() etc to extract and skip over the bits + pieces and
to confirm that they form a sub declaration rather than anything else.

At this point it calls scan_str() to skip over the prototype. Here it goes
wrong, because PL_lex_stuff is still set, which causes scan_str() to think
it's scanning the second part of a s/// or tr///, and so sets PL_lex_repl
instead of PL_lex_stuff.

So that diff above just ensures that PL_lex_stuff is NULL before calling
scan_str() to skip over the prototype/signature again.

I believe this means this ticket is no longer a blocker.

From @iabyn

On Fri, Apr 26, 2019 at 04​:16​:45PM -0600, Karl Williamson wrote​:

but I don't know whether it works with older perls, or why switching
away from the 'stolen' scan_str() to perl's version broke things.

Thanks for doing this. I was dreading having to work on this ticket, as I
don't much understand the lexing process (I can deal with the low level, but
not the grand view).

When run under older perls the stolen chunk is used, so it is lquite ikely
to work, and if not, it's a simple matter to make that code dependent on the
perl version.

I've now analysed it running under 5.28.0 and understand why it broke
later.

Blead's scan_str() has the property of saving the scanned string in
PL_lex_stuff, unless its a second call, in which case save it in
lex_sub_repl. That second value is later copied to PL_lex_repl in
sublex_push.

Older perls (and the stolen scan_str()) instead copy on second call
directly into PL_lex_repl.

In all cases signatures.xs was buggy not to clear PL_lex_stuff
before calling scan_str() again, but before, it happened not to trip up on
this faux pax.

So I think my fix is good - I'll mention it in the associated rt.cpan.org
ticket.

I'll also remove this from the blockers list.

--
A walk of a thousand miles begins with a single step...
then continues for another 1,999,999 or so.

From @karenetheridge

I've released 0.14 with this patch. Thank you very much!

@karenetheridge - Status changed from 'open' to 'resolved'