Disable "shadow" warnings category by default

[tobyink]

no warnings "shadow" is fatal on regular Perl because there's no such category.

[rurban]

But why should cperl disable it then? lot of perl5 code is wrong because of that mistake. cperl needs to catch it.

[tobyink]

How would you recommend I disable these warnings then?

BEGIN {
  'warnings'->unimport('shadow') if "$^V" =~ /c$/;
};

At the top of every file affected (because it needs to be before the package statement) is pretty ugly.

[tobyink]

Note that I'm not saying use warnings or -w shouldn't be able to enable these warnings. But right now, they happen without opting into them at all.

[rurban]

No, I explicitly defaulted this new warning, because it detects real problems which should not be silenced. Rather fix the problem instead.

[tobyink]

It doesn't detect problems though; it detects situations which might result in problems. Like how if you were parsing HTML and somehow ended up with a variable containing what you thought was a number, but it actually contained "5</span>", then:

say $var+1;

Would result in a warning Argument "5</span>" isn't numeric in addition. This isn't warning you about a problem because it does still give the correct answer of 6. It's warning you about a situation that might result in future problems.

The difference though is that "numeric" warnings are opt-in.

"shadow" warnings are enabled without opting in. And the obvious way to opt out:

no warnings "shadow";

Is not compatible with p5p's perl.

[rurban]

Hmm, will discuss within my group, which came up with this problem.

[tobyink]

To summarize my feelings on this warning, because I don't think I have necessarily expressed my self clearly.

my $var = "5</span>";   # perhaps because of broken HTML scraping
say 1+$var;             # warns about argument not being numeric

I think we can all agree that the "numeric" warning category can be helpful, however:

• Perl (neither p5p perl nor cperl!) does not enable this warning by default. It needs to be enabled using use warnings or -w.
• If it is enabled, it can easily be disabled lexically using no warnings "numeric".

The "shadow" warnings category is also useful, however:

• It is enabled by default in cperl, without opting in.
• It cannot easily be disabled because the obvious way of disabling it (no warnings "shadow") will cause a fatal error on p5p perl.

I do not dispute the usefulness of the warning, or question its existence. I only question the wisdom of making it enabled by default when it's annoying to disable.

[rurban]

Sure, I get your point. Our side is that many external API's are wrong, and no one knows why. The external API does not have this warning enabled, so you'll never catch it. Extraordinary API breakage should require extraordinary measures to circumvent it.

Which package warns for you? Cannot it be fixed. Is it a false positive. There might be cases, where the shadow warning should be suppressed, but I want to see them first. It's not comparable to numeric because there you can simply ignore it and continue. shadowed namespaces however are mostly an error, because some method hijacked your whole package.

[tobyink]

I've been trying to get the test suite for my own distribution, Type::Tiny, passing on cperl. The version on github does now pass, albeit with these warnings and needing to skip a handful of tests.

The warnings are mostly caused by the fact that Types::Standard exports functions like HashRef and ArrayRef which return objects blessed into the "Type::Tiny" class. As a load-time optimization, some of these objects will load parts of their implementation from modules like Types::Standard::HashRef and Types::Standard::ArrayRef on demand. So there's a function Types::Standard::HashRef and a package Types::Standard::HashRef. The latter is indirectly used by the former and not intended to be used by end users.

A solution I've considered would be to change something like this:

# lib/Types/Standard/HashRef.pm
package Types::Standard::HashRef;
sub helper1 { ... }
sub helper2 { ... }

To something like this:

# lib/Types/Standard/HashRef.pm
package Types::Standard;
sub Types::Standard::HashRef::helper1 { ... }
sub Types::Standard::HashRef::helper2 { ... }

I don't think this would trigger the "shadow" warning, but it seems to just be exploiting a current weakness in the code that detects violations, and there's no guarantee that this won't also generate a warning in the future, so long term it's better to address the problem at its root rather than work around it like this.

Whatsmore, the CPAN indexer looks for the "package" keyword when it's indexing distributions, so for namespace management purposes, it seems like a bad idea to make this change.

Another alternative would be to rename the Types::Standard::HashRef package to something like Types::Standard::Internals::HashRef but due to the way CPAN installers work, I believe on upgrading from an old version, people would end up with a redundant old version of "lib/Types/Standard/HashRef.pm" hanging around, probably not causing any harm, but still an annoyance. I don't like this solution.

So yeah, short answer is I have function Foo::Bar and package Foo::Bar, but the subs defined in package Foo::Bar are never going to be called as methods on a Foo::Bar bareword, so the issues the shadow warning is designed to protect you against are never going to arise anyway.

[rurban]

Toby Inkster notifications@github.com schrieb am Mi., 11. März 2020, 10:22:

I've been trying to get the test suite for my own distribution, Type::Tiny, passing on cperl. The version on github does now pass, albeit with these warnings and needing to skip a handful of tests.

The warnings are mostly caused by the fact that Types::Standard exports functions like HashRef and ArrayRef which return objects blessed into the "Type::Tiny" class. As a load-time optimization, some of these objects will load parts of their implementation from modules like Types::Standard::HashRef and Types::Standard::ArrayRef on demand. So there's a function Types::Standard::HashRef and a package Types::Standard::HashRef. The latter is indirectly used by the former and not intended to be used by end users.

A solution I've considered would be to change something like this:

lib/Types/Standard/HashRef.pmpackage Types::Standard::HashRef;sub helper1 { ... }sub helper2 { ... }

To something like this:

lib/Types/Standard/HashRef.pmpackage Types::Standard;sub Types::Standard::HashRef::helper1 { ... }sub Types::Standard::HashRef::helper2 { ... }

I don't think this would trigger the "shadow" warning, but it seems to just be exploiting a current weakness in the code that detects violations, and there's no guarantee that this won't also generate a warning in the future, so long term it's better to address the problem at its root rather than work around it like this.

Whatsmore, the CPAN indexer looks for the "package" keyword when it's indexing distributions, so for namespace management purposes, it seems like a bad idea to make this change.

Another alternative would be to rename the Types::Standard::HashRef package to something like Types::Standard::Internals::HashRef but due to the way CPAN installers work, I believe on upgrading from an old version, people would end up with a redundant old version of "lib/Types/Standard/HashRef.pm" hanging around, probably not causing any harm, but still an annoyance. I don't like this solution.

So yeah, short answer is I have function Foo::Bar and package Foo::Bar, but the subs defined in package Foo::Bar are never going to be called as methods on a Foo::Bar bareword, so the issues the shadow warning is designed to protect you against are never going to arise anyway.

I see. A good case for an exception in the warning check. But only for upcoming cperl versions.