trwyant / perl-Perl-Critic-Policy-Variables-ProhibitUnusedVarsStricter

Critique unused variables in Perl source.
5 stars 0 forks source link

Detecting vars which are used in subroutine signatures #2

Closed oalders closed 2 years ago

oalders commented 2 years ago

First off, thanks so much for this wonderful policy. I'm using it at $work and it found some cases which Test::Vars seems to have missed. Very helpful.

I've had to add some no critic annotations for the following kinds of cases:

use strict;
use warnings;

use feature qw(signatures);
no warnings qw(experimental::signatures);

use Const::Fast qw( const );

const my $THING => 99;

sub something ( $arg = $THING ) {
    print "got $arg\n";
}

something();

The warning is:

$THING is declared but not used at line 9, near 'const my $THING => 99;

I did some questionable stuff in perlimports to detect a similar case:

https://metacpan.org/release/OALDERS/App-perlimports-0.000034/source/lib/App/perlimports/Include.pm#L281-309

Not sure if something similar would be possible here, but I figured I'd float the possibility.

trwyant commented 2 years ago

Thank you very much.

I anticipate this to be fixable, but the fix may be ugly since PPI thinks the signature is a prototype. Of course, it will be far from the first ugly code in this module.

What I will probably do is look at your code, steal what I can, and wedge it in where it will fit. This policy already has a bunch of munging fragments into their own little PPI::Document objects and rummaging through them. One more won't hurt, and it looks like you have found some of the edge cases for me.

trwyant commented 2 years ago

Version 0.112_01 just went to PAUSE. I ran both it and 0.112 against my local Mini-CPAN and diff-ed the logs. I found three false positives from 0.112 that 0.112_01 fixed. I found no other test differences, but most differences were cruft generated by bad distributions of some sort so I may have missed a couple.

Please give this a try and let me know what you think. The work is done by method _get_subroutine_signature_uses(). I added your example to the test suite and it did pass, but maybe you have other examples?

The policy does not (yet) detect unused signature variables.

oalders commented 2 years ago

Thanks so much for those changes. I was able to remove a fair number of ## no critic annotations. I think that covers our use case as far as signatures go. I'll open a new issue to document some other things I'm seeing. Not sure if they need to be dealt with or not.