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

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

Feature request: allow symbols to be unused if they match a pattern like $foo_unused #1

Closed bmartin closed 4 years ago

bmartin commented 4 years ago

I find this policy very valuable, but it can be pretty cumbersome in cases of functions that return two arguments like so:

my ($aa, $bb) = MyFunc();

This can occur very common in code and writing a supression each time is awkward

my ($aa, $bb) = MyFunc(); ## no critic qw(Unused)

Also, if I know that $aa is expected to be unused, I'd still like it to tell me that $bb is unused.

I believe this is nicer because it is very explicit:

my ($aa_unused, $bb) = MyFunc();

I have a sample implementation below. Ideally, it would be symmetric so using $aa_unused would become invalid.

Possible Solution

To achieve that in my personal fork, I added this to my .perlcriticrc:

# Allow unused symbols matching regex
allow_unused_symbol_regex = unused

and made these changes to Perl/Critic/Policy/Variables/ProhibitUnusedVarsStricter.pm

        {   name        => 'allow_unused_symbol_regex',
            description => 'Allow unused symbols matching regex',
            behavior    => 'string list',
        },
my $regexpCacheAPtr;
sub _is_allowed_symbol {
    my ( $self, $symbol ) = @_;

    if (!defined $regexpCacheAPtr) {
        $regexpCacheAPtr = [ map { qr{$_} } keys %{ $self->{_allow_unused_symbol_regex} } ];
    }
    for my $re (@{$regexpCacheAPtr}) {
        return 1 if $symbol =~ $re;
    }

    return;
}
sub _get_violations {
    my ( $self, $declared ) = @_;

    my @in_violation;

    foreach my $name ( values %{ $declared } ) {
        foreach my $declaration ( @{ $name } ) {
            $declaration->{is_global}
                and next;
            $declaration->{used}
                and next;
            $declaration->{is_allowed_computation}
                and next;
            $self->_is_allowed_symbol($declaration->{element}->symbol())
                and next;
            ...
trwyant commented 4 years ago

The motivation for this policy is to restrict or eliminate unused variables. In the case of a subroutine some of whose return values are unused, I believe it would be simpler and clearer to code it thus:

my ( undef, $bb ) = MyFunc();

No need to annotate on your part, no need to maintain extra code on mine.

bmartin commented 4 years ago

Yes, that works. Perhaps it makes sense to convert this to a doc request to add an example of using undef?

trwyant commented 4 years ago

Will do. I believe it's the usual Perl idiom for things like this, but it's far from obvious.

Normally what I do in this case is figure out a doc change, run it by the requestor, then put out a development release. But I don't have a lot of tuits at the moment. Would you be so good as to rattle my cage in a couple weeks if you don't see or hear anything from me?

trwyant commented 4 years ago

Just pushed a commit. What I did was to add a POD section AVOIDING UNUSED VARIABLES. This has one subsection, "List Assignments." You're invited to read and comment. Please read critically, as I have great faith in my own ability to communicate unclearly.

bmartin commented 4 years ago

Thank you! That's exactly what I was hoping for.

trwyant commented 4 years ago

Thanks for the feedback. Development version 0.107_01 just went to PAUSE, and ultimately to a CPAN mirror near you. I normally give things a week for the CPAN testers to chew on them, and then make a production release.