Parsing bug for return statement

[thaljef]

RT Ticket: http://rt.cpan.org/Ticket/Display.html?id=72737
Requested On: Sat Nov 26 06:24:22 2011
Requested By: john.deighan@gmail.com

---

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.

[thaljef]

RT Ticket: http://rt.cpan.org/Ticket/Display.html?id=72737
Comment On: 2011-11-26 21:41:47
Comment By: wyant@cpan.org

---

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

[thaljef]

RT Ticket: http://rt.cpan.org/Ticket/Display.html?id=72737
Comment On: 2011-11-28 19:20:53
Comment By: jdeighan@cpan.org

---

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:

<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

[thaljef]

RT Ticket: http://rt.cpan.org/Ticket/Display.html?id=72737
Comment On: 2011-11-30 01:50:23
Comment By: wyant@cpan.org

---

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

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.

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.

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.

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.

[wchristian]

A fix for this has been released as a dev version to CPAN. Please test and give feedback here:

Perl-Critic/PPI#92

[wchristian]

The full version of the fix for this is now released to CPAN as PPI 1.220.

[thaljef]

This is fixed in Perl-Critic-1.123 via PPI-1.220