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: 65465
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: user42 [...] zip.com.au
Cc:
AdminCc:

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



From gg [...] zip.com.au Fri Feb 4 18: 06:42 2011
MIME-Version: 1.0
X-Spam-Status: No, score=-6.894 tagged_above=-99.9 required=10 tests=[AWL=-0.004, BAYES_00=-1.9, RCVD_IN_DNSWL_HI=-5, T_TVD_MIME_NO_HEADERS=0.01] autolearn=ham
X-Spam-Flag: NO
X-Virus-Checked: Checked by ClamAV on 16.mx.develooper.com
Content-Type: multipart/mixed; boundary="=-=-="
Message-ID: <87d3n79z7p.fsf [...] blah.blah>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
X-Spam-Score: -6.894
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 4DB6224153A for <cpan-bug+Perl-Critic [...] hipster.bestpractical.com>; Fri, 4 Feb 2011 18:06:42 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id wb-8reLr18TK for <cpan-bug+Perl-Critic [...] hipster.bestpractical.com>; Fri, 4 Feb 2011 18:06:40 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id 7271024151A for <bug-Perl-Critic [...] rt.cpan.org>; Fri, 4 Feb 2011 18:06:40 -0500 (EST)
Received: (qmail 12865 invoked by uid 103); 4 Feb 2011 23:06:39 -0000
Received: from x16.dev (10.0.100.26) by x1.dev with QMQP; 4 Feb 2011 23:06:39 -0000
Received: from mailout2-2.pacific.net.au (HELO mailout2.pacific.net.au) (61.8.2.225) by 16.mx.develooper.com (qpsmtpd/0.80) with ESMTP; Fri, 04 Feb 2011 15:06:37 -0800
Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout2.pacific.net.au (Postfix) with ESMTP id 1AC4E2AE718 for <bug-Perl-Critic [...] rt.cpan.org>; Sat, 5 Feb 2011 10:06:34 +1100 (EST)
Received: from blah.blah (unknown [203.26.175.90]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id 47F628C04 for <bug-Perl-Critic [...] rt.cpan.org>; Sat, 5 Feb 2011 10:06:33 +1100 (EST)
Received: from gg by blah.blah with local (Exim 4.72) (envelope-from <gg [...] zip.com.au>) id 1PlUja-0002OJ-Uc for bug-Perl-Critic [...] rt.cpan.org; Sat, 05 Feb 2011 10:06:34 +1100
Delivered-To: cpan-bug+Perl-Critic [...] hipster.bestpractical.com
User-Agent: Gnus/5.110011 (No Gnus v0.11) Emacs/23.2 (gnu/linux)
Subject: ProhibitUnusedVariables notice readline <$fh>
Return-Path: <gg [...] zip.com.au>
X-RT-Mail-Extension: perl-critic
X-Original-To: cpan-bug+Perl-Critic [...] hipster.bestpractical.com
X-Spam-Check-BY: 16.mx.develooper.com
Date: Sat, 05 Feb 2011 10:06:34 +1100
X-Spam-Level:
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
Content-Length: 0
X-RT-Original-Encoding: ascii
content-type: text/plain; charset="utf-8"
Content-Length: 490
Download (untitled) / with headers
text/plain 490b
ProhibitUnusedVariables doesn't seem to know that a readline style <$foo> is a use of the variable $foo. I get some joy from the change below. I struck this in the second .run addition, open (my $foo, '<', '/etc/motd') or die; my $line = <$foo>; I suppose it might also recognise that open(my $foo, ...) initializes $foo by side-effect, if the intention of the policy is to pick up only vars which are both uninitialized and then unused. Dunno if that would be a bit tricky.
Content-Type: text/x-diff
content-disposition: inline; filename="unused-readline.diff"
Content-Length: 2259
Index: lib/Perl/Critic/Policy/Variables/ProhibitUnusedVariables.pm =================================================================== --- lib/Perl/Critic/Policy/Variables/ProhibitUnusedVariables.pm (revision 4013) +++ lib/Perl/Critic/Policy/Variables/ProhibitUnusedVariables.pm (working copy) @@ -41,6 +41,7 @@ my %symbol_usage; _get_symbol_usage( \%symbol_usage, $document ); _get_regexp_symbol_usage( \%symbol_usage, $document ); + _get_readline_usage( \%symbol_usage, $document ); return if not %symbol_usage; my $declarations = $document->find('PPI::Statement::Variable'); @@ -114,6 +115,28 @@ return; } +# Look for PPI::Token::QuoteLike::Readline of <$foo>, which is handle $foo +# being used. +# Sometimes PPI (as of version 1.213) parses <$foo> as operator '<', symbol +# $foo, operator '>', eg. "print <$foo>". That's picked up by +# _get_symbol_usage() above. +# +sub _get_readline_usage { + my ( $symbol_usage, $document ) = @_; + + my $readlines = $document->find('PPI::Token::QuoteLike::Readline') + || return; # if none + + foreach my $readline ( @{$readlines} ) { + ### readline content: $readline->content + if ($readline->content =~ /^<(\$.*)>$/) { + ### add: $1 + $symbol_usage->{$1}++; + } + } + + return; +} #----------------------------------------------------------------------------- 1; Index: t/Variables/ProhibitUnusedVariables.run =================================================================== --- t/Variables/ProhibitUnusedVariables.run (revision 4013) +++ t/Variables/ProhibitUnusedVariables.run (working copy) @@ -162,6 +162,24 @@ #----------------------------------------------------------------------------- +## name Variable used in <$foo> readline +## failures 0 +## cut + +my $foo; +my $line = <$foo>; + +#----------------------------------------------------------------------------- + +## name Variable used in open() and <$foo> +## failures 0 +## cut + +open (my $foo, '<', '/etc/motd') or die; +my $line = <$foo>; + +#----------------------------------------------------------------------------- + ############################################################################## # $URL$ # $Date$
MIME-Version: 1.0
In-Reply-To: <87d3n79z7p.fsf [...] blah.blah>
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
References: <87d3n79z7p.fsf [...] blah.blah>
Content-Type: text/plain; charset="UTF-8"
Message-ID: <rt-3.8.HEAD-9726-1296925879-748.65465-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 1210
Download (untitled) / with headers
text/plain 1.1k
On Fri Feb 04 18:06:42 2011, user42@zip.com.au wrote: Show quoted text
> ProhibitUnusedVariables doesn't seem to know that a readline style > <$foo> is a use of the variable $foo. I get some joy from the change > below. > > I struck this in the second .run addition, > > open (my $foo, '<', '/etc/motd') or die; > my $line = <$foo>; > > I suppose it might also recognise that open(my $foo, ...) initializes > $foo by side-effect, if the intention of the policy is to pick up only > vars which are both uninitialized and then unused. Dunno if that would > be a bit tricky. > >
Thank you for the report. This is a specific case of RT #64929, which simply notes that ProhibitUnusedVariables does not find all unused variables. I'm attempting to put a few more teeth in this policy, and believe me, the devil is in the details. The current philosophy is that initialization does not count as use, so open my $fh, '<', 'foo.bar'; would be a violation unless the file was actually read. On the other hand, while ( <$fh> ) { ... } would count as a use of the variable, addressing your specific concern. Whether this makes the upcoming release is another question - I rather suspect it will not, I'm afraid. Tom
From gg [...] zip.com.au Mon Feb 7 19: 09:29 2011
MIME-Version: 1.0
X-Spam-Status: No, score=-6.899 tagged_above=-99.9 required=10 tests=[AWL=0.001, BAYES_00=-1.9, RCVD_IN_DNSWL_HI=-5] autolearn=ham
In-Reply-To: <rt-3.8.HEAD-9726-1296925879-1355.65465-6-0 [...] rt.cpan.org> (Tom Wyant via's message of "Sat, 5 Feb 2011 12:11:19 -0500")
X-Spam-Flag: NO
References: <RT-Ticket-65465 [...] rt.cpan.org> <87d3n79z7p.fsf [...] blah.blah> <rt-3.8.HEAD-9726-1296925879-1355.65465-6-0 [...] rt.cpan.org>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
Message-ID: <87ei7j9yku.fsf [...] blah.blah>
Content-Type: text/plain; charset="utf-8"
X-RT-Original-Encoding: utf-8
X-Spam-Score: -6.899
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 0560524150A for <cpan-bug+Perl-Critic [...] hipster.bestpractical.com>; Mon, 7 Feb 2011 19:09:29 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id v73sJ+i4FZ1P for <cpan-bug+Perl-Critic [...] hipster.bestpractical.com>; Mon, 7 Feb 2011 19:09:27 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id A998824136B for <bug-Perl-Critic [...] rt.cpan.org>; Mon, 7 Feb 2011 19:09:26 -0500 (EST)
Received: (qmail 23805 invoked by uid 103); 8 Feb 2011 00:09:25 -0000
Received: from x16.dev (10.0.100.26) by x1.dev with QMQP; 8 Feb 2011 00:09:25 -0000
Received: from mailout2-2.pacific.net.au (HELO mailout2.pacific.net.au) (61.8.2.225) by 16.mx.develooper.com (qpsmtpd/0.80) with ESMTP; Mon, 07 Feb 2011 16:09:23 -0800
Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.2.163]) by mailout2.pacific.net.au (Postfix) with ESMTP id F09782ADEBA for <bug-Perl-Critic [...] rt.cpan.org>; Tue, 8 Feb 2011 11:09:17 +1100 (EST)
Received: from blah.blah (unknown [203.26.175.158]) by mailproxy2.pacific.net.au (Postfix) with ESMTP id 6A26D2742E for <bug-Perl-Critic [...] rt.cpan.org>; Tue, 8 Feb 2011 11:09:17 +1100 (EST)
Received: from gg by blah.blah with local (Exim 4.72) (envelope-from <gg [...] zip.com.au>) id 1Pmb8z-0002On-If for bug-Perl-Critic [...] rt.cpan.org; Tue, 08 Feb 2011 11:09:21 +1100
Delivered-To: cpan-bug+Perl-Critic [...] hipster.bestpractical.com
Subject: Re: [rt.cpan.org #65465] ProhibitUnusedVariables notice readline <$fh>
User-Agent: Gnus/5.110011 (No Gnus v0.11) Emacs/23.2 (gnu/linux)
Return-Path: <gg [...] zip.com.au>
X-Spam-Check-BY: 16.mx.develooper.com
X-Original-To: cpan-bug+Perl-Critic [...] hipster.bestpractical.com
X-RT-Mail-Extension: perl-critic
Date: Tue, 08 Feb 2011 11:09:21 +1100
X-Spam-Level:
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
RT-Message-ID: <rt-3.8.HEAD-17550-1297123769-872.65465-0-0 [...] rt.cpan.org>
Content-Length: 504
Download (untitled) / with headers
text/plain 504b
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > The current philosophy is that > initialization does not count as use, so
Umm, I think initialization currently does count as use, so for instance my $guard = Scope::Guard->new (sub { some_cleanup() }); passes. Dunno if that should remain so. warnings::unused reports it as unused. As long as it's not too hard to flag as something created for block-scoped purposes then it could be worth reporting initialized but otherwise unused.
MIME-Version: 1.0
In-Reply-To: <rt-3.8.HEAD-17550-1297123769-872.65465-0-0 [...] rt.cpan.org>
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
References: <RT-Ticket-65465 [...] rt.cpan.org> <87d3n79z7p.fsf [...] blah.blah> <rt-3.8.HEAD-9726-1296925879-1355.65465-6-0 [...] rt.cpan.org> <87ei7j9yku.fsf [...] blah.blah> <rt-3.8.HEAD-17550-1297123769-872.65465-0-0 [...] rt.cpan.org>
Content-Type: text/plain; charset="UTF-8"
Message-ID: <rt-3.8.HEAD-19313-1297180834-1286.65465-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 2442
Download (untitled) / with headers
text/plain 2.3k
On Mon Feb 07 19:09:29 2011, user42@zip.com.au wrote: Show quoted text
> "Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes:
> > > > The current philosophy is that > > initialization does not count as use, so
> > Umm, I think initialization currently does count as use, so for instance > > my $guard = Scope::Guard->new (sub { some_cleanup() }); > > passes. Dunno if that should remain so. warnings::unused reports it as > unused. As long as it's not too hard to flag as something created for > block-scoped purposes then it could be worth reporting initialized but > otherwise unused.
That's one that could go either way, and it depends on what you think is a use. As you note, warnings::unused thinks it's unused. And certainly my $VERSION = something-or-other; which appears in a surprising number of CPAN modules, does not represent a use of $VERSION in any way I can think of. But you are right that sentinel variables are used in a meaningful sense of the word even though they are initialized and then not otherwise referred to in the code. I was going to provide a configuration variable to cause the policy to accept otherwise-unused variables computed by the code of your choice: [ProhibitUnusedVariables] allow_if_computed_by = Scope::Guard unpack stat This is a fairly simple-minded test -- the code simply compares the first significant token to the right of the assignment operator with the configured word. It has already been suggested that Scope::Guard and maybe some of its friends ought to be pre-configured, but I have not done that yet because I'm validating versus warnings::unused and I'm lazy enough not to want to deliberately introduce differences in behavior. By default I was _not_ going to complain about unused subroutine arguments -- at least, if unpacked in a way the code understands, like my ( $self, $arg1, $unused ) = @_; nor when a reference to the initialization is taken, as in \( my $foo = 'baz' ) though in the latter case I have not actually been ambitious enough to try to find out if anything is done with the reference. So far, there have been more than enough edge cases to keep me busy tracking down false positives. Some of the false positives can not be detected by static analysis in any way I can think of: variables only used in a stringy eval, or by code inserted by a filter. Those will require an 'I meant to do that' comment (spelled '## no critic (ProhibitUnusedVariables)').
From gg [...] zip.com.au Fri Feb 11 18: 35:55 2011
MIME-Version: 1.0
X-Spam-Status: No, score=-6.899 tagged_above=-99.9 required=10 tests=[AWL=0.001, BAYES_00=-1.9, RCVD_IN_DNSWL_HI=-5] autolearn=ham
In-Reply-To: <rt-3.8.HEAD-19313-1297180834-1986.65465-6-0 [...] rt.cpan.org> (Tom Wyant via's message of "Tue, 8 Feb 2011 11:00:35 -0500")
X-Spam-Flag: NO
References: <RT-Ticket-65465 [...] rt.cpan.org> <87d3n79z7p.fsf [...] blah.blah> <rt-3.8.HEAD-9726-1296925879-1355.65465-6-0 [...] rt.cpan.org> <87ei7j9yku.fsf [...] blah.blah> <rt-3.8.HEAD-17550-1297123769-872.65465-6-0 [...] rt.cpan.org> <rt-3.8.HEAD-19313-1297180834-1986.65465-6-0 [...] rt.cpan.org>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
Message-ID: <878vxmi1pj.fsf [...] blah.blah>
Content-Type: text/plain; charset="utf-8"
X-RT-Original-Encoding: utf-8
X-Spam-Score: -6.899
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 531D861E01B for <cpan-bug+Perl-Critic [...] hipster.bestpractical.com>; Fri, 11 Feb 2011 18:35:55 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id H2O28y-FN-u2 for <cpan-bug+Perl-Critic [...] hipster.bestpractical.com>; Fri, 11 Feb 2011 18:35:52 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id 226C461E00C for <bug-Perl-Critic [...] rt.cpan.org>; Fri, 11 Feb 2011 18:35:51 -0500 (EST)
Received: (qmail 22583 invoked by uid 103); 11 Feb 2011 23:35:51 -0000
Received: from x16.dev (10.0.100.26) by x1.dev with QMQP; 11 Feb 2011 23:35:51 -0000
Received: from mailout1-5.pacific.net.au (HELO mailout1.pacific.net.au) (61.8.2.212) by 16.mx.develooper.com (qpsmtpd/0.80) with ESMTP; Fri, 11 Feb 2011 15:35:48 -0800
Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout1.pacific.net.au (Postfix) with ESMTP id 9FF7B5E7A1C for <bug-Perl-Critic [...] rt.cpan.org>; Sat, 12 Feb 2011 10:35:43 +1100 (EST)
Received: from blah.blah (unknown [203.26.175.157]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id DE2928C08 for <bug-Perl-Critic [...] rt.cpan.org>; Sat, 12 Feb 2011 10:35:42 +1100 (EST)
Received: from gg by blah.blah with local (Exim 4.72) (envelope-from <gg [...] zip.com.au>) id 1Po2Wm-00046q-Gr for bug-Perl-Critic [...] rt.cpan.org; Sat, 12 Feb 2011 10:35:52 +1100
Delivered-To: cpan-bug+Perl-Critic [...] hipster.bestpractical.com
Subject: Re: [rt.cpan.org #65465] ProhibitUnusedVariables notice readline <$fh>
User-Agent: Gnus/5.110011 (No Gnus v0.11) Emacs/23.2 (gnu/linux)
Return-Path: <gg [...] zip.com.au>
X-Spam-Check-BY: 16.mx.develooper.com
X-Original-To: cpan-bug+Perl-Critic [...] hipster.bestpractical.com
X-RT-Mail-Extension: perl-critic
Date: Sat, 12 Feb 2011 10:35:52 +1100
X-Spam-Level:
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
RT-Message-ID: <rt-3.8.HEAD-17550-1297467356-1940.65465-0-0 [...] rt.cpan.org>
Content-Length: 1656
Download (untitled) / with headers
text/plain 1.6k
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > my $VERSION = something-or-other;
Ah. I see for instance Finance::BeanCounter has that doing its own exported BeanCounterVersion() func, which wouldn't seem to add much to the usual ->VERSION etc. Show quoted text
> [ProhibitUnusedVariables] > allow_if_computed_by = Scope::Guard unpack stat
There's quite a few of those destructor-action things. My crib list of the generic ones (from my own Glib::Ex::FreezeNotify, which is a specific one), AtExit End ReleaseAction Sub::ScopeFinalizer Guard Show quoted text
> simply compares the > first significant token to the right of the assignment operator
I suppose it could do an "is this a class method call" to pick up the alternative syntax thingie "new Scope::Guard ...". Show quoted text
> It has already been suggested that Scope::Guard and > maybe some of its friends ought to be pre-configured,
If nothing else those like SelectSaver which come with perl would be strong candidates. Show quoted text
> By default I was _not_ going to complain about unused subroutine > arguments -- at least, if unpacked in a way the code understands, like > > my ( $self, $arg1, $unused ) = @_;
Sounds good, that's one of the most annoying things warnings:unused gives (which is just in it's nature of course). Show quoted text
> ## no critic (ProhibitUnusedVariables)
A bit ugly, but as long as it wasn't needed most of the time. It's tempting to think of naming block-scoped variables in some way suggesting their purpose to both human and computer readers, like "..._saver", "block_scope_...", or something. I suppose people might have different preferences for that though. my $foo_saver = guard { some_code(); };


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.