search

houseabsolute / p5-Test-Vars

Detects unused variables in unit tests
https://metacpan.org/pod/Test::Vars
Other
6 stars 10 forks source link

Updates may be required for new OPs in Perl v5.38 #47

Closed richardleach closed 1 month ago

richardleach commented 2 years ago

New optimized OPs have been introduced in the 5.37 development cycle and might need to be recognised in Test::Vars. Specifically:

For example, ops like these might need to be added to the @padops and/or @op_svusers arrays.

(I'm not sufficiently clear on the intended classifications to be able to work up a pull request here.)

The fact that Test::Vars does not yet take PADSV_STORE into consideration has caused some breakage in App-perlvars' test suite, as discussed in part of https://github.com/Perl/perl5/commit/aafefcb90183e1d6ef62d9e1ccc1fae7fcdf9c8e. For example, for this sub:

sub bar {
    my $unused;
    my $one;
    my $two;
    my $three;
    my $four = 4;
}

Test::Vars with bleadperl reports that there are 5 unused variables, but previous versions of perl report only 4 unused. (I'm not sure if assignment alone is supposed to count as usage or not?)

However, Test::Vars' own test suite does not show any failures with the new OPs in place.

autarch commented 2 years ago

Thanks for the report, @richardleach. I'm not doing much with this code nowadays, but I'm happy to review a PR and make a new release.

richardleach commented 2 years ago

Thanks, @autarch. I can get App::Perlvars' tests to pass with this trivial change:

diff --git a/lib/Test/Vars.pm b/lib/Test/Vars.pm
index 725100a..c010cd7 100644
--- a/lib/Test/Vars.pm
+++ b/lib/Test/Vars.pm
@@ -269,6 +269,18 @@ BEGIN{
     my $aelemfast = B::opnumber('aelemfast_lex');
     $padops[$aelemfast == -1 ? B::opnumber('aelemfast') : $aelemfast]++;

+    # The 5.37 development cycle introduced some new ops.
+    my $padsv_store = B::opnumber('padsv_store');
+    if ($padsv_store != -1) {
+        $padops[$padsv_store]++;
+        $op_svusers[$padsv_store]++;
+    }
+    my $aelemfastlex_store = B::opnumber('aelemfastlex_store');
+    if ($aelemfastlex_store != -1) {
+        $padops[$aelemfastlex_store]++;
+        $op_svusers[$aelemfastlex_store]++;
+    }
+
     $op_anoncode = B::opnumber('anoncode');
     $padops[$op_anoncode]++;

But I'm unsure of: (1) The actual correctness of just adding the new ops to $padops and $op_svusers. (2) How Test::Vars handles existing OPs with the TARGMY optimization? (e.g. pp_add)

autarch commented 2 years ago

Unfortunately, I'm also not clear on whether this is a good fix. @gfx - what do you think?

demerphq commented 1 year ago

FWIW this bit me today. @richardleach did a bunch of the core work on this, if he is confident that this PR fixes it I'd go with it until I had evidence of the contrary.

jkeenan commented 1 year ago

Unfortunately, I'm also not clear on whether this is a good fix. @gfx - what do you think?

This module is still failing its tests against blead, and a production release of perl is coming soon. Would it be possible to move forward with this?

Thank you very much. Jim Keenan

jkeenan commented 7 months ago

Thanks, @autarch. I can get App::Perlvars' tests to pass with this trivial change:

diff --git a/lib/Test/Vars.pm b/lib/Test/Vars.pm
index 725100a..c010cd7 100644
--- a/lib/Test/Vars.pm
+++ b/lib/Test/Vars.pm
@@ -269,6 +269,18 @@ BEGIN{
     my $aelemfast = B::opnumber('aelemfast_lex');
     $padops[$aelemfast == -1 ? B::opnumber('aelemfast') : $aelemfast]++;

+    # The 5.37 development cycle introduced some new ops.
+    my $padsv_store = B::opnumber('padsv_store');
+    if ($padsv_store != -1) {
+        $padops[$padsv_store]++;
+        $op_svusers[$padsv_store]++;
+    }
+    my $aelemfastlex_store = B::opnumber('aelemfastlex_store');
+    if ($aelemfastlex_store != -1) {
+        $padops[$aelemfastlex_store]++;
+        $op_svusers[$aelemfastlex_store]++;
+    }
+
     $op_anoncode = B::opnumber('anoncode');
     $padops[$op_anoncode]++;

But I'm unsure of: (1) The actual correctness of just adding the new ops to $padops and $op_svusers. (2) How Test::Vars handles existing OPs with the TARGMY optimization? (e.g. pp_add)

@richardleach, could you make an actual pull request from this patch? That might enable others to play around with this more. Thanks.

richardleach commented 7 months ago

@richardleach, could you make an actual pull request from this patch? That might enable others to play around with this more. Thanks.

I can do this in about a week or so, currently away from my dev VM.

hakonhagland commented 6 months ago

I can get App::Perlvars' tests to pass with this trivial change: ....

@richardleach When I test the patch with perl 5.38.1, I get the following test failures:

$ perl --version | head -2
This is perl 5, version 38, subversion 1 (v5.38.1) built for x86_64-linux
$ git diff
diff --git a/lib/Test/Vars.pm b/lib/Test/Vars.pm
index 725100a..5696e91 100644
--- a/lib/Test/Vars.pm
+++ b/lib/Test/Vars.pm
@@ -269,6 +269,18 @@ BEGIN{
     my $aelemfast = B::opnumber('aelemfast_lex');
     $padops[$aelemfast == -1 ? B::opnumber('aelemfast') : $aelemfast]++;

+   # The 5.37 development cycle introduced some new ops.
+    my $padsv_store = B::opnumber('padsv_store');
+    if ($padsv_store != -1) {
+        $padops[$padsv_store]++;
+        $op_svusers[$padsv_store]++;
+    }
+    my $aelemfastlex_store = B::opnumber('aelemfastlex_store');
+    if ($aelemfastlex_store != -1) {
+        $padops[$aelemfastlex_store]++;
+        $op_svusers[$aelemfastlex_store]++;
+    }
+
     $op_anoncode = B::opnumber('anoncode');
     $padops[$op_anoncode]++;

$ ./Build test
t/00_load.t .............. 1/1 # Testing Test::Vars/0.015
t/00_load.t .............. ok   
t/01_all_vars_ok_self.t .. skipped: No MANIFEST ready
t/02_no_warnings.t ....... 1/? # Test::Vars ignores CompileError.pm because: Intentional compile error.
# Test::Vars ignores ImplicitTopic.pm because: Can't use global $_ in "my" at t/lib/ImplicitTopic.pm line 6, near "my $_ "
t/02_no_warnings.t ....... ok    
t/03_warned.t ............ ok    
t/04_ignores.t ........... ok   
t/05_test_vars.t ......... ok   
t/06_vars_ok_self.t ...... ok   
t/07_stub_sub_bug.t ...... ok   
t/08_undef_aux_list.t .... ok   
t/09_array_slice.t ....... 1/? 
#   Failed test 'got expected output from test_vars - no unused vars'
#   at t/09_array_slice.t line 22.
#     Structures begin differing at:
#          $got->[0][1] = '256'
#     $expected->[0][1] = '0'
# Looks like you failed 1 test of 2.
t/09_array_slice.t ....... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 
t/10_my_sub.t ............ ok   

Test Summary Report
-------------------
t/09_array_slice.t     (Wstat: 256 (exited 1) Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=11, Tests=70,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.55 cusr  0.11 csys =  0.70 CPU)
Result: FAIL

Any ideas?

richardleach commented 6 months ago

Presumably related to the fact that with the patch, test_vars() states that

@foo is used once in &ArraySlice::foo at t/lib/ArraySlice.pm line 5

in t/lib/ArraySlice.pm:

package ArraySlice;

sub foo {
    my @foo = qw( foo bar );
    my $last = $foo[-1];
    return $last;
}

1;

I'll try stepping through Test::Vars' _check_into_code subroutine in the coming days to hopefully understand how it works - and if that points to a solution.

richardleach commented 6 months ago

It migtht be the logic around this bit of _make_scan_subs:

        elsif($op->private & _OPpLVAL_INTRO){
            # my($var) = @_;
            #    ^^^^     padsv/non-void context
            #          ^  sassign/void context
            #
            # We gather all of the sibling ops that are not NULL. If all of
            # them are SV-using OPs (see the BEGIN block earlier) _and_ all of
            # them are in VOID context, then the variable from the first op is
            # being used once.
            my @ops;
            for(my $o = $op->next; ${$o} && ref($o) ne 'B::COP'; $o = $o->next){
                push @ops, $o
                    unless $o->type == $op_null;
            }

            if (all {$op_svusers[$_->type] && ($_->flags & B::OPf_WANT) == B::OPf_WANT_VOID} @ops){
                if(_ckwarn_once($cop)){
                    $p->{context} = sprintf 'at %s line %d',
                        $cop->file, $cop->line;
                    return; # unused, but ok
                }
            }
        }

I remember when writing the original patch, it was unclear to me whether the new OPs should be on both the @padops and @op_svusers arrays, or just the latter. Any guidance on this would be helpful!

richardleach commented 6 months ago

Within _make_scan_subs, changing

if (all {$op_svusers[$_->type] && ($_->flags & B::OPf_WANT) == B::OPf_WANT_VOID} @ops){

to 

if (all {$op_svusers[$_->type] && (($_->flags & B::OPf_WANT) == B::OPf_WANT_VOID)
    && ($_->type != $padsv_store) && ($_->type != $aelemfastlex_store) } @ops){

fixes _09_arrayslice.t but freshly breaks _07_stub_subbug.t.

So I'm assuming that __make_scansubs is the correct place to be making changes, but need to step through the logic more.

richardleach commented 6 months ago

@hakonhagland - please could you test https://github.com/houseabsolute/p5-Test-Vars/pull/52 ?

hakonhagland commented 6 months ago

@richardleach Thanks for the PR, i will check now.

richardleach commented 5 months ago

@autarch / @gfx - any thoughts on https://github.com/houseabsolute/p5-Test-Vars/pull/52 ? Is it something you can merge or would you like to see any changes?

jkeenan commented 2 months ago

@autarch / @gfx - any thoughts on #52 ? Is it something you can merge or would you like to see any changes?

https://github.com/houseabsolute/p5-Test-Vars/pull/52 was merged into master today. We are awaiting the resolution of some PAUSE permission problems, at which point a new CPAN release will be issued. That should permit us to close this ticket. Self-assigning in the interim.

jkeenan commented 1 month ago

@autarch / @gfx - any thoughts on #52 ? Is it something you can merge or would you like to see any changes?

52 was merged into master today. We are awaiting the resolution of some PAUSE permission problems, at which point a new CPAN release will be issued. That should permit us to close this ticket. Self-assigning in the interim.

I hope to release Test-Vars-0.016 by September 19. If you'd like, you can test out this tarball which contains most of the changes that will go into 0.016.

jkeenan commented 1 month ago

@richardleach @gfx @hakonhagland, please try Test-Vars-0.017, released today to CPAN.