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)

History Show all quoted text
  Fri Feb 04 18:06:42 2011 user42 [...] zip.com.au - Ticket created
Subject: ProhibitUnusedVariables notice readline <$fh>
Date: Sat, 05 Feb 2011 10:06:34 +1100
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
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.
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$
  Sat Feb 05 12:11:19 2011 wyant [...] cpan.org - Correspondence added
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
  Sat Feb 05 12:11:19 2011 The RT System itself - Status changed from 'new' to 'open'
  Mon Feb 07 19:09:29 2011 user42 [...] zip.com.au - Correspondence added
Subject: Re: [rt.cpan.org #65465] ProhibitUnusedVariables notice readline <$fh>
Date: Tue, 08 Feb 2011 11:09:21 +1100
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
"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.
  Tue Feb 08 11:00:34 2011 wyant [...] cpan.org - Correspondence added
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)').
  Fri Feb 11 18:35:56 2011 user42 [...] zip.com.au - Correspondence added
Subject: Re: [rt.cpan.org #65465] ProhibitUnusedVariables notice readline <$fh>
Date: Sat, 12 Feb 2011 10:35:52 +1100
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
"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(); };