search

atoomic / perl

a repo to show what could be p7
Other
18 stars 8 forks source link

Study changes to t/comp/*.t (1 of 2) #201

Closed jkeenan closed 4 years ago

jkeenan commented 4 years ago

In this ticket we will study our progress in pursuit of Objective 2, "strict-by-default", for the following files in the Perl core distribution:

t/comp/cmdopt.t
t/comp/decl.t
t/comp/final_line_num.t
t/comp/fold.t
t/comp/form_scope.t
t/comp/hints.t
t/comp/line_debug.t
t/comp/line_debug_0.aux
t/comp/multiline.t
t/comp/our.t

These files should be evaluated in light of the guidelines presented on this wiki page. Please post the summary of your findings in a single comment in this issue.

First-time contributors: Please post a comment to this ticket indicating you are working on it. That will enable us to assign the ticket to you. (GitHub won't let us do that until it already thinks you're part of the project.)

Thank you for helping to improve the Perl programming language.

oodler577 commented 4 years ago

happy to take this one, @jkeenan

oodler577 commented 4 years ago

@jkeenan - here you go:

t/comp/cmdopt.t

  • shebang is relative, "./perl"

  • doesn't "use strict" or "use warnings"

  • outputs TAP, but does so "by hand" (not using a Test:: module)

  • tests could benefit from Test::Simple

  • NOTE: $x is scoped with my, but $a is not

  • change for strictness seems ok, but scoping "$a" with "my" seems to have been missed

t/comp/decl.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • uses "format" to output TAP
  • there is a label "quux:" above the "five" subroutine, but it doesn't seem to be used inside of this script
  • change for strictness seems ok

t/comp/final_line_num.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • has a single package variable (our)
  • composed of 3 BEGIN block, looks to be set up strictly to print TAP for a single test
  • change for strictness seems okay,

t/comp/fold.t

  • shebang is relative, "./perl -w"

  • doesn't "use strict", invokes warnings by virtue of "-w" in shebang

  • prints out TAP buy hand

  • implements its own "ok", "is", and "like" subroutines

  • looks at $@, but $@ is not localized

  • inside of a lexical block, defines handlers for WARN and DIE to accumulate counts when encountered; though signal handlers are not protected using "local"

  • defines a package "other" lexically to 'hide the "ok" sub' from tests contained therein

  • lexical package firsts unsets warnings ($^W) and sets it in two distinct BEGIN blocks (I am not sure when these get executed wrt to the main script's BEGIN phase)

  • contains two subsequent bare blocks that set "no strict 'subs'", but this script doesn't set strict or warnings itself

  • bare block with "BEGIN { $^W = 0; $::{u} = \undef }" - not sure what "$::{u}" is

  • redefinition of internal variables not protected with "local" scoping

  • all user defined variables seem to be scoped with 'my'

  • NOTE: "no strict 'subs'" has been introduced - not clear why, or if it's necessary - minor recommendation to try to solve so this is not necessary (to be 100% strict compliant)

  • change for strictness seems ok

t/comp/form_scope.t

  • shebang is relative, "./perl"

  • doesn't "use strict" or "use warnings"

  • prints out TAP buy hand

  • contains a regression test for bug #22977, declares then defines a subroutine with a parameter proto

  • regression test for bug 22977 and 50528 for a sub defined in a parent sub; has a series of repeating "my" declarations for 22 variables ($a-$u) over 18 lexical blocks - comment there says it's to "fill the pad with alphabet soup to give the closed-over variable a high padoff set (more likely to trigger the bug and crash)"

  • uses "format" to output test to TAP

  • redefining of internal variables is protected within block scope and "local" keyword; as are signal handlers

  • contains some sub generators, return generated subroutine references that seem to be regression tests for sub refs being returned from calling generator subs that also contain "format" commands - format are used to report test success as TAP

  • nothing immediate seems like it'd be impacted by "use strict", but this is the most complicated code I've seen yet so I can't so 100%

  • change for strictness seems ok

t/comp/hints.t

  • shebang is relative, "./perl"

  • doesn't "use strict" or "use warnings"

  • prints out TAP buy hand

  • take that back this is now the most complicated test :)

  • Comment at top: "Tests the scoping of $^H and %^H" - which are the the current state of syntax checks (had to look that up again);

  • there seems to be little opportunity to adding "use strict" to break this test since it's flexing $^H/%^H (in perldoc perlvar, it states that this is strictly for internal use and meant to store hints during perl complation phases wrt scoping) - this variable itself is used heavily to facilitate the "strict" pragma

  • interesting use of testing with string eval

  • change for strictness seems ok

t/comp/line_debug.t

  • shebang is relative, "./perl"

  • doesn't "use strict" or "use warnings"

  • prints out TAP buy hand

  • implements its own "is" sub

  • looks at $^O (operating system; aka, $OSNAME)

  • NOTE: "no strict 'refs" is used, but very precisely scoped - wondering if this can be done in a different way (just to say it's 100% strict safe)

  • changes for strictness seem ok

t/comp/line_debug_0.aux

  • looks like a snippet?

  • snippet itself is strict compliant

  • $z is package scoped with "our"

  • uses heredoc and format

  • change for strictness seems ok

t/comp/multiline.t

  • shebang is relative, "./perl"

  • doesn't "use strict" or "use warnings"

  • prints out TAP buy hand

  • implements its own "is", "like", etc subs

  • leverages END block to unlink temporary files (presumably in the case of a test failure

  • changes for strictness seem ok

t/comp/our.t

  • shebang is relative, "./perl"

  • doesn't "use strict" or "use warnings"

  • prints out TAP buy hand

  • implements its own "is"

  • defines a lexically scoped package (TieAll), defines functionality by leveraging AUTOLOAD

  • package used outside of definition scope

  • tests visibility of package variables named the same as higher scope lexical vars ($y) in a nest of blocks 2 deep (in addition to main scope)

  • NOTE: perldoc has "our $AUTOLOAD" declared inside of the "sub AUTOLOAD" not sure if that matters, but it's stated in the doc that this is done to, "keep 'use strict'" happy

  • changes for strictness seem ok (other than note above about $AUTOLOAD)

GENERAL NOTES:

  • using "git diff v5.32.0 -- path/to/file" worked well, but there were a number of cases where the diff revealed white space only changes - I think this was mentioned, so I just need to find the git diff flag that ignores these kinds of things
jkeenan commented 4 years ago

@jkeenan - here you go:

t/comp/cmdopt.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"

Our immediate objective is "strict-by-default". So, outside of the dist/ and cpan/ directories, we should not need use strict at the head of any file -- because strict is already in the air the program is breathing, so to speak.

Whether a program emits warnings or not only becomes in scope with Objective 3.

  • outputs TAP, but does so "by hand" (not using a Test:: module)
  • tests could benefit from Test::Simple

See perldoc perlhack. Some directories are such "baby Perl" that we don't assume that use MODULE; works yet. In some cases, we use manually coded tests. In other cases, we use t/test.pl which replicates much of the functionality of Test::Simple. In t/comp/*.t, we won't be using Test::Simple.

  • NOTE: $x is scoped with my, but $a is not
  • change for strictness seems ok, but scoping "$a" with "my" seems to have been missed

Good catch. This is something we've corrected in many places in the alpha codebase, e.g., t/comp/fold.t discussed below. In Perl 5, even with use strict in force, $a and $b can be used as global variables but really should not be because they are the comparator variables within the sort built-in function.

In other parts of the codebase, we have tried to change $a to $alpha and $b to $beta. We then scope $alpha and $beta with my or our as appropriate (usually, my).

Would you like to provide a pull request doing that for this file?

t/comp/decl.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • uses "format" to output TAP
  • there is a label "quux:" above the "five" subroutine, but it doesn't seem to be used inside of this script
  • change for strictness seems ok

t/comp/final_line_num.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • has a single package variable (our)
  • composed of 3 BEGIN block, looks to be set up strictly to print TAP for a single test
  • change for strictness seems okay,

t/comp/fold.t

  • shebang is relative, "./perl -w"
  • doesn't "use strict", invokes warnings by virtue of "-w" in shebang
  • prints out TAP buy hand
  • implements its own "ok", "is", and "like" subroutines
  • looks at $@, but $@ is not localized
  • inside of a lexical block, defines handlers for WARN and DIE to accumulate counts when encountered; though signal handlers are not protected using "local"

They appear localized to me: see lines 89-90, 186, 201

  • defines a package "other" lexically to 'hide the "ok" sub' from tests contained therein
  • lexical package firsts unsets warnings ($^W) and sets it in two distinct BEGIN blocks (I am not sure when these get executed wrt to the main script's BEGIN phase)
  • contains two subsequent bare blocks that set "no strict 'subs'", but this script doesn't set strict or warnings itself

In the first of those blocks, can you see whether, if you quote the two barewords and then comment out the no strict 'subs; statement, the file then compiles and run under p7? If so, then you can submit a p.r. "de-blocking" lines 139-146.

As for the second, I'll have to take another look. I think the bug ticket I filed for Perl 5 was rejected.

  • bare block with "BEGIN { $^W = 0; $::{u} = \undef }" - not sure what "$::{u}" is

It's $u in the main package, i.e., $main::u.

  • redefinition of internal variables not protected with "local" scoping
  • all user defined variables seem to be scoped with 'my'
  • NOTE: "no strict 'subs'" has been introduced - not clear why, or if it's necessary - minor recommendation to try to solve so this is not necessary (to be 100% strict compliant)
  • change for strictness seems ok

t/comp/form_scope.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • prints out TAP buy hand
  • contains a regression test for bug #22977, declares then defines a subroutine with a parameter proto
  • regression test for bug 22977 and 50528 for a sub defined in a parent sub; has a series of repeating "my" declarations for 22 variables ($a-$u) over 18 lexical blocks - comment there says it's to "fill the pad with alphabet soup to give the closed-over variable a high padoff set (more likely to trigger the bug and crash)"
  • uses "format" to output test to TAP
  • redefining of internal variables is protected within block scope and "local" keyword; as are signal handlers
  • contains some sub generators, return generated subroutine references that seem to be regression tests for sub refs being returned from calling generator subs that also contain "format" commands - format are used to report test success as TAP
  • nothing immediate seems like it'd be impacted by "use strict", but this is the most complicated code I've seen yet so I can't so 100%

Yes, this is very obscure for me, too. But if the tests pass and the diff is small, then we're good.

  • change for strictness seems ok

t/comp/hints.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • prints out TAP buy hand
  • take that back this is now the most complicated test :)
  • Comment at top: "Tests the scoping of $^H and %^H" - which are the the current state of syntax checks (had to look that up again);
  • there seems to be little opportunity to adding "use strict" to break this test since it's flexing $^H/%^H (in perldoc perlvar, it states that this is strictly for internal use and meant to store hints during perl complation phases wrt scoping) - this variable itself is used heavily to facilitate the "strict" pragma
  • interesting use of testing with string eval
  • change for strictness seems ok

t/comp/line_debug.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • prints out TAP buy hand
  • implements its own "is" sub
  • looks at $^O (operating system; aka, $OSNAME)
  • NOTE: "no strict 'refs" is used, but very precisely scoped - wondering if this can be done in a different way (just to say it's 100% strict safe)

It's the kind of statement that cannot be made with 'strict refs' in force. As an experiment, delete it, run the file with p7, and note the failure message.

  • changes for strictness seem ok

t/comp/line_debug_0.aux

  • looks like a snippet?
  • snippet itself is strict compliant

Yes. This is one of the most startling (?), annoying (?) things about strict-by-default. Any snippet of code you're going to 'eval', 'require' or 'do' (as on line 26) or any HEREDOC you write has to be strict-compliant as well.

  • $z is package scoped with "our"
  • uses heredoc and format
  • change for strictness seems ok

t/comp/multiline.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • prints out TAP buy hand
  • implements its own "is", "like", etc subs
  • leverages END block to unlink temporary files (presumably in the case of a test failure
  • changes for strictness seem ok

t/comp/our.t

  • shebang is relative, "./perl"
  • doesn't "use strict" or "use warnings"
  • prints out TAP buy hand
  • implements its own "is"
  • defines a lexically scoped package (TieAll), defines functionality by leveraging AUTOLOAD
  • package used outside of definition scope
  • tests visibility of package variables named the same as higher scope lexical vars ($y) in a nest of blocks 2 deep (in addition to main scope)
  • NOTE: perldoc has "our $AUTOLOAD" declared inside of the "sub AUTOLOAD" not sure if that matters, but it's stated in the doc that this is done to, "keep 'use strict'" happy

Which "perldoc" are you referring to here? In t/comp/our.t, we need $AUTOLOAD to be a package variable inside the TieAll package. So it's declared and scoped before we define sub AUTOLOAD.

  • changes for strictness seem ok (other than note above about $AUTOLOAD)

GENERAL NOTES:

  • using "git diff v5.32.0 -- path/to/file" worked well, but there were a number of cases where the diff revealed white space only changes - I think this was mentioned, so I just need to find the git diff flag that ignores these kinds of things

You probably want git diff -w v5.32.0..HEAD -- path/to/file. Do this often enough and you'll want to make that a shell routine.

Summary: Overall, a very thorough review. Going forward, we probably only need the NOTEs and the things that you have questions about.

If you'd like to make those pull requests, be sure to make them against the alpha-dev-02-strict branch.

Once we've closed this ticket, we'll offer you a menu of other tickets to work on.

Thank you very much. Jim Keenan

oodler577 commented 4 years ago

The following diff allows fold.t to not use no strict 'subs'; - looking at other thing you mentioned. Will create a PR when done.

diff --git a/t/comp/fold.t b/t/comp/fold.t
index 19feaee304..0adff87965 100644
--- a/t/comp/fold.t
+++ b/t/comp/fold.t
@@ -122,15 +122,14 @@ is ($@, '', 'no error');

 # [perl #78064] or print
 package other { # hide the "ok" sub
- no strict 'subs';
  BEGIN { $^W = 0 }
- print 0 ? not_ok : ok;
+ print 0 ? "not_ok" : "ok";
  print " ", ++$test, " - print followed by const ? BEAR : BEAR\n";
- print 1 ? ok : not_ok;
+ print 1 ? "ok" : "not_ok";
  print " ", ++$test, " - print followed by const ? BEAR : BEAR (again)\n";
- print 1 && ok;
+ print 1 && "ok";
  print " ", ++$test, " - print followed by const && BEAR\n";
- print 0 || ok;
+ print 0 || "ok";
  print " ", ++$test, " - print followed by const || URSINE\n";
  BEGIN { $^W = 1 }
 }
oodler577 commented 4 years ago

@jkeenan - updated t/comp/fold.t, was not able to get t/comp/line_debug.t to run clean (as you pointed out would be the case). PR #260 has been submitted.

jkeenan commented 4 years ago

@jkeenan - updated t/comp/fold.t, was not able to get t/comp/line_debug.t to run clean (as you pointed out would be the case). PR #260 has been submitted.

Thanks. This ticket looks sufficiently well studied, hence closable. I'll email you suggestions for next actions.

Thank you very much. Jim Keenan

jkeenan commented 4 years ago

I had to back out the code committed in the pull request yesterday. We need a username and email address that we can use in AUTHORS and keep t/porting/authors.t happy. @oodler577 please let us know when you have a username and email address associated with your github account.

Thank you very much. Jim Keenan

oodler577 commented 4 years ago

@jkeenan I applied for a PAUSE account, so I am waiting for the confirmation email. I am assuing the address will be oodler@cpan.org. PAUSE usernames are restricted from having numbers, which I found out when attempting to request a username that matched the my GitHub username.

jkeenan commented 4 years ago

All problems were resolved with this commit:

commit d0ef5ac9094c68801d4ececa7d43ce58c82acbb6
Author:     oodler <oodler@cpan.org>
AuthorDate: Tue Sep 1 17:04:09 2020 -0500
Commit:     James E Keenan <jkeenan@cpan.org>
CommitDate: Fri Sep 4 14:14:16 2020 -0400

Closing this ticket.

Thank you very much. Jim Keenan