t/opbasic/: 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/opbasic/ 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 5312, which compares with a line count of 5 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]

I've started to eyeball this group and it looks like there are a lot of tests that do things that should warn, like isn't numeric in numeric comparison . I can start going through these and turn off expected warnings.

One specific test I arbitrarily picked to examine assigns a *typeglob and then declaring (but not assigning to) a variable of the same name. The goal of the test appears to be that the variable should be valid but not inherit the value from the typeglob when no value has been assigned to it. For this test turning off strict and not declaring the variable should prevent one warning. Alternately both warnings could be suppressed. How would you proceed?

concat.t

*pi = \undef;
# This bug existed earlier than the $2 bug, but is fixed with the same
# patch. Without the fix this 5.7.0 would also croak:
# Modification of a read-only value attempted at ...
my $pi;
eval{"$pi\x{1234}"};
ok(!$@, "bug id 20001020.006 (#4484), constant left");

[jkeenan]

I've started to eyeball this group and it looks like there are a lot of tests that do things that should warn, like isn't numeric in numeric comparison .

Indeed! You'll find that many unit tests under t/ fall into one of two categories:

1. They are the sort of tests that were written simultaneously with the code they are exercising and so are intended to systematically exercise all intended features of the code. For example, tests that Larry wrote back in 1987 in t/base/*.t.
2. They are tests written in later years in response to bug tickets about weird edge cases. If you do git blame you'll see often these tests attributed to Dave Mitchell, Father Chrysostomos, or whoever was pumpking at the time.

In this file, it seems that mosts of the tests fall into category 2. So, yeah, they're weird by design -- and weird by design means that they'll often throw warnings. The art is to decide which of 3 ways to proceed is best:

1. "Fixing" the code that is generating the warning, in the belief that it was an unintentional error upon the original contributor's part.
2. Acknowledging that the warning is saying something intrinsically important about the code, then capturing it in a $SIG{__WARN__} and writing a new test that demonstrates we captured what we expected.
3. Simply quieting the warning with no warnings 'xxx'; in an appropriately narrow scope.

In this file approach 1 would probably not be wise, as the weirdness of much of the code is intentional and we should respect that intention.

I would take approach 2 for something like these warnings:

Use of uninitialized value $c in concatenation (.) or string at opbasic/concat.t line 48.
Use of uninitialized value $c in concatenation (.) or string at opbasic/concat.t line 48.

... for this code:

ok("$c$alpha$c" eq "foo",    "concatenate undef, fore and aft");

... because the test description signals that the author knew this was weird and demonstrating that we have captured a warning as expected confirms that weirdness.

I can start going through these and turn off expected warnings.

One specific test I arbitrarily picked to examine assigns a *typeglob and then declaring (but not assigning to) a variable of the same name. The goal of the test appears to be that the variable should be valid but not inherit the value from the typeglob when no value has been assigned to it. For this test turning off strict and not declaring the variable should prevent one warning. Alternately both warnings could be suppressed. How would you proceed?

concat.t

*pi = \undef;
# This bug existed earlier than the $2 bug, but is fixed with the same
# patch. Without the fix this 5.7.0 would also croak:
# Modification of a read-only value attempted at ...
my $pi;
eval{"$pi\x{1234}"};
ok(!$@, "bug id 20001020.006 (#4484), constant left");

And here approach 3 is probably best. Given the large number of warnings coming from the block running from line 87 to line 112, I would start off that block with no warnings 'void'; no warnings 'uninitialized';. That would take care of most of the warnings. The ones that remain could be handled by encasing the code in a block and starting that block off with no warnings 'once'; or whatever else is needed.

Ah, the joys of ancient code!

[brainbuz]

There are some void context warnings which can easily be fixed by declaring a variable discard and then changing 'eval{something}' to $discard = 'eval{something}' the test is then to look at the $@ to see if eval failed. Is it better to make this change or to silence the warning?

[jkeenan]

There are some void context warnings which can easily be fixed by declaring a variable discard and then changing 'eval{something}' to $discard = 'eval{something}' the test is then to look at the $@ to see if eval failed. Is it better to make this change or to silence the warning?

I think that, at least in the examples cited in https://github.com/atoomic/perl/pull/323, it is sufficient to silence the warning with no warnings 'void';.

One of the major objections we're bound to get when we propose going to warnings-by-default is that we're being overly pedantic. There are probably cases where creating a variable solely to hold the results of an eval is appropriate. But in the examples in this file, my impression is that the original test author is using eval mainly as a way of demonstrating a weird edge case. The author is consciously not doing anything with the return value. Silencing the warning within a block acknowledges the author's intent while at the same time respecting the thrust of warnings-by-default.

[jkeenan]

With the two pull requests from brainbuz merged just now, t/opbasic/*.t runs warnings-free. So work on this ticket is complete. Closing.

Thank you very much. Jim Keenan