Perl-Toolchain-Gang / Module-Metadata

Gather package and POD information from perl module files
http://metacpan.org/release/Module-Metadata/
Other
8 stars 16 forks source link

Clarify that __clean_eval does not use strict or warnings. #35

Open toddr opened 4 years ago

toddr commented 4 years ago

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 commented 4 years ago

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;
karenetheridge commented 4 years ago

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

toddr commented 4 years ago

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

Agreed. Changed it to our.

Grinnz commented 4 years ago

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

Grinnz commented 4 years ago

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

Grinnz commented 4 years ago
toddr commented 4 years ago
  • 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 commented 4 years ago

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 commented 4 years ago

That's one of the tricky things here.

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