t/comp/: implement warnings-by-default

[jkeenan]

In this ticket we will track our progress on implementing Objective 3, warnings-by-default, for tests in the t/comp/ directory in the core distribution. See that ticket for guidance on addressing warnings.

On Oct 11 I ran make test on the alpha-dev-03-warnings branch at commit 78482fccd5. I then chopped up the output into individual files, gzipped them, and placed them for your reference in this directory on my server.

By my count, the line count in the output file was 120, which compares with a line count of 25 for the same group of files when I run make test on the alpha branch at commit c5c5fb4fb2. So one, somewhat crude way of measuring our progress is how far we manage to reduce the line count in these test runs.

Acceptance Criteria: All tests for this subdirectory PASS; no tests emit warnings.

[brainbuz]

These tests are unable to find the warnings module.

In other test files I see code like:

BEGIN {
    chdir 't' if -d 't';
    @INC = '../lib';
} 

there is some variation on this block, I think all that is really needed is @INC.

I'm going to proceed under the assumption that I can paste this where needed.

The following tests in the folder need warnings silenced: t/comp/bproto.t t/comp/form_scope.t t/comp/opsubs.t t/comp/parser.t t/comp/proto.t t/comp/require.t t/comp/uproto.t

[jkeenan]

These tests are unable to find the warnings module.

In other test files I see code like:

BEGIN {
    chdir 't' if -d 't';
    @INC = '../lib';
} 

there is some variation on this block, I think all that is really needed is @INC.

I'm going to proceed under the assumption that I can paste this where needed.

The following tests in the folder need warnings silenced: t/comp/bproto.t t/comp/form_scope.t t/comp/opsubs.t t/comp/parser.t t/comp/proto.t t/comp/require.t t/comp/uproto.t

Hang on a bit. I've got a branch in which I've been working on these tests. Let me identify it and check its status before you do work that's already been done.

jimk

[jkeenan]

Hang on a bit. I've got a branch in which I've been working on these tests. Let me identify it and check its status before you do work that's already been done.

jimk

Here's the branch, which I've just rebased on alpha-dev-03-warnings:

gha-307-t-comp-warnings

In a minute, I'll post the current test output in that branch.

[jkeenan]

Here's the output for the t/comp/*.t tests in that branch:

