Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 72737
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: john.deighan [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.116
Fixed in: (no value)



Subject: Parsing bug for return statement
Download (untitled) / with headers
text/plain 204b
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
Download test.pl
text/x-perl 276b
# 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'; }
Download (untitled) / with headers
text/plain 1.7k
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
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>
Download (untitled) / with headers
text/plain 2.7k
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 >
Download (untitled) / with headers
text/plain 2.3k
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.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.