Bug #14731 for Perl-Critic: false positive: Builtin function called with parens

Sat Sep 24 06:40:08 2005 BORISZ [...] cpan.org - Ticket created

Subject:

false positive: Builtin function called with parens

Hi, For this code, I get while ( defined( my $child = shift @free_children ) ) {;} Builtin function called with parens at line 1, column 10. See page 13 of PBP. but without the parens it means: while ((defined(my $child) = shift @free_children ) ){;} which is wrong.

Wed Sep 28 01:01:08 2005 thaljef [...] cpan.org - Taken

Wed Sep 28 01:02:18 2005 thaljef [...] cpan.org - Status changed from 'new' to 'open'

Mon Oct 03 16:44:42 2005 kclark [...] cpan.org - Correspondence added

From:

kclark [...] cpan.org

I would like to add that I'm also getting this warning when a builtin that takes a list is an arg to another list-taking builtin, e.g.: print join( ':', qw[ foo bar baz ] ), "\n"; Where, of course, w/o the parens the join would put an extra colon after "baz" because it would eat the newline. ky

Mon Oct 03 17:24:29 2005 thaljef [...] cpan.org - Cc KCLARK added

Mon Oct 03 17:27:04 2005 thaljef [...] cpan.org - Correspondence added

[KCLARK - Mon Oct 3 16:44:42 2005]: Show quoted text

> I would like to add that I'm also getting this warning when a builtin > that takes a list is an arg to another list-taking builtin, e.g.: > > print join( ':', qw[ foo bar baz ] ), "\n"; > > Where, of course, w/o the parens the join would put an extra colon > after > "baz" because it would eat the newline. > > ky

This is a tough one. A strict interpretation of Conway would indicate that you should write the code differently so that the parens aren't needed. However, I don't think that's what he really had in mind. I'm still trying to figure out the right logic for making exceptions to this Policy. My first thought was to allow parens whenever the argument list contains an expression with some kind of operator (other than ','). But that still wouldn't cover this case. If you think of a good rule-of-thumb for when to allow parens, let me know. -Jeff

Thu Oct 13 11:35:26 2005 cpan [...] clotho.com - Correspondence added

From:

cdolan [...] cpan.org

[BORISZ - Sat Sep 24 06:40:08 2005]: Show quoted text

> Hi, > > For this code, I get > > while ( defined( my $child = shift @free_children ) ) {;} > > Builtin function called with parens at line 1, column 10. See page 13 > of PBP. > > but without the parens it means: > > while ((defined(my $child) = shift @free_children ) ){;} > > which is wrong. >

I have seen similar issues with ref(). I think this is a rule that just can't be universal without code rewriting. My workaround has sometimes been to simply add a "## no critic" flag to the line. Otherwise, altering the code may be beneficial. Perhaps a better way to rewrite your code would be via List::MoreUtils like so: use List::MoreUtils qw(firstidx); # Delete leading undef elements if (0 < (my $i = firstidx {defined $_} @free_children)) { splice(@free_children, $i); } which is actually more efficient since it only alters the array once instead of once per while() iteration. -- Chris

Fri Mar 03 12:12:18 2006 Guest - Correspondence added

From:

Paul Lalli

On Mon Oct 03 17:27:04 2005, THALJEF wrote: Show quoted text

> This is a tough one. A strict interpretation of Conway would indicate > that you should write the code differently so that the parens aren't > needed. However, I don't think that's what he really had in mind. I'm > still trying to figure out the right logic for making exceptions to this > Policy. My first thought was to allow parens whenever the argument list > contains an expression with some kind of operator (other than ','). But > that still wouldn't cover this case. If you think of a good > rule-of-thumb for when to allow parens, let me know.

