5.23.8 + "Assuming NOT a POSIX" causes spurious warning in PPIx::Regexp::Token::Literal

p5pRT commented 8 years ago

Migrated from rt.perl.org#127581 (status was 'resolved')

Searchable as RT127581$

p5pRT commented 8 years ago

From @kentfredric

This is a bug report for perl from kentnl@cpan.org\, generated with the help of perlbug 1.40 running under perl 5.23.8.

The following line of code raises a warning\, despite looking like legal use of POSIX character classes:

https://metacpan.org/source/WYANT/PPIx-Regexp-0.047/lib/PPIx/Regexp/Token/Literal.pm#L242

This cascades into Perl::Critic::Pulp where that warning becomes an error:

http://www.cpantesters.org/cpan/report/3df914e0-d0cb-11e5-82fe-70d067b6c32e

The code in question doesn't seem obviously wrong:

c [][[:alpha:]\@\\^_?] | # control characters

Flags: category=core severity=low

Site configuration information for perl 5.23.8:

Configured by kent at Sun Feb 21 13:35:32 NZDT 2016.

Summary of my perl5 (revision 5 version 23 subversion 8) configuration:
Platform: osname=linux\, osvers=4.4.0-gentoo-r1\, archname=x86_64-linux uname='linux katipo2 4.4.0-gentoo-r1 #33 smp preempt fri jan 29 01:03:50 nzdt 2016 x86_64 intel(r) core(tm) i5-2410m cpu @ 2.30ghz genuineintel gnulinux ' config_args='-de -Dprefix=/home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc -Doptimize= -fno-stack-protector -O3 -march=native -mtune=native -Dman1dir=none -Dman3dir=none -Dusedevel -Accflags= -fno-stack-protector -DPERL_HASH_FUNC_SDBM -DPERL_DISABLE_PMC -Aldflags= -fno-stack-protector -Aeval:scriptdir=/home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/bin' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef use64bitint=define\, use64bitall=define\, uselongdouble=undef usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fno-stack-protector -DPERL_HASH_FUNC_SDBM -DPERL_DISABLE_PMC -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\, optimize=' -fno-stack-protector -O3 -march=native -mtune=native'\, cppflags='-fno-stack-protector -DPERL_HASH_FUNC_SDBM -DPERL_DISABLE_PMC -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong' ccversion=''\, gccversion='5.3.0'\, gccosandvers='' intsize=4\, longsize=8\, ptrsize=8\, doublesize=8\, byteorder=12345678\, doublekind=3 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16\, longdblkind=3 ivtype='long'\, ivsize=8\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8 alignbytes=8\, prototype=define Linker and Libraries: ld='cc'\, ldflags =' -fno-stack-protector -fstack-protector-strong -L/usr/local/lib' libpth=/usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/include-fixed /usr/lib /usr/local/lib /lib/../lib64 /usr/lib/../lib64 /lib /lib64 /usr/lib64 /usr/local/lib64 libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.22.so\, so=so\, useshrplib=false\, libperl=libperl.a gnulibc_version='2.22' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E' cccdlflags='-fPIC'\, lddlflags='-shared -fno-stack-protector -O3 -march=native -mtune=native -L/usr/local/lib -fstack-protector-strong'

@INC for perl 5.23.8: /home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/lib/site_perl/5.23.8/x86_64-linux /home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/lib/site_perl/5.23.8 /home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/lib/5.23.8/x86_64-linux /home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/lib/5.23.8 .

Environment for perl 5.23.8: HOME=/home/kent LANG=en_NZ.UTF8 LANGUAGE (unset) LC_CTYPE=en_NZ.UTF8 LC_TIME=en_NZ.UTF8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/kent/perl5/perlbrew/bin:/home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/bin:/home/kent/.perl6/2013.04/bin:/home/kent/.gem/ruby/1.8/bin/:/home/kent/.rvm/gems/ruby-2.1.2/bin:/home/kent/.rvm/gems/ruby-2.1.2@global/bin:/home/kent/.rvm/rubies/ruby-2.1.2/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/5.3.0:/usr/games/bin:/home/kent/.rvm/bin:/home/kent/.rvm/bin PERLBREW_BASHRC_VERSION=0.74 PERLBREW_HOME=/home/kent/.perlbrew PERLBREW_MANPATH=/home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/man PERLBREW_PATH=/home/kent/perl5/perlbrew/bin:/home/kent/perl5/perlbrew/perls/5.23.8-nossp-sdbm-nopmc/bin PERLBREW_PERL=5.23.8-nossp-sdbm-nopmc PERLBREW_ROOT=/home/kent/perl5/perlbrew PERLBREW_VERSION=0.74 PERL_BADLANG (unset) SHELL=/bin/bash

p5pRT commented 8 years ago

From @tonycoz

On Sat Feb 20 18:36:56 2016\, kentfredric wrote:

c [][[:alpha:]\@\\^_?] | # control characters

$ ./perl -Ilib -We 'qr/[][[:alpha:]]/' Assuming NOT a POSIX class since there must be a starting ':' in regex; marked by \<-- HERE in m/[][ \<-- HERE [:alpha:]]/ at -e line 1. $ ./perl -Ilib -We 'qr/[[:alpha:]]/' $

Tony

p5pRT commented 8 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 8 years ago

From @cpansprout

On Sat Feb 20 18:36:56 2016\, kentfredric wrote:

This is a bug report for perl from kentnl@cpan.org\, generated with the help of perlbug 1.40 running under perl 5.23.8.

-----------------------------------------------------------------

The following line of code raises a warning\, despite looking like legal use of POSIX character classes:

https://metacpan.org/source/WYANT/PPIx-Regexp- 0.047/lib/PPIx/Regexp/Token/Literal.pm#L242

