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

BBC: Spp-2.03 fails since perl 5.31.3 #17256

Open
eserte opened this issue Nov 4, 2019 · 7 comments
Open

BBC: Spp-2.03 fails since perl 5.31.3 #17256

eserte opened this issue Nov 4, 2019 · 7 comments
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s)

Comments

@eserte
Copy link
Contributor

eserte commented Nov 4, 2019

Description
The test suite of https://cpan.metacpan.org/authors/id/S/SS/SSQQ/Spp-2.03.tar.gz fails since 5.31.3. Reports overview: http://matrix.cpantesters.org/?dist=Spp%202.03

@eserte eserte added BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Needs Triage affects-5.31 labels Nov 4, 2019
@dur-randir
Copy link
Member

Bisect points to

commit d1bc97f (refs/bisect/bad)
Author: Hauke D haukex@zero-g.net
AuthorDate: Fri Nov 30 13:06:07 2018 +0100
Commit: Tony Cook tony@develop-help.com
CommitDate: Thu Aug 8 10:40:05 2019 +1000

(perl #133695) "0".."-1" should act like 0..-1

Previously, *any* string beginning with 0, including the string "0"
itself, would be subject to the magic string auto-increment, instead of
being treated like a number. This meant that "-2".."-1" was the same as
-2..-1 and "1".."-1" was the same as 1..-1, but "0".."-1" was the same
as "0".."99".

This patch fixes that inconsistency, while still allowing ranges like
"01".."31" to produce the strings "01", "02", ... "31", which is what
the "begins with 0" exception was intended for.

This patch also expands the documentation in perlop and states the rules
for the range operator in list context with both operands being strings
more explicitly.

See also #18165 and #18114.

@jkeenan
Copy link
Contributor

jkeenan commented Nov 6, 2019

Specifics:

$ bleadperl -v | head -2 | tail -1
This is perl 5, version 31, subversion 6 (v5.31.6 (v5.31.5-163-ga6d5b8290f)) built for x86_64-linux

$ bleadprove -vb t/01-to-spp.t 
t/01-to-spp.t .. 
1..3
redefine token: <door>
redefine token: <door>
Str: |,["Assert",| could not flat! at /home/jkeenan/.cpanm/work/1573072534.12027/Spp-2.03/blib/lib/Spp.pm line 89.
# Looks like your test exited with 255 before it could output anything.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/3 subtests 

Test Summary Report
-------------------
t/01-to-spp.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 3 tests but ran 0.
Files=1, Tests=0,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.18 cusr  0.01 csys =  0.21 CPU)
Result: FAIL

@jkeenan
Copy link
Contributor

jkeenan commented Dec 13, 2019

Bisect points to

commit d1bc97f (refs/bisect/bad)
Author: Hauke D haukex@zero-g.net
AuthorDate: Fri Nov 30 13:06:07 2018 +0100
Commit: Tony Cook tony@develop-help.com
CommitDate: Thu Aug 8 10:40:05 2019 +1000

(perl #133695) "0".."-1" should act like 0..-1

Previously, *any* string beginning with 0, including the string "0"
itself, would be subject to the magic string auto-increment, instead of
being treated like a number. This meant that "-2".."-1" was the same as
-2..-1 and "1".."-1" was the same as 1..-1, but "0".."-1" was the same
as "0".."99".

This patch fixes that inconsistency, while still allowing ranges like
"01".."31" to produce the strings "01", "02", ... "31", which is what
the "begins with 0" exception was intended for.

This patch also expands the documentation in perlop and states the rules
for the range operator in list context with both operands being strings
more explicitly.

See also #18165 and #18114.

@tonycoz, have you seen this BBC report?

@jkeenan
Copy link
Contributor

jkeenan commented Jan 28, 2020

Bisect points to
commit d1bc97f (refs/bisect/bad)
Author: Hauke D haukex@zero-g.net
AuthorDate: Fri Nov 30 13:06:07 2018 +0100
Commit: Tony Cook tony@develop-help.com
CommitDate: Thu Aug 8 10:40:05 2019 +1000

(perl #133695) "0".."-1" should act like 0..-1

Previously, *any* string beginning with 0, including the string "0"
itself, would be subject to the magic string auto-increment, instead of
being treated like a number. This meant that "-2".."-1" was the same as
-2..-1 and "1".."-1" was the same as 1..-1, but "0".."-1" was the same
as "0".."99".

This patch fixes that inconsistency, while still allowing ranges like
"01".."31" to produce the strings "01", "02", ... "31", which is what
the "begins with 0" exception was intended for.

This patch also expands the documentation in perlop and states the rules
for the range operator in list context with both operands being strings
more explicitly.

See also #18165 and #18114.

@tonycoz, have you seen this BBC report?

@tonycoz, have you seen this BBC report?

Thank you very much.
Jim Keenan

@haukex
Copy link
Contributor

haukex commented Feb 10, 2020

I came across this issue by chance. It took a while to find, but the offending line is this one: return $r ~~ ['0' .. '9'];, replacing it with e.g. return $r =~ /\A[0-9]\z/; fixes the issue. It boils down to this:

$ perlbrew exec --with=perl-5.30.1,perl-5.31.8 perl -wM5.030 -le 'say "\x20"~~["0".."9"]?"Yes":"No"'
perl-5.30.1
==========
Smartmatch is experimental at -e line 1.
No

perl-5.31.8
==========
Smartmatch is experimental at -e line 1.
Argument " " isn't numeric in smart match at -e line 1.
Yes

Because where Perl previously was returning a list of strings for "0".."9", it now returns a list of integers (#16770 / RT133695), and a number on the RHS causes smart match to use == instead of eq.

A "clever" workaround is $r ~~ ['0' .. 'x'], but I've submitted a patch to the module author with the regex version, since that isn't experimental. I also proposed an addition to the docs in #17552.

@tonycoz
Copy link
Contributor

tonycoz commented Feb 10, 2020

@tonycoz, have you seen this BBC report?

@tonycoz, have you seen this BBC report?

Sorry, I missed this, I've disabled successful github action spam so hopefully I'll miss less.

I think the original change is still valid, and the user is using an experimental feature.

The only way I can see to fix this while retaining the new and desired behaviour would be to record the old PVness when incrementing and re-apply it after we've calculated the new value, but I don't think it's worth the cost in performance to other users.

The sensitivity to normally transparent flags is one of the flaws with smartmatch, I don't think we should hold other parts of perl's development back to support those flaws.

@jkeenan
Copy link
Contributor

jkeenan commented Feb 10, 2020

I can confirm that the patch which @haukex submitted upstream to Spp's author enables all tests to pass against blead.

So, while this is technically still a BBC ticket, it's also a "must-be-fixed-upstream" ticket and is not a blocker for perl-5.32. How should we adjust the labels attached to this ticket to reflect that?

Thank you very much.
Jim Keenan

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

7 participants