Bug #122179 for Keyword-Declare: The switch to PPR is a tad unfortunate wrt error message quality

Thu Jun 22 16:11:03 2017 mst [...] shadowcat.co.uk - Ticket created

Subject:	The switch to PPR is a tad unfortunate wrt error message quality
Date:	Thu, 22 Jun 2017 20:10:47 +0000
To:	bugs-Keyword-Declare [...] rt.cpan.org
From:	Matt S Trout <mst [...] shadowcat.co.uk>

So, having already noted that the way Method::Signatures::PP uses PPR makes it great for working code but produce terrible errors if not, I realised that for things like block keywords the problem might be more general than just my LT hack. After a bit of playing ... yes, yes it is. I took t/attributes_basic_unified.t from Dios (picked basically at random) and then changed @nums = @NEWNUMS; to @nums = @NEWNUMS: since this is a moderately common mistake for me and also one that often annoys me to debug. With Dios 0.001001 and Keyword::Declare 0.000005 that results in: Matt@demeisen$ perl -I$HOME/tmp/oldkwdeclare/lib/perl5 t/attributes_basic_unified.t syntax error at t/attributes_basic_unified.t line 39, near "@NEWNUMS:" Execution of t/attributes_basic_unified.t aborted due to compilation errors. With Dios 0.002000 and Keyword::Declare 0.001000 that results in: Matt@demeisen$ perl -Ilib t/attributes_basic_unified.t Invalid class at t/attributes_basic_unified.t line 22. Expected: class <class name> <bases> <block> but found: class Demo is Base1 is Base2 { Compilation failed at t/attributes_basic_unified.t line 22. which is, well, accurate, but *vastly* less helpful. This suggests three options that I can immediately think of: (1) Use PPI for my stuff and rely on .pmc-ification ameliorating the speed (2) Use PPR, then fall back to PPI if PPR doesn't parse it, since if you're about to crash anyway taking an extra .5s to crash more usefully is fine (3) Find some way to make PPR report partial parses, which probably ends up heading towards PPR having to duplicating perl5's compilation errors as well as parsing behaviour My initial thoughts are that (1) would be fine for *me*, (2) is possibly the least worst option for Keyword::Declare, and (3) is a great idea in theory but a vast amount of work for not much payoff in practice. Thoughts? -- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

Thu Jun 22 21:11:39 2017 DCONWAY [...] cpan.org - Status changed from 'new' to 'resolved'

Thu Jun 22 21:11:50 2017 damian [...] conway.org - Correspondence added

Subject:	Re: [rt.cpan.org #122179] The switch to PPR is a tad unfortunate wrt error message quality
Date:	Thu, 22 Jun 2017 21:10:59 -0400
To:	bug-Keyword-Declare [...] rt.cpan.org
From:	Damian Conway <damian [...] conway.org>

Hi Matt, Thanks for your considered and thoughtful feedback. I certainly agree that Keyword::Declare's error messages are currently Less Than Awesome when the PPR-based parser fails to match. To improve that, I have added a new variable to PPR: $PPR::ERROR This variable is set iff it is undefined when a PPR-based parse encounters an unparsable (?&PerlStatement), in which case the grammar attempts to isolate the offending code that caused that subrule to fail, and returns an object via through that code can be identified and located. With that, I upgraded Keyword::Declare's error reporter to include the following: my $error = eval "no strict; sub{ $PPR::ERROR }" ? $expected_syntax : $@ =~ s{^}{ }gmr =~ s{$eval \d++$ line \d++} { "$filename line " . $PPR::ERROR->line($linenum) }reg; In other words, I interpolate the bad code into an anonymous sub and compile it via string eval. If it is a genuine syntax error, the failed eval leaves the appropriate error message in $@, whose file and line number I massage back to the correct location using the information provided by $PPR::ERROR. Otherwise, I fall back on using the current vastly less useful error message (on the principle that it's still better than nothing). This is more-or-less a variation on your option 3, and might well work for you too with Method::Signatures::PP. So far it seems to be doing a much better job for Keyword::Declare (though, of course, I would welcome any further feedback on that score). Thanks again for inspiring these significant improvements in both PPR and Keyword::Declare. Damian

Fri Jun 23 12:42:52 2017 mst [...] shadowcat.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #122179] The switch to PPR is a tad unfortunate wrt error message quality
Date:	Fri, 23 Jun 2017 16:42:35 +0000
To:	"damian [...] conway.org via RT" <bug-Keyword-Declare [...] rt.cpan.org>
From:	Matt S Trout <mst [...] shadowcat.co.uk>

On Thu, Jun 22, 2017 at 09:11:50PM -0400, damian@conway.org via RT wrote: Show quoted text

> In other words, I interpolate the bad code into an anonymous sub and compile > it via string eval. If it is a genuine syntax error, the failed eval leaves > the > appropriate error message in $@, whose file and line number I massage > back to the correct location using the information provided by $PPR::ERROR.

I think that probably needs to be hookable so you can mangle the erroring code first - otherwise if the bad statement contains e.g. a reference to $self you're going to get a strict failure. Recreating the entire lexical environment of that piece of code is probably tricky at best but at least making sure the keyword can inject any my declarations it's adding would be good. -- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

Fri Jun 23 12:47:41 2017 mst [...] shadowcat.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #122179] The switch to PPR is a tad unfortunate wrt error message quality
Date:	Fri, 23 Jun 2017 16:47:33 +0000
To:	"damian [...] conway.org via RT" <bug-Keyword-Declare [...] rt.cpan.org>
From:	Matt S Trout <mst [...] shadowcat.co.uk>

On Thu, Jun 22, 2017 at 09:11:50PM -0400, damian@conway.org via RT wrote: Show quoted text

> With that, I upgraded Keyword::Declare's error reporter to include > the following: > > my $error = eval "no strict; sub{ $PPR::ERROR }" > ? $expected_syntax > : $@ =~ s{^}{ }gmr > =~ s{$eval \d++$ line \d++} > { "$filename line " . > $PPR::ERROR->line($linenum) }reg;

Bah. (1) I'm dumb, I didn't see the 'no strict', but I'd like to still get other strict errors so I think a hook would still be better (2) Thought: eval "no strict;\n#line ...\n sub{ $PPR::ERROR }" might be more reliable? -- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

Fri Jun 23 20:14:32 2017 damian [...] conway.org - Correspondence added

Subject:	Re: [rt.cpan.org #122179] The switch to PPR is a tad unfortunate wrt error message quality
Date:	Fri, 23 Jun 2017 20:13:16 -0400
To:	bug-Keyword-Declare [...] rt.cpan.org
From:	Damian Conway <damian [...] conway.org>

I've thought about it at some length and I'm probably not going to add another hook. Currently the error API can provide the offending code ($PPR::ERROR->source) as well as the non-offending code that precedes it ($PPR::ERROR->prefix)...so you could reconstruct the full leading context within the eval if you really wanted to. I did try adding the #line there, but actually found it *less* reliable than post-mangling the locations by hand. That said, the error API now also provides $PPR::ERROR->origin, which lets you set file and line before interrogating for errors. Bear in mind that PPR (like PPI) treats Perl code as a document, so I think it's pretty reasonable for it's error messages to be merely syntactic, rather than semantic. That said, if you come up with any useful techniques for improving M::S::PP's error reporting I'll be more than happy to steal the final code for K::D and PPR as well. ;-) Damian

Mon Jun 26 12:41:26 2017 mst [...] shadowcat.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #122179] The switch to PPR is a tad unfortunate wrt error message quality
Date:	Mon, 26 Jun 2017 16:41:11 +0000
To:	"damian [...] conway.org via RT" <bug-Keyword-Declare [...] rt.cpan.org>
From:	Matt S Trout <mst [...] shadowcat.co.uk>

On Fri, Jun 23, 2017 at 08:14:37PM -0400, damian@conway.org via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=122179 > > > I've thought about it at some length and > I'm probably not going to add another hook.

That's reasonable; I'm probably going to need to initially steal parts of Keyword::Declare rather than building directly on top of it anyway, so I think "show you what I've got once I've got more working and we can discuss whether it's worth re-integrating on the merits based on running code" is likely the best answer anyway. I'm not right now entirely certain my argument for a Keyword::Declare level hook is convincing anyway, so as long as "probably not" includes "unless/until somebody comes up with a properly convincing argument for it" I'm totally happy with the current state of play. Thanks for talking this out with me, the existing PPR-level stuff is I believe spot on and the Keyword::Declare related discussion has helped me clarify my plans even in the spots where you've said "no" for the moment :) -- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support, training and consultancy packages could help your team.

Mon Jun 26 16:09:39 2017 damian [...] conway.org - Correspondence added

Subject:	Re: [rt.cpan.org #122179] The switch to PPR is a tad unfortunate wrt error message quality
Date:	Tue, 27 Jun 2017 06:08:52 +1000
To:	bug-Keyword-Declare [...] rt.cpan.org
From:	Damian Conway <damian [...] conway.org>

Thanks, Matt. I very much appreciated our discussion, which has allowed me to materially improve both PPR and Keyword::Declare. I will welcome any further suggestions you may have down the track. Damian