I just ran into this with the *very* common idiom of: chomp (my $val = <STDIN>); As for the general rule, in English, it's obvious that parentheses should be allowed whenever precedence changes if they're not there. Now, how to implement that in your module, I have no idea. I do wonder if B::Deparse would be at all helpful, however. When running perl from the command line as: perl -MO=Deparse,-p file.pl the results will show all possible parentheses added, allowing us to see exactly what the precedence is. Paul Lalli

Sat Mar 04 02:06:27 2006 thaljef [...] cpan.org - Correspondence added

Show quoted text

> I just ran into this with the *very* common idiom of: > chomp (my $val = <STDIN>); > > As for the general rule, in English, it's obvious that parentheses > should be allowed whenever precedence changes if they're not there. > Now, how to implement that in your module, I have no idea. I do

wonder Show quoted text

> if B::Deparse would be at all helpful, however. When running perl

from Show quoted text

> the command line as: > perl -MO=Deparse,-p file.pl > the results will show all possible parentheses added, allowing us to

see Show quoted text

> exactly what the precedence is.

Thanks for bringing this up again (I had kinda forgotten about it). One of the philosophical features of Perl::Critic is that it doesn't actually execute any of the code it analyzes, so B::Deparse isn't really an option for us (remember that compiling the code causes BEGIN blocks to be executed). As you said, the ideal rule is to prohibit parens only when removing them would change the precedenece of the expression. I think that's doable, but it's probably more work than I'm willing to do right now. Instead, I might just come up with some simple exceptions (such as unary operators like 'chomp') that cover the most common cases. I'll work on this and try to improve it before the next release. -Jeff

Mon Mar 06 12:53:56 2006 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'

Mon Mar 06 12:53:57 2006 thaljef [...] cpan.org - Fixed in 0.14_01 added

Mon Mar 06 12:57:19 2006 thaljef [...] cpan.org - Correspondence added

It is difficult to apply this Policy with complete accuracy, but I have made some improvements that should reduce the frequency of hitting a false-positive. Currently, this feature is only available in a development release (0.14_01).

Mon Mar 06 12:57:19 2006 The RT System itself - Status changed from 'resolved' to 'open'

Mon Mar 06 12:58:20 2006 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'

Tue May 16 13:31:22 2006 Guest - Correspondence added

From:

Paul Lalli <lallip [...] cs.rpi.edu>

On Mon Oct 03 17:27:04 2005, THALJEF wrote: Show quoted text

> This is a tough one. A strict interpretation of Conway would indicate > that you should write the code differently so that the parens aren't > needed.

Point of order - that's plainly not true. The rule in PBP is "Don't use unnecessary parentheses for builtins and 'honorary' builtins." Keyword there is "unnecessary." The section (page 13, for those following along at home) goes on to say "Note, however, that in any cases wehre you find that you need to use parentheses in builtins, they should follow the rules for subroutines, not those for control keywords. That is, treat them as subroutines, with no space between the buitlin name and the opening parenthesis." Paul Lalli

Tue May 16 13:31:22 2006 The RT System itself - Status changed from 'resolved' to 'open'

Wed May 17 00:51:59 2006 thaljef [...] cpan.org - Correspondence added

There is still room for interpretation. I could be wrong, but I think any statement that requires parens could be factored into two or more statements that do require parens. Or in other words, if each statement only has one operator, then you never need parens. That may sound ridiculous, but consider the other extreme: forcing all your code into a single statement. So whether or not the parens are "necessary" depends on how compact (or verbose) you want the code to be. But in general, I agree that the sprit of Conway's rule is to ask "if the parens are removed from a given expression, does it still have the same meaning?" And that's exactly what Perl::Critic tries to do. The logic has improved a lot since this bug was first reported in version 0.07. If the parens are followed by an operator with higher precedence than the builtin function, or if the function gobbles all trailing arguments, then the parens are deemed necessary. I'm sure there are some edge cases that slip through the cracks, but I think it covers most cases. -Jeff

Tue Jun 13 23:22:59 2006 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #14731 for Perl-Critic: false positive: Builtin function called with parens

Preferred bug tracker