Bug #72737 for Perl-Critic: Parsing bug for return statement

Sat Nov 26 09:24:22 2011 john.deighan [...] gmail.com - Ticket created

Subject:

Parsing bug for return statement

The attached file compiles just fine, but Perl::Critic gives incorrect warnings, apparently because the return statement doesn't have whitespace between the word 'return' and the following string literal.

Subject:

test.pl

# test.pl use strict; use warnings; no warnings qw(uninitialized numeric); # ------------------------------------ MAIN: { my $str = f(); print("str = '$str'\n"); } # MAIN() # ------------------------------------ sub f { return'ProposedOverdue.png'; }

Sat Nov 26 16:41:47 2011 wyant [...] cpan.org - Correspondence added

On Sat Nov 26 09:24:22 2011, jdeighan wrote: Show quoted text

> The attached file compiles just fine, but Perl::Critic gives incorrect > warnings, apparently because the return statement doesn't have > whitespace between the word 'return' and the following string literal.

You don't say what warnings you object to, but running your example I presume it is Subroutine "f" does not end with "return" at line 16, column 1. See page 197 of PBP. (Severity: 4) The line in question is return'ProposedOverdue.png'; Perl::Critic relies on PPI to do its parsing for it, and the PPI parse does not find a 'return' in subroutine f(). PPI produces the following parse for the file starting at the declaration of subroutine f(): PPI::Statement::Sub [ 16, 1, 1 ] PPI::Token::Word 'sub' [ 16, 5, 5 ] PPI::Token::Word 'f' PPI::Structure::Block { ... ??? PPI::Statement [ 18, 1, 1 ] PPI::Token::Word 'return'ProposedOverdue' [ 18, 23, 23 ] PPI::Token::Operator '.' [ 18, 24, 24 ] PPI::Token::Word 'png' [ 18, 27, 27 ] PPI::Token::Quote::Single '';\n}\n' In other words, without the space after the 'return', PPI thinks that it has a Perl 4 package reference (i.e. the equivalent of return::ProposedOverdue), and the rest of the parse is basically garbage. PPI does not fail, because when run against what it thinks is invalid Perl, its design goal appears to be not to die horribly. You can try filing a ticket against PPI, referring to this one. But this is a case where I would really advise that you just put a space after the 'return', since I'm inclined to regard Perl's 'correct' parse of this as a bug in Perl. Tom

Sat Nov 26 16:41:48 2011 The RT System itself - Status changed from 'new' to 'open'

Sat Nov 26 16:41:49 2011 wyant [...] cpan.org - Status changed from 'open' to 'stalled'

Mon Nov 28 14:20:53 2011 john.deighan [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #72737] Parsing bug for return statement
Date:	Mon, 28 Nov 2011 14:20:43 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	John Deighan <john.deighan [...] gmail.com>

Actually, I got both of these warnings: Subroutine "f" does not end with "return" at line 19, column 1. See page 197 of PBP. Literal line breaks in a string at line 21, column 27. See pages 60,61 of PBP. More importantly, though, when I run the script, the function returns the correct value. So, clearly, the PPI parser is NOT doing what the actual Perl parser does, so in my opinion, PPI is wrong simply because Perl is "right" by definition. I will, of course, always put a space after the word 'return' (this came up simply because I made a global editing change that ended up creating the problematic construct), and I will submit a bug report for PPI, though I suspect that they will take your point of view, not mine ;-) Thanks. On Sat, Nov 26, 2011 at 4:41 PM, Tom Wyant via RT <bug-Perl-Critic@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=72737 > > > On Sat Nov 26 09:24:22 2011, jdeighan wrote:

>> The attached file compiles just fine, but Perl::Critic gives incorrect >> warnings, apparently because the return statement doesn't have >> whitespace between the word 'return' and the following string literal.

> > You don't say what warnings you object to, but running your example I > presume it is > > Subroutine "f" does not end with "return" at line 16, column 1. See > page 197 of PBP. (Severity: 4) > > The line in question is > > return'ProposedOverdue.png'; > > Perl::Critic relies on PPI to do its parsing for it, and the PPI parse > does not find a 'return' in subroutine f(). PPI produces the following > parse for the file starting at the declaration of subroutine f(): > > PPI::Statement::Sub > [ 16, 1, 1 ] PPI::Token::Word 'sub' > [ 16, 5, 5 ] PPI::Token::Word 'f' > PPI::Structure::Block { ... ??? > PPI::Statement > [ 18, 1, 1 ] PPI::Token::Word 'return'ProposedOverdue' > [ 18, 23, 23 ] PPI::Token::Operator '.' > [ 18, 24, 24 ] PPI::Token::Word 'png' > [ 18, 27, 27 ] PPI::Token::Quote::Single '';\n}\n' > > In other words, without the space after the 'return', PPI thinks that it > has a Perl 4 package reference (i.e. the equivalent of > return::ProposedOverdue), and the rest of the parse is basically > garbage. PPI does not fail, because when run against what it thinks is > invalid Perl, its design goal appears to be not to die horribly. > > You can try filing a ticket against PPI, referring to this one. But this > is a case where I would really advise that you just put a space after > the 'return', since I'm inclined to regard Perl's 'correct' parse of > this as a bug in Perl. > > Tom >

Mon Nov 28 14:20:54 2011 The RT System itself - Status changed from 'stalled' to 'open'

Tue Nov 29 20:50:23 2011 wyant [...] cpan.org - Correspondence added

On Mon Nov 28 14:20:53 2011, jdeighan wrote: Show quoted text

> Actually, I got both of these warnings: > > Subroutine "f" does not end with "return" at line 19, column 1. See > page 197 of PBP. > Literal line breaks in a string at line 21, column 27. See pages 60,61 > of PBP. >

Thanks. Here is where I think the second one came from. One of PPI's explicit goals it to be round-trip safe. That is, if you feed it some Perl, you should be able to get the same Perl back out of it. So when PPI decided to parse "return'ProposedOverdue.png';" as Perl 4 package name "return'ProposedOverdue", it had to do something with the unclosed quoted string that this decision created. So it just returned it as is. Perl::Critic looked at this parse, and generated the spurious "Literal line breaks" error. Show quoted text

> More importantly, though, when I run the script, the function returns > the correct value. So, clearly, the PPI parser is NOT doing what the > actual Perl parser does, so in my opinion, PPI is wrong simply because > Perl is "right" by definition.

This argument has been made before. The counter-example I had in mind was 'for $x qw{Larry Moe Curly} {...}', with no parentheses around the list. Perl never complained about this, until 5.14 when it became deprecated. The true situation is that Perl is only right until the Perl maintainers decide it is wrong. Show quoted text

> I will, of course, always put a space > after the word 'return' (this came up simply because I made a global > editing change that ended up creating the problematic construct),

Thank you. And thank you for submitting your report. You found something interesting, and let us know, which is the right thing to do. Show quoted text

> and > I will submit a bug report for PPI, though I suspect that they will > take your point of view, not mine ;-) Thanks.

Well, I confess my opinion that fixing this may take an amount of time disproportionate to the benefit gained. And it may be impossible to fix it completely. It is known that you can not statically parse Perl. That is, you can not be guaranteed to construct a correct parse of a Perl program simply from looking at the program. But this is what PPI tries to do. Usually it does it pretty well. But if duplicating Perl's parse in this case depends on knowing that the first element of the offending construct is a subroutine name and what its prototype (if any) is, than I suspect that duplicating Perl's parse in all cases may be out of PPI's reach.

Bug #72737 for Perl-Critic: Parsing bug for return statement

Preferred bug tracker