Some cases where state is still being detected as unused

[oalders]

Thanks again for all of your help. 😄 I've got the following in our .perlcriticrc:

[Variables::ProhibitUnusedVarsStricter]
severity = 3
allow_if_computed_by = [list of functions]
allow_state_in_expression = 1

However, there are still a few things that are failing:

use strict;
use warnings;
use feature qw( state );

sub something {
    state $dir = do {
        ...
    };
}

use strict;
use warnings;
use feature qw( state );

use Mojo::IOLoop ();

local *Mojo::IOLoop::singleton = sub ($) { state $l = Mojo::IOLoop->new };

Those are possibly the same issue? There's also this:

use strict;
use warnings;
use feature qw( state );

use Linux::Systemd::Daemon qw( sd_ready );
local $SIG{USR1} = sub ($) { sd_ready() unless state $ready++ };

None of the above are dealbreakers and it ends up being very few annotations over many lines of code, but I thought I'd raise them here in case they are of interest.

[trwyant]

Thanks for the report. I intended to allow the kind of thing the first two do, but it looks like the policy is too restrictive. I don't know that I considered the third case at all, but it seems a reasonable exception also.

I may not jump right on these, but please feel free to rattle my cage in a week or so.

[oalders]

I may not jump right on these, but please feel free to rattle my cage in a week or so.

Thanks for all of your help!

[trwyant]

After a little thought: The intent of 'allow_state_in_expression' was to cover the case where effect was to occur only once no matter how often control flow went through the 'state' declaration. A static analysis will never get this completely right, so I have to figure out what it is reasonable to do.

Your first example only makes sense to me if the ellipsis contains code with a side effect that is only to occur once. Is this what is going on?

Your second example is an implicit 'return' of the value of the state variable, which is only computed the first time. The code does not currently accommodate returned values of state variables, but it should, and I think it does for lexical variables. I will take a shot at this one.

I understand the crux of your third example to be that 'state $ready++' is false the first time, and true subsequent times. This generalizes to accepting both post-increment and post-decrement. Interestingly, pre-increment and pre-decrement are syntax errors, so I'm going to ignore them.

[trwyant]

Correction: as written your first two examples are implicit returns, though when I wrote the previous comment that fact escaped me for the first example.

[trwyant]

... and, in fact, the policy as written accepts both the first two under the following two conditions:

• the 'return' is made explicit, and
• allow_returned_lexicals is made true.

Now, on the one hand Subroutines::RequireFinalReturn is a core policy, and a Perl Best Practices policy. On the other hand I think policies should be orthogonal, and there is at least one case (making something inline-able) where an implicit return is not equivalent to an implicit return.

So what I'm thinking of doing in this case is looking into how hard it is to capture implicit returns (probably by figuring out how RequireFinalReturn does it), and figuring out how to tweak the docs to point to the correct configuration item for this case.

The post-increment (your third example) is implemented (I think), though not yet pushed. It turned out to be more complicated than I thought it would be because PPI did not parse the example as a PPI::Statement::Variable. This did not entail an actual new code path, but it did mean refactoring to reduce duplication of code between the two paths.

[oalders]

Your first example only makes sense to me if the ellipsis contains code with a side effect that is only to occur once. Is this what is going on?

Yes

Your second example is an implicit 'return' of the value of the state variable, which is only computed the first time. The code does not currently accommodate returned values of state variables, but it should, and I think it does for lexical variables. I will take a shot at this one.

I took a look at both of these cases and making the return explicit cleared up the policy violation and made the code's intent easier to understand. So I think maybe just making this clearer in the documentation is helpful. I'm happy with forcing the explicit return here.

The post-increment (your third example) is implemented (I think), though not yet pushed.

That was fast! 🚀

[trwyant]

I actually did a naïve implementation of implicit return, which called the statement an implicit return if it was the last statement in the subroutine. This turned out to have a fair amount of fallout in the tests, and it misses some obvious cases like the last statement of the if-block of an if() { ... } then { ... }. So all in all I would be happier trying to make the documentation more explicit. I will hope to have something in the next couple days.

[trwyant]

Version 0.113_01 just went to PAUSE. This has the post-increment and post-decrement code, and POD changes in lieu of trying to find implicitly-returned lexical variables.

[oalders]

Thanks! I will test drive that this week and follow up here.

[oalders]

It all looks great from what I can tell. Thanks so much for this, @trwyant!