Clarify that __clean_eval does not use strict or warnings.

[toddr]

MM's code to determine version does a local on $VERSION without having
first declared it. Instead it should do 'my' on it unless the variable
is defined at a distance.

my $VERSION = ...
# or
local $Foo::Bar::VERSION = ...

Doing this causes this code to pass strict where it did not previously
do so.

[atoomic]

this is incomplete needs an extra no strict vars

diff --git a/cpan/Module-Metadata/lib/Module/Metadata.pm b/cpan/Module-Metadata/lib/Module/Metadata.pm
index 0309d768ae..1013a7e0bc 100644
--- a/cpan/Module-Metadata/lib/Module/Metadata.pm
+++ b/cpan/Module-Metadata/lib/Module/Metadata.pm
@@ -691,6 +691,7 @@ sub _evaluate_version_line {
   my $eval = qq{ my \$dummy = q#  Hide from _packages_inside()
     #; package Module::Metadata::_version::p${pn};
     use version;
+    no strict 'vars';
     sub {
       local $sigil$variable_name;
       $line;

[atoomic]

when strict is enabled the test suite fails with this change when trying to parse something like this

package Simple;
  our $CVSVERSION   = '$Revision: 1.7 $';
  our ($VERSION)    = ($CVSVERSION =~ /(\d+\.\d+)/);
}

[karenetheridge]

my $VERSION = .. makes no sense as $VERSION must be a package variable in global scope.

[toddr]

my $VERSION = .. makes no sense as $VERSION must be a package variable in global scope.

Agreed. Changed it to our.

[Grinnz]

Sorry this doesn't make any sense. Can you give an example that is currently failing?

[Grinnz]

Perhaps what it should be doing is local our $variablename; (if the variable is not namespaced).

[Grinnz]

• The variables this is dealing with are always package variables.
• This is perfectly legal without strict 'vars' enabled since variables are then package variables by default. So if that line is added there should not be a need for any other changes.
• This line is intending to localize the package variable, it must continue to do so in all cases.

[toddr]

• The variables this is dealing with are always package variables.
• This is perfectly legal without strict 'vars' enabled since variables are then package variables by default. So if that line is added there should not be a need for any other changes.
• This line is intending to localize the package variable, it must continue to do so in all cases.

Right but you can't localize a variable that hasn't yet been declared in the package. In this case, $VERSION was never declared.

perl -Mstrict -e'{ local $VERSION = 5; }' 
Global symbol "$VERSION" requires explicit package name (did you forget to declare "my $VERSION"?) at -e line 1.

So in the end, the issue was that __clean_eval explicitly had strict/warnings off. I think this is hiding some issues but not in scope for this PR.

[Leont]

this is incomplete needs an extra no strict vars

That's one of the tricky things here. Ideally it would eval that line in the same environment as it has in the source file, but it may not be easy to establish if it should be strict or not (or something in between). Not evaluating any of the other lines only complicates that further.

[toddr]

That's one of the tricky things here.

The change has been re-worked. This part is no longer a part of the merge.

[Grinnz]

I'm mostly ok with this, but just would like to note that "no warnings" does not currently bring things to the "default" state, it in fact disables default warnings. However, I don't believe there is any reason for this module to care about default warnings here either.

[karenetheridge]

Is there a specific warning category that is being targeted here? If so, can't we just disable that one?

In other words, what forms of syntax are being tripped up by having warnings enabled?

[Grinnz]

I don't think this is meant to guard against anything specific, but against the possibility of warnings being enabled in an outer scope.

[toddr]

I was simply trying to explicitly do what I guessed the intentions were by declaring this sub prior to use warnings/strict.

[Grinnz]

And as far as I know there is no way to declare that a scope should use what is currently the default set of warnings, which is an unfortunate limitation of the warnings.pm api.

[atoomic]

IMO the current change make the original intent clear.

The function __clean_eval as it stands (before this fix) is eval without warnings and strict hint set.
With this patch, it stays the same but would avoid someone to figure out what's happening and make the intent clear.

Yes this is hiding some issues that would have been exposed with strict and warnings, but I think this is a different case, no?

That code runs without warnings and strict being set.

sub alien { $x = "2:" + 3; return $x }

use strict;
use warnings;

print alien() . "\n";

[Grinnz]

Correct. I am pointing out that it is not in fact the same as without warnings and strict set. "no warnings" also disables default warnings.

[toddr]

Correct. I am pointing out that it is not in fact the same as without warnings and strict set. "no warnings" also disables default warnings.

What do you suggest instead?

[toddr]

I would note that this sub is only used in 2 places and the change that created it seemed to want to turn off all strict/warnings regardless. I didn't write it so I can only speculate on the intention.

[Grinnz]

If I had a better suggestion I would not have approved it 😉