This cascades into Perl::Critic::Pulp where that warning becomes an error:

http://www.cpantesters.org/cpan/report/3df914e0-d0cb-11e5-82fe- 70d067b6c32e

The code in question doesn't seem obviously wrong:

c [][[:alpha:]\@\\^_?] | # control characters

In fact\, I don’t see *anything* wrong with it. It looks as though this new warning is always going to trigger false positives. I can’t see any way around that (except by removing the warning).

--

Father Chrysostomos

p5pRT commented 8 years ago

From @khwilliamson

On 02/20/2016 10:42 PM\, Father Chrysostomos via RT wrote:

On Sat Feb 20 18:36:56 2016\, kentfredric wrote:

This is a bug report for perl from kentnl@cpan.org\, generated with the help of perlbug 1.40 running under perl 5.23.8.

-----------------------------------------------------------------

The following line of code raises a warning\, despite looking like legal use of POSIX character classes:

https://metacpan.org/source/WYANT/PPIx-Regexp- 0.047/lib/PPIx/Regexp/Token/Literal.pm#L242

This cascades into Perl::Critic::Pulp where that warning becomes an error:

http://www.cpantesters.org/cpan/report/3df914e0-d0cb-11e5-82fe- 70d067b6c32e

The code in question doesn't seem obviously wrong:

c [][[:alpha:]\@\\^_?] | # control characters

In fact\, I don’t see *anything* wrong with it. It looks as though this new warning is always going to trigger false positives. I can’t see any way around that (except by removing the warning).

Actually\, it is fairly easy to get rid of this entire class of false positive.

The changes I made do not change what actually get compiled. So this warning was wrong\, the [:alpha:] was parsed correctly. The warning was actually saying that this portion '[[:alpha:]' was not a valid posix class. It's easy to suppress the warning if that region of text overlaps a region that had a proper posix class. And I'll do that over the next few days. Thus the warning can be made to appear only when something isn't parsed as a posix class\, but looks like it was intended to be one.

p5pRT commented 8 years ago

From @trwyant

This warning is suppressed in PPIx-Regexp 0.048\, which went to PAUSE yesterday. There's no test coverage tool that I know of to test regex coverage\, but I believe the regex was functioning correctly.

What happened was that the lazy way to include a right square bracket in a bracketed character class is to put it first. Having square brackets on the brain\, I put the left square bracket next\, and followed with the POSIX class. The fix was to shuffle the \@ before the POSIX class\, as suggested by perldiag. I admit that I grumbled to myself a little over this\, but on reflection it's no different than shuffling that right square bracket to the front.

As I noted to a correspondent on this issue\, Perl is so overloaded that it is fairly easy to write something that is valid Perl but nevertheless quite different than the author's intention\, and warnings like this have a long history. But if you will allow the opinion of someone whose normal involvement with the porters' mailing list is reading (with profound thanks!) SawyerX's summaries\, I personally would appreciate the suppression of the false positive (which I believe this is) if it can be done without eliminating the message completely.

With thanks to all of you for the continuous improvement of Perl 5\, Tom Wyant

p5pRT commented 8 years ago

From @khwilliamson

This has been fixed by b7c50ab8b3b6603d103ce357115e8ba2e741742d

I forgot to flesh out the commit message\, so here is some more detail:

Basically the fix is to delay the warning message until we are sure it was not a false positive. In the failure from this ticket:

/[][[:alpha:]]

it sees the 2nd '[' (the 3rd character within the pattern\, immediately after a ']') and starts looking for a posix class. It finds one at that position\, but with an apparent typo: '[[:alpha:]. So it raised a warning. Now\, instead\, it saves the warning. Then it starts parsing at the next '[' and sees a legitimate class '[:alpha:]'\, and it then compiled that\, but the warning had already escaped. With this commit\, the warning will be cleared if a later legitimate class is found that overlaps the area the warning was about. -- Karl Williamson

p5pRT commented 8 years ago

@khwilliamson - Status changed from 'open' to 'pending release'

p5pRT commented 8 years ago

From @khwilliamson

On 03/01/2016 09:44 AM\, Tom Wyant via RT wrote:

This warning is suppressed in PPIx-Regexp 0.048\, which went to PAUSE yesterday. There's no test coverage tool that I know of to test regex coverage\, but I believe the regex was functioning correctly.

What happened was that the lazy way to include a right square bracket in a bracketed character class is to put it first. Having square brackets on the brain\, I put the left square bracket next\, and followed with the POSIX class. The fix was to shuffle the \@ before the POSIX class\, as suggested by perldiag. I admit that I grumbled to myself a little over this\, but on reflection it's no different than shuffling that right square bracket to the front.

As I noted to a correspondent on this issue\, Perl is so overloaded that it is fairly easy to write something that is valid Perl but nevertheless quite different than the author's intention\, and warnings like this have a long history. But if you will allow the opinion of someone whose normal involvement with the porters' mailing list is reading (with profound thanks!) SawyerX's summaries\, I personally would appreciate the suppression of the false positive (which I believe this is) if it can be done without eliminating the message completely.

With thanks to all of you for the continuous improvement of Perl 5\, Tom Wyant

--- via perlbug: queue: perl5 status: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127581

I'm sorry that you felt you had to change your code. Your code was correct\, and it was Perl that needed to change. And blead has now been changed to fix this problem\, and so your original code would now work properly without a warning. The code the regex pattern compiled to was always correct; it just had an inappropriate warning raised.

There will be false positives with this new warning\, but there should not be any where there is actually a legal posix class\, as in your case.

p5pRT commented 8 years ago

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
With the release of Perl 5.24.0 on May 9\, 2016\, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

p5pRT commented 8 years ago

@khwilliamson - Status changed from 'pending release' to 'resolved'

Perl / perl5