comp/bproto.t ............ ok
comp/cmdopt.t ............ ok
comp/colon.t ............. ok
comp/decl.t .............. ok
comp/filter_exception.t .. ok
comp/final_line_num.t .... ok
comp/fold.t .............. ok
Variable "$x" is not available at comp/form_scope.t line 56.
Use of uninitialized value $x in concatenation (.) or string at comp/form_scope.t line 50.
Variable "$x" is not available at (eval 1) line 1.
comp/form_scope.t ........ ok
comp/hints.t ............. ok
comp/line_debug.t ........ ok
comp/multiline.t ......... ok
comp/opsubs.t ............ ok
comp/our.t ............... ok
comp/package.t ........... ok
comp/package_block.t ..... ok
Useless use of string in void context at comp/parser.t line 427.
Useless use of array element in void context at comp/parser.t line 545.
Useless use of anonymous hash ({}) in void context at comp/parser.t line 126.
Useless use of array element in void context at comp/parser.t line 547.
Useless use of time in void context at parser.t line 42.
Useless use of string in void context at parser.t line 44.
Useless use of string in void context at parser.t line 24.
Useless use of concatenation (.) or string in void context at parser.t line 641.
Use of uninitialized value $_ in substitution (s///) at (eval 35) line 1.
"my" variable $r masks earlier declaration in same statement at (eval 37) line 2.
"my" variable $r masks earlier declaration in same statement at (eval 39) line 2.
"my" variable $r masks earlier declaration in same statement at (eval 40) line 3.
Applying substitution (s///) to @a will act on scalar(@a) at (eval 40) line 4.
Subroutine foo2 redefined at (eval 72) line 1.
Subroutine foo2 redefined at (eval 73) line 1.
Subroutine foo2 redefined at (eval 74) line 1.
Subroutine foo2 redefined at (eval 75) line 1.
Subroutine foo2 redefined at (eval 76) line 1.
Subroutine foo2 redefined at (eval 77) line 1.
Subroutine foo2 redefined at (eval 78) line 1.
Subroutine foo2 redefined at (eval 79) line 1.
Subroutine foo2 redefined at (eval 80) line 1.
Subroutine foo3 redefined at (eval 82) line 1.
Subroutine foo3 redefined at (eval 83) line 1.
Subroutine foo3 redefined at (eval 84) line 1.
Subroutine foo3 redefined at (eval 85) line 1.
Subroutine foo3 redefined at (eval 86) line 1.
Subroutine foo3 redefined at (eval 87) line 1.
Subroutine foo3 redefined at (eval 88) line 1.
Subroutine foo3 redefined at (eval 89) line 1.
Subroutine foo3 redefined at (eval 90) line 1.
Useless use of single ref constructor in void context at (eval 101) line 1.
Useless use of single ref constructor in void context at (eval 102) line 1.
Useless use of single ref constructor in void context at (eval 103) line 1.
Useless use of single ref constructor in void context at (eval 104) line 1.
Useless use of single ref constructor in void context at (eval 105) line 1.
Useless use of single ref constructor in void context at (eval 106) line 1.
Useless use of single ref constructor in void context at (eval 107) line 1.
Useless use of single ref constructor in void context at (eval 108) line 1.
Useless use of single ref constructor in void context at (eval 109) line 1.
Useless use of single ref constructor in void context at (eval 110) line 1.
Applying substitution (s///) to @array will act on scalar(@array) at (eval 127) line 1.
Prototype after '%' for main::rima : $%&*$&*\\$%\\*&$%*& at (eval 127) line 6.
Use of uninitialized value in scalar dereference at (eval 128) line 2.
Use of uninitialized value in string at (eval 128) line 2.
Use of uninitialized value in scalar dereference at comp/parser.t line 427.
Use of uninitialized value in string at comp/parser.t line 427.
Use of uninitialized value $a in numeric gt (>) at maggapom line 1.
Argument "" isn't numeric in array or hash lookup at comp/parser.t line 545.
Use of uninitialized value $1[0] in array element at comp/parser.t line 547.
Use of uninitialized value in concatenation (.) or string at parser.t line 536.
Use of uninitialized value in concatenation (.) or string at parser.t line 536.
Use of uninitialized value in concatenation (.) or string at (eval 165)[parser.t:543] line 545.
Use of uninitialized value in concatenation (.) or string at (eval 165)[parser.t:543] line 545.
Use of uninitialized value in string at (eval 165)[parser.t:543] line 545.
Use of uninitialized value in concatenation (.) or string at parser.t line 24.
Use of uninitialized value in concatenation (.) or string at parser.t line 641.
comp/parser.t ............ ok
comp/parser_run.t ........ ok
Useless use of subroutine prototype in void context at comp/proto.t line 455.
Useless use of subroutine prototype in void context at comp/proto.t line 460.
Use of uninitialized value within @_ in join or string at comp/proto.t line 276.
main::l564() called too early to check prototype at (eval 22) line 1.
comp/proto.t ............. ok
comp/redef.t ............. ok
comp/require.t ........... ok
comp/retainedlines.t ..... ok
comp/term.t .............. ok
Useless use of a constant (22) in void context at comp/uproto.t line 104.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value in join or string at comp/uproto.t line 51.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 44.
Illegal character after '_' in prototype for main::wrong1 : _$ at (eval 3) line 1.
Illegal character after '_' in prototype for main::wrong2 : $__ at (eval 4) line 1.
Use of uninitialized value $name in concatenation (.) or string at comp/uproto.t line 38.
comp/uproto.t ............ ok
comp/use.t ............... ok
comp/utf.t ............... ok
All tests successful.
Files=25, Tests=5196,  3 wallclock secs ( 0.27 usr  0.04 sys +  0.36 cusr  0.34 csys =  1.00 CPU)
Result: PASS

You can find a tarball of those results here.

Thank you very much. Jim Keenan

[brainbuz]

The following still issue warnings: t/comp/form_scope.t t/comp/parser.t t/comp/proto.t t/comp/uproto.t do you want me to look at them? If your work is overlapping some of the unassigned cases do you want to pick some that it is safe for me to work on?

[jkeenan]

On 10/29/20 5:09 PM, John Karr wrote:

The following still issue warnings: t/comp/form_scope.t t/comp/parser.t t/comp/proto.t t/comp/uproto.t do you want me to look at them? If your work is overlapping some of the unassigned cases do you want to pick some that it is safe for me to work on?

You're welcome to have a go at them. The reason why they are still throwing warnings (and why I did not as of yet make a pull request) is because they're hard.

If you think it would be administratively simpler, I could make a pull request for this branch (merging into alpha-dev-03-strict) now which would merge those files already fixed, leaving the 4 files listed above for you (or anyone else) to work on. Let me know what would work best for you.

Thank you very much.

jimk

[brainbuz]

Please Merge your branch I'll take a look at the files, also please pick some other cases and assign them.

[jkeenan]

Please Merge your branch I'll take a look at the files, also please pick some other cases and assign them.

Okay, I just merged the gha-307-t-comp-warnings branch into alpha-dev-03-warnings branch. 4 or file files still generate warnings. Have at them! Since I know they're hard, feel free to work submit per-file pull requests.

Thank you very much. Jim Keenan

[brainbuz]

This my first time actively working on a big project with many collaborators and branches. I closed the duplicate PRs,

The only file left with warnings should be parser.t which is challenging me quite a bit, it has lots of heredocs (one of which editors confuse with vcs merge conflict tags), lots of warnings, lots of warnings on evals that tell the line of the eval not the line eval was called at. Some warnings are originating at places other than where what the warning says.

[brainbuz]

I found a block in parser.t which does nothing (around line 555), commenting it does not change the number of tests or cause any failures.

    # More potential multideref assertion failures
    # OPf_PARENS on OP_RV2SV in subscript
    $x[($_)];
    # OPf_SPECIAL on OP_GV in subscript
    $x[FILE1->[0]];```

[jkeenan]

This my first time actively working on a big project with many collaborators and branches.

Yes, this can be challenging -- but you're doing fine!

I closed the duplicate PRs,

Thanks.

The only file left with warnings should be parser.t which is challenging me quite a bit, it has lots of heredocs (one of which editors confuse with vcs merge conflict tags), lots of warnings, lots of warnings on evals that tell the line of the eval not the line eval was called at. Some warnings are originating at places other than where what the warning says.

Yeah, it stumped me, too. I'll try to take another look at it tomorrow and see how we can collaborate.

Thank you very much. Jim Keenan

[brainbuz]

There is a comment a bit earlier that successful parsing is sufficient.

[jkeenan]

I found a block in parser.t which does nothing (around line 555), commenting it does not change the number of tests or cause any failures.

    # More potential multideref assertion failures
    # OPf_PARENS on OP_RV2SV in subscript
    $x[($_)];
    # OPf_SPECIAL on OP_GV in subscript
    $x[FILE1->[0]];```

One technique that can prove helpful in understanding the core distribution is "git archeology" -- essentially, repeated use of git blame to try to determine the original contributor's content.

In this case, the fact that these lines are in a block which starts with no strictvars; is a hint that this is a "Perl 7" addition to make the code work under strict-by-default. git blame confirms this:

4245fea43c8 (Father Chrysostomos   2014-12-13 13:32:09 -0800 540) 
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 541) {
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 542)     no strict 'vars';
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 543)     # More potential multideref assertion fail
ures
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 544)     # OPf_PARENS on OP_RV2SV in subscript
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 545)     $x[($_)];
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 546)     # OPf_SPECIAL on OP_GV in subscript
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 547)     $x[FILE1->[0]];
1cd1b0b84df (James E Keenan        2020-08-06 19:24:09 -0400 548) }
b39c1059f09 (Father Chrysostomos   2014-12-20 04:33:47 -0800 549) 

