Bug #56624 for Perl-Critic: Double-sigil dereference $% (possibly false positive)

Thu Apr 15 06:47:42 2010 JARIAALTO [...] cpan.org - Ticket created

Subject:

Double-sigil dereference $% (possibly false positive)

Double-sigil dereference at line 7, column 7. See page 228 of PBP. 1 #!/usr/bin/perl 2 use strict; 3 4 my %hash; 5 my $ref = \%hash; 6 7 print %$href; The use of "%$" is quite common, likewise for the "@$ref"

Thu Apr 15 09:05:37 2010 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #56624] Double-sigil dereference $% (possibly false positive)
Date:	Thu, 15 Apr 2010 08:05:21 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

On 4/15/10 5:47 AM, Jari Aalto via RT wrote: Show quoted text

> The use of "%$" is quite common, likewise for the "@$ref"

Just because something is common doesn't make it right. Just because eval { blah }; if ($@) { blah blah } is common doesn't mean that it doesn't have a serious bug in it.

Thu Apr 15 09:05:40 2010 The RT System itself - Status changed from 'new' to 'open'

Thu Apr 15 09:14:41 2010 JARIAALTO [...] cpan.org - Correspondence added

On Thu Apr 15 09:05:37 2010, clonezone wrote: Show quoted text

> On 4/15/10 5:47 AM, Jari Aalto via RT wrote:

> > The use of "%$" is quite common, likewise for the "@$ref"

> > Just because something is common doesn't make it right. > > Just because > > eval { blah }; > if ($@) { > blah blah > } > > is common doesn't mean that it doesn't have a serious bug in it.

That's different. The bug in question is about: <sigil><sigil><full variable name> where <full variable name> := simple expressions as in [\w_]+ or something. That should be handled more carefully and not label everything <sigil><sigil> as suspect.

Wed Apr 21 21:42:33 2010 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #56624] Double-sigil dereference $% (possibly false positive)
Date:	Wed, 21 Apr 2010 20:42:19 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

On 4/15/10 8:14 AM, Jari Aalto via RT wrote: Show quoted text

> That should be handled more carefully and not label everything > <sigil><sigil> as suspect.

If you pay attention to what Conway is talking about, he is not discussing bugs. He's talking about communicating intent. A significant portion of Perl developers don't understand the precedence of the expression "$$foo->bar()": does it mean "${$foo->bar()}" or does it mean "${$foo}->bar()"? You and I may know the answer, but Conway's point is that most people do not, so he wants you to always separate your sigils. If you don't agree with the policy, disable it.

Wed Apr 21 21:43:01 2010 ELLIOTJS [...] cpan.org - Status changed from 'open' to 'rejected'

Sat Apr 24 15:03:32 2010 JARIAALTO [...] cpan.org - Correspondence added

On Wed Apr 21 21:42:33 2010, clonezone wrote: Show quoted text

> On 4/15/10 8:14 AM, Jari Aalto via RT wrote:

> > That should be handled more carefully and not label everything > > <sigil><sigil> as suspect.

> > If you pay attention to what Conway is talking about, he is not > discussing bugs. He's talking about communicating intent. A > significant portion of Perl developers don't understand the > precedence of the expression "$$foo->bar()": does it mean "${$foo-

> >bar()}" or does it mean "${$foo}->bar()"? You and I may know the

> answer, but Conway's point is that most people do not, so he wants > you to always separate your sigils.

Sure, a message for complex expressions is ok, but the bug report and my last comment was addressed to: <full variable name> := simple expression That doesn't include "$$foo->" Labeling everything suspect is not a good idea. If someone wants to to be fanatic, he may consider including cases like @$var (and the like) to be a suspect. But in practice it would be hard to find a Perl coder that would find code like above to be "ambiguous" or prone to "misinterpretation". Any added punctuation to that will make it less clear in spirit of "less is more" (according to the XP methodology in programming). TO RECAP --------- Fine tune the warning: - allow simple cases and do not emit warnings on those: <sigil><sigil><straight simple variable> Better: offer configuration to bypass simple cases and continue to emit warnings from more complex cases.

Sat Apr 24 15:03:33 2010 The RT System itself - Status changed from 'rejected' to 'open'

Sat Apr 24 15:54:18 2010 jeff [...] imaginative-software.com - Correspondence added

Subject:	Re: [rt.cpan.org #56624] Double-sigil dereference $% (possibly false positive)
Date:	Sat, 24 Apr 2010 12:54:00 -0700
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Jeffrey Thalhammer <jeff [...] imaginative-software.com>

On Apr 24, 2010, at 12:03 PM, Jari Aalto via RT wrote: Show quoted text

> TO RECAP > --------- > > Fine tune the warning: > > - allow simple cases and do not emit warnings on those: > <sigil><sigil><straight simple variable> > > Better: offer configuration to bypass simple cases and continue to emit > warnings from more complex cases.

I see your point, and I am strongly considering adding this flexibility. But aside from detecting bugs and making code easier to read, perlcritic also strives to enforce consistency. And sometimes, consistency is more valuable than flexibility. For new code, it is pretty easy to get into the habit of using curly-braces to separate the sigils in both complex and simple expressions. But when critiquing legacy code, it is tempting to try and bend Perl::Critic to fit your existing habits. Sometimes this makes perfect sense, and sometimes not. I think the key thing to consider is whether one truly objects to a Policy (such as it is), or if he/she just don't have the time & will to make all their code compliant. Your mileage may vary. Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf

Bug #56624 for Perl-Critic: Double-sigil dereference $% (possibly false positive)

Preferred bug tracker