Detecting 'our $foo => 1' mistake?

[perlancar]

Dear monks,

I just wasted half an hour trying to find a cause why a variable is undefined (and chasing the wrong rabbit of trying to figure out why a particular module won't load but require() doesn't raise an error), before finally realizing that I made a typo:

our $foo => { a=>'blah', b=>'blah', ... };

instead of:

our $foo = { a=>'blah', b=>'blah', ... };

And come to think of it, this is perhaps my third or fourth time being bitten by this. This mistake is not caught by warnings, so any suggestion on how to detect this? Perhaps there's a Perl::Critic policy somewhere and I need to just (re-)plunge and use Perl::Critic (again).

UPDATE 2017-01-23: my bad, it turns out I didn't use warnings in the original code. our $foo => value indeed produces warning in most cases.

[haukex]

Hi perlancar,

I whipped up a Perl::Critic policy for you because I think it might also be useful to others... drop the following into, for example, /tmp/testlib/Perl/Critic/Policy/Variables/ProhibitFatCommaInDeclaration.pm

Read more... (1162 Bytes)

Then:

$ cat testpolicy.pl
#!/usr/bin/env perl
use warnings;
use strict;

my $foo => 'bar';
our $foo => 1;
our $foo => { a=>'blah', b=>'blah' };
our $foo = { a=>'blah', b=>'blah' };
my %foo = ( a=>1, b=>2 );

$ PERL5LIB=/tmp/testlib perlcritic -4 testpolicy.pl
Fat comma used in declaration at line 5, column 9.  You probably meant
+ "=" instead of "=>".  (Severity: 4)
Fat comma used in declaration at line 6, column 10.  You probably mean
+t "=" instead of "=>".  (Severity: 4)
Fat comma used in declaration at line 7, column 10.  You probably mean
+t "=" instead of "=>".  (Severity: 4)

Really just a quick one for now, so I haven't done any further testing except the above, but I hope it's a start :-)

Update: You could perhaps monkey-patch this into your Perl::Critic installation by copying ProhibitFatCommaInDeclaration.pm into the same directory as the one shown when you say perldoc -l Perl::Critic::Policy::Variables::ProhibitLocalVars (untested). Or perhaps you've already got a local::lib setup. Of course, this could be turned into a real module distro too, but I haven't gotten that far :-)

Hope this helps,
-- Hauke D

[Anonymous Monk]

This mistake is not caught by warnings,

Sure it is

$ perl -Mstrict -Mwarnings -le " our $shy => (); "

$ perl -Mstrict -Mwarnings -le " our $shy => (0); "

$ perl -Mstrict -Mwarnings -le " our $shy => (1); "

$ perl -Mstrict -Mwarnings -le " our $shy => (2); "
Useless use of a constant (2) in void context at -e line 1.

$ perl -Mstrict -Mwarnings -le " our $shy => []; "
Useless use of anonymous list ([]) in void context at -e line 1.

$ perl -Mstrict -Mwarnings -le " our $shy => {}; "
Useless use of anonymous hash ({}) in void context at -e line 1.

$ perl -Mstrict -Mwarnings -le " our $shy => q{}; "
Useless use of a constant ("") in void context at -e line 1.

[haukex]

Hi perlancar,

Could you provide an SSCCE that demonstrates the problem? As the AM already pointed out, the code you showed does provide a warning:

$ perl -wMstrict -e 'our $foo => { a=>"blah", b=>"blah" };'
Useless use of anonymous hash ({}) in void context at -e line 1.

Update: Ok, the code you showed in the title does not warn. Standard Perl::Critic doesn't seem to catch this one either, and I don't yet see any add-on policies that catch this. Update 2: See my other reply.

$ perl -wMstrict -e 'our $foo => 1;'
$

Regards,
-- Hauke D

[LanX]

Without being able to test I think the parser sees a list in void context, which is reduced to the last statement.

The => is just a fat comma and the declaration resets the var to undef.

So our $foo => 1; -> undef, 1;

I suppose that at runtime the scalar comma operator will only try to "return" the last statement to void (I know this sounds weird°)

And 0 and 1 are allowed in void (think modules needing a true return value).

That's why the error is only caught for other values.

°) from Programming Perl

Binary "," is the comma operator. In scalar context it evaluates its left argument in void context, throws that value away, then evaluates its right argument in scalar context and returns that value.

[haukex]

Hi Rolf,

Yes, I think you're right:

$ perl -MO=Deparse,-p -e 'our $foo => 1;'
(our($foo), '???');

Regards,
-- Hauke D

[ikegami]

0 and 1 in void context don't warn so you can do stuff like 1 while s/.../.../;.