But that's just us being pedantic. What's really interesting is to do git blame on this file in the Perl 5 distribution, i.e., before we got our hands on the codebase. Do git blame there, then do git show on the SHAs listed from lines 515 to 550 there. You'll get a glimpse into the strange world of Father Chrysostomos.

jimk

[brainbuz]

I've created my suggestion as a patch against perl 5 bleed if you can look at it before I make it a pull request.

0001-make-pass-type-tests-for-two-lines-that-could-break-.patch.zip

[jkeenan]

I've created my suggestion as a patch against perl 5 bleed if you can look at it before I make it a pull request.

0001-make-pass-type-tests-for-two-lines-that-could-break-.patch.zip

My feelings on this are two-fold:

1. I don't think these changes are necessary. If you read Father C's inline comment, demonstrating that the code parses without the file dying is sufficient for what the code in t/comp/parser.t is intended to do.
2. However, if you're feeling bold you can submit this as a p.r. to https://github.com/Perl/perl5/issues.

@atoomic, do you have any strong opinions on this?

Thank you very much. Jim Keenan

[brainbuz]

Something is tested (the lines aren't fatal), and should be counted as a test, moving the braces clarifies what code is part of the block testing those two lines.

On 11/7/20 8:21 AM, James E Keenan wrote:

I've created my suggestion as a patch against perl 5 bleed if you
can look at it before I make it a pull request.

0001-make-pass-type-tests-for-two-lines-that-could-break-.patch.zip
<https://github.com/atoomic/perl/files/5499828/0001-make-pass-type-tests-for-two-lines-that-could-break-.patch.zip>

My feelings on this are two-fold:

1. I don't think these changes are necessary. If you read Father C's inline comment, demonstrating that the code parses without the file dying is sufficient for what the code in |t/comp/parser.t| is intended to do.
2. However, if you're feeling bold you can submit this as a p.r. to https://github.com/Perl/perl5/issues.

@atoomic https://github.com/atoomic, do you have any strong opinions on this?

Thank you very much. Jim Keenan

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/atoomic/perl/issues/307#issuecomment-723445599, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADOTPDNCHT4IPQUZRGD7QTSOVCV7ANCNFSM4SNFJXWQ.

[jkeenan]

With the merge for t/comp/parser.t, it looks like the last file in this directory which is emitting warnings is t/comp/uproto.t.

[jkeenan]

This ticket will be closable once the confusion over t/comp/parser.t is cleared up.

[jkeenan]

This ticket will be closable once the confusion over t/comp/parser.t is cleared up.

And that was accomplished with commit 25cbffb0f8daa42936ccf24ae094d5931b89e489 for p.r. https://github.com/atoomic/perl/pull/338. t/comp/*.t now runs warnings-free!

Thank you very much. Jim Keenan