Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subroutine redefined warnings since Perl 5.35.9, dependent on module loading order #20246

Closed
jkeenan opened this issue Sep 4, 2022 · 7 comments
Assignees
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s)

Comments

@jkeenan
Copy link
Contributor

jkeenan commented Sep 4, 2022

(Creating a GH issue from https://www.nntp.perl.org/group/perl.perl5.porters/2022/09/msg264699.html.)

Kenichi Ishigaki wrote to Perl 5 Porters on September 4, 2022:

I have received a bug report that says loading JSON::PP after Cpanel::JSON::XS has started issuing Subroutine redefined warnings (again) since Perl 5.36 (more precisely, 5.35.9).

The report suggests this should be fixed in JSON::PP, and it is possible to add a weird workaround such as this in Cpanel-JSON-XS to JSON-PP, but I am wondering if this is more like a regression in Perl as the warnings reappear only recently.

Any thoughts?

Kenichi, initial thoughts ... On the one hand, these new warnings can be attributed to a change in Perl. On the other hand, you may wish to add a workaround similar to what @rurban did in Cpanel-JSON-XS.

My investigation indicates that these warnings first appeared in this commit:

commit c6874a08f06d60ec8b3f9e21a538a38282910123
Author:     Paul Evans <leonerd@leonerd.org.uk>
AuthorDate: Fri Jan 21 18:37:38 2022 +0000
Commit:     Paul Evans <leonerd@leonerd.org.uk>
CommitDate: Tue Jan 25 15:02:58 2022 +0000

    Fix bundled .pm files for experimental::builtin warnings

...

diff --git a/lib/overload.pm b/lib/overload.pm
index 88e5ecc657..78b72567e4 100644
--- a/lib/overload.pm
+++ b/lib/overload.pm
@@ -2,8 +2,9 @@ package overload;
 
 use strict;
 no strict 'refs';
+no warnings 'experimental::builtin';
 
-our $VERSION = '1.34';
+our $VERSION = '1.35';
 
 our %ops = (
     with_assign         => "+ - * / % ** << >> x .",

This commit actually improved the warnings situation, because if we were to build perl at the previous commit, you would get even murkier warnings. Before and after on FreeBSD:

# c6874a08f06d60ec8b3f9e21a538a38282910123^ FreeBSD
[perlmonger: c6874a08f06d60ec8b3f9e21a538a38282910123^] $ ./bin/perl -Ilib -v | head -2 | tail -1;./bin/perl -Ilib -MCpanel::JSON::XS -E 'say q{hello world cpanel-only};';./bin/perl -Ilib -MJSON::PP -E 'say q{hello world pp-only};';./bin/perl -Ilib -w -e 'use Cpanel::JSON::XS(); use JSON::PP;'
This is perl 5, version 35, subversion 9 (v5.35.9 (v5.35.8-15-g8b1953ca0c)) built for amd64-freebsd-thread-multi
Built-in function 'builtin::blessed' is experimental at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123^/lib/perl5/5.35.9/overload.pm line 104.
hello world cpanel-only
Built-in function 'builtin::blessed' is experimental at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123^/lib/perl5/5.35.9/overload.pm line 104.
hello world pp-only
Built-in function 'builtin::blessed' is experimental at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123^/lib/perl5/5.35.9/overload.pm line 104.

# c6874a08f06d60ec8b3f9e21a538a38282910123 FreeBSD
$ ./bin/perl -Ilib -v | head -2 | tail -1;./bin/perl -Ilib -MCpanel::JSON::XS -E 'say q{hello world cpanel-only};';./bin/perl -Ilib -MJSON::PP -E 'say q{hello world pp-only};';./bin/perl -Ilib -w -e 'use Cpanel::JSON::XS(); use JSON::PP;'
This is perl 5, version 35, subversion 9 (v5.35.9 (v5.35.8-16-gc6874a08f0)) built for amd64-freebsd-thread-multi
hello world cpanel-only
hello world pp-only
Subroutine JSON::PP::Boolean::(++ redefined at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123/lib/perl5/5.35.9/overload.pm line 52.
Subroutine JSON::PP::Boolean::(0+ redefined at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123/lib/perl5/5.35.9/overload.pm line 52.
Subroutine JSON::PP::Boolean::(-- redefined at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123/lib/perl5/5.35.9/overload.pm line 52.

If you were to run a one-liner without warnings at the before and after commits, you would get:

[perlmonger: c6874a08f06d60ec8b3f9e21a538a38282910123^] $ ./bin/perl -Ilib -e 'use Cpanel::JSON::XS(); use JSON::PP;'
Built-in function 'builtin::blessed' is experimental at /home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123^/lib/perl5/5.35.9/overload.pm line 104.
[perlmonger: c6874a08f06d60ec8b3f9e21a538a38282910123^] $ cd -
/home/jkeenan/testing/c6874a08f06d60ec8b3f9e21a538a38282910123
[perlmonger: c6874a08f06d60ec8b3f9e21a538a38282910123] $ ./bin/perl -Ilib -e 'use Cpanel::JSON::XS(); use JSON::PP;'
[perlmonger: c6874a08f06d60ec8b3f9e21a538a38282910123] $ 

So the crucial commit suppressed certain experimental warnings, but (I suspect) earlier commits in this time period created the potential for the warnings which you will only see if you ask for warnings.

@leonerd, can you investigate further?

Thank you very much.
Jim Keenan

@jkeenan
Copy link
Contributor Author

jkeenan commented Sep 21, 2022

@leonerd, can you take a look at this ticket? The problem appears connected to one of your commits from the last dev cycle. Thanks.

@ntyni
Copy link
Contributor

ntyni commented Sep 29, 2022

Hi, could I request some eyeballs on this? It's a clear regression that affects at least MetaCPAN-Client for us, making it warn on plain use MetaCPAN::Client. (This is probably very sensitive to the dependency tree so it may not be easily reproducible with the CPAN toolchain.)

See also https://bugs.debian.org/1019757

Thanks for your work as always!

@jkeenan
Copy link
Contributor Author

jkeenan commented Sep 29, 2022

Hi, could I request some eyeballs on this? It's a clear regression that affects at least MetaCPAN-Client for us, making it warn on plain use MetaCPAN::Client. (This is probably very sensitive to the dependency tree so it may not be easily reproducible with the CPAN toolchain.)

See also https://bugs.debian.org/1019757

Thanks for your work as always!

@leonerd, please take a look at this ticket. Thanks.

@haarg
Copy link
Contributor

haarg commented Sep 30, 2022

Previously, overload.pm did not enable or disable any warnings. This meant that it would emit redefined warnings based on the $^W variable or -w argument. Both JSON::PP and Cpanel::JSON::XS would do local $^W before calling overload.pm, silencing these warnings.

Now, overload.pm includes no warnings 'experimental::builtin';. Since it doesn't enable or disable any other warnings, this ends up taking the default warning set, disabling the experimental::builtin warning, and enforcing this across the entire module. This means changes to $^W after loading the module will have no impact. But the "default" warning set that gets applied depends on the current value of $^W. So when run under -w, having most warnings enabled is baked in at compile time, and can't be changed later.

It would be possible for overload.pm to only disable the experimental::builtin warnings in the immediate scope the builtin functions are used. This would restore the old behavior.

But overload.pm should probably apply a consistent set of warnings to itself. If we wanted to keep the old normal behavior (without -w), this would mean silencing the redefinition warnings. I'm not sure if this is sensible.

As for Cpanel::JSON::XS and JSON::PP, they should probably take advantage of overload's unimport method to remove the methods they intend to overwrite.

@charsbar
Copy link
Contributor

charsbar commented Oct 9, 2022

@haarg , thank you. Shipped JSON::PP 4.12 with your suggested fix.

@jkeenan
Copy link
Contributor Author

jkeenan commented Oct 10, 2022

JSON-PP-4.12 synched into blead via commit 2edec0e.

Will monitor for a few days before closing ticket.

@jkeenan jkeenan self-assigned this Oct 10, 2022
@jkeenan jkeenan added Closable? We might be able to close this ticket, but we need to check with the reporter BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) labels Oct 10, 2022
@jkeenan
Copy link
Contributor Author

jkeenan commented Dec 5, 2022

JSON-PP-4.12 synched into blead via commit 2edec0e.

Will monitor for a few days before closing ticket.

Well, that was more than a few days. No problems reported; closing ticket.

@jkeenan jkeenan closed this as completed Dec 5, 2022
@jkeenan jkeenan removed the Closable? We might be able to close this ticket, but we need to check with the reporter label Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s)
Projects
None yet
Development

No branches or pull requests

4 participants