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

/(?-p)/ should be an error #14936

Closed
p5pRT opened this issue Sep 25, 2015 · 11 comments
Closed

/(?-p)/ should be an error #14936

p5pRT opened this issue Sep 25, 2015 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 25, 2015

Migrated from rt.perl.org#126185 (status was 'rejected')

Searchable as RT126185$

@p5pRT
Copy link
Author

p5pRT commented Sep 25, 2015

From victor@drawall.cc

Created by @Grimy

How to reproduce
----------------

perl5.23.4 -e '/(?-p)/p; print defined for ${^MATCH}, ${^PREMATCH}, ${^POSTMATCH}'

Expected behavior
-----------------

Perl should die with the following diagnostic​:

Regexp modifier "p" may not appear after the "-" in regex; marked by <-- HERE in m/(?-p <-- HERE )/ at -e line 1.

Actual behavior
---------------

Perl terminates normally and prints​:

111

The ${^MATCH} variables get defined, which proves that /p is still in effect.
This shows that (?-p) is completely ignored. This also goes against the
documentation, which says that only i, m, s and x can go on the right side of
the minus sign.

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.23.4:

Configured by grimy at Tue Sep 22 21:18:14 CEST 2015.

Summary of my perl5 (revision 5 version 23 subversion 4) configuration:
  Commit id: 2d9b5f101563ac9fee41e6ca496f79db6222d2e3
  Platform:
    osname=linux, osvers=4.0.7-2-arch, archname=x86_64-linux
    uname='linux localhost 4.0.7-2-arch #1 smp preempt tue jun 30
07:50:21 utc 2015 x86_64 gnulinux '
    config_args='-ds -e -Dusedevel'
    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 ='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include'
    ccversion='', gccversion='5.1.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 =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib
/usr/lib/gcc/x86_64-unknown-linux-gnu/5.1.0/include-fixed /usr/lib
/lib/../lib /usr/lib/../lib /lib /lib64 /usr/lib64
    libs=-lpthread -lnsl -lnm -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
-lgdbm_compat
    perllibs=-lpthread -lnsl -lnm -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.21.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.21'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib
-fstack-protector-strong'



@INC for perl 5.23.4:
    /usr/local/lib/perl5/site_perl/5.23.4/x86_64-linux
    /usr/local/lib/perl5/site_perl/5.23.4
    /usr/local/lib/perl5/5.23.4/x86_64-linux
    /usr/local/lib/perl5/5.23.4
    /usr/local/lib/perl5/site_perl
    .


Environment for perl 5.23.4:
    HOME=/home/grimy
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/grimy/bin:/home/grimy/.nvim/scripts:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/opt/plan9/bin
    PERL_BADLANG (unset)
    SHELL (unset)

@p5pRT
Copy link
Author

p5pRT commented Oct 7, 2015

From @demerphq

On 25 September 2015 at 19​:03, Victor ADAM <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by Victor ADAM
# Please include the string​: [perl #126185]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=126185 >

This is a bug report for perl from victor.adam@​derpymail.org,
generated with the help of perlbug 1.40 running under perl 5.23.4.

-----------------------------------------------------------------
[Please describe your issue here]

How to reproduce
----------------

perl5.23.4 -e '/(?-p)/p; print defined for ${^MATCH}, ${^PREMATCH}, ${^POSTMATCH}'

Expected behavior
-----------------

Perl should die with the following diagnostic​:

Regexp modifier "p" may not appear after the "-" in regex; marked by <-- HERE in m/(?-p <-- HERE )/ at -e line 1.

I dont know that I agree that Perl should die. We warn instead​:

$ ./perl -we'qr/(?-p)foo/'
Useless use of (?-p) in regex; marked by <-- HERE in m/(?-p <-- HERE
)foo/ at -e line 1.

Given the mild consequences of this type of mistake I think warnings
are sufficient.

Unless someone strongly disagrees I would like to close this ticket
as "wont fix".

Thanks for the report though!

Yves

Yves

@p5pRT
Copy link
Author

p5pRT commented Oct 7, 2015

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

@p5pRT
Copy link
Author

p5pRT commented Oct 7, 2015

From @khwilliamson

On 10/07/2015 05​:34 AM, demerphq wrote​:

On 25 September 2015 at 19​:03, Victor ADAM <perlbug-followup@​perl.org> wrote​:

# New Ticket Created by Victor ADAM
# Please include the string​: [perl #126185]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=126185 >

This is a bug report for perl from victor.adam@​derpymail.org,
generated with the help of perlbug 1.40 running under perl 5.23.4.

-----------------------------------------------------------------
[Please describe your issue here]

How to reproduce
----------------

perl5.23.4 -e '/(?-p)/p; print defined for ${^MATCH}, ${^PREMATCH}, ${^POSTMATCH}'

Expected behavior
-----------------

Perl should die with the following diagnostic​:

Regexp modifier "p" may not appear after the "-" in regex; marked by <-- HERE in m/(?-p <-- HERE )/ at -e line 1.

I dont know that I agree that Perl should die. We warn instead​:

$ ./perl -we'qr/(?-p)foo/'
Useless use of (?-p) in regex; marked by <-- HERE in m/(?-p <-- HERE
)foo/ at -e line 1.

Given the mild consequences of this type of mistake I think warnings
are sufficient.

Unless someone strongly disagrees I would like to close this ticket
as "wont fix".

Thanks for the report though!

I agree with Yves. But I think the ticket should be rejected rather
than wont fix, as I think the latter indicates that we agree that this
is a problem but the downside of changing it outweighs the upside (for
example too hard to fix or it would break too much existing code). And
here, I think that the consequences are mild, so it was the right design
decision to be a warning. There are places where I think we've made the
wrong design decision and carried on in the face of garbage input and
done some action that is almost certainly not what was intended, but
this isn't one of those places IMO.

@p5pRT
Copy link
Author

p5pRT commented Oct 7, 2015

From victor@drawall.cc

I think that the consequences are mild, so it was the right design decision to be a warning.

In that case we should fix all the equivalent but inconsistent errors​:
/(?-a)/ dies with “Regexp modifier "a" may not appear after the "-" in
regex; marked by <-- HERE in m/(?-a <-- HERE )/ at -e line 1.”

Unless I’m missing something, the two are essentially the same, and
should thus have the same behavior. Having /(?-a)/ die but /(?-p)/
warn doesn’t sound like a “right design decision” to me.

@p5pRT
Copy link
Author

p5pRT commented Oct 7, 2015

From @Abigail

On Wed, Oct 07, 2015 at 10​:02​:22PM +0200, Victor ADAM wrote​:

I think that the consequences are mild, so it was the right design decision to be a warning.

In that case we should fix all the equivalent but inconsistent errors​:
/(?-a)/ dies with “Regexp modifier "a" may not appear after the "-" in
regex; marked by <-- HERE in m/(?-a <-- HERE )/ at -e line 1.”

Unless I’m missing something, the two are essentially the same, and
should thus have the same behavior. Having /(?-a)/ die but /(?-p)/
warn doesn’t sound like a “right design decision” to me.

Yeah, but we have to be practical. /a is fairly new, and /(?-a)/ has
died since /a was introduced. /(?-p)/ has been allowed for some time.
We don't know whether there's code out there which has it. Or how much. [1]
Considering that it's pretty harmless, I don't think dying is appropriate.
Perhaps we should not even add a warning now. Now, if /p was new, I
would not object to it dying.

What's the gain if /(?-p)/ started dying?

[1] If there's a lot of code out there which uses /(?-p)/, then making
  it die (or even warn) now hurts a lot of people. If there isn't a
  lot of code out there which uses it, then it hardly matters what
  is done (die, warn, nothing).

Abigail

@p5pRT
Copy link
Author

p5pRT commented Oct 7, 2015

From @khwilliamson

On 10/07/2015 03​:13 PM, Abigail wrote​:

On Wed, Oct 07, 2015 at 10​:02​:22PM +0200, Victor ADAM wrote​:

I think that the consequences are mild, so it was the right design decision to be a warning.

In that case we should fix all the equivalent but inconsistent errors​:
/(?-a)/ dies with “Regexp modifier "a" may not appear after the "-" in
regex; marked by <-- HERE in m/(?-a <-- HERE )/ at -e line 1.”

Unless I’m missing something, the two are essentially the same, and
should thus have the same behavior. Having /(?-a)/ die but /(?-p)/
warn doesn’t sound like a “right design decision” to me.

Yeah, but we have to be practical. /a is fairly new, and /(?-a)/ has
died since /a was introduced. /(?-p)/ has been allowed for some time.
We don't know whether there's code out there which has it. Or how much. [1]
Considering that it's pretty harmless, I don't think dying is appropriate.
Perhaps we should not even add a warning now. Now, if /p was new, I
would not object to it dying.

What's the gain if /(?-p)/ started dying?

[1] If there's a lot of code out there which uses /(?-p)/, then making
it die (or even warn) now hurts a lot of people. If there isn't a
lot of code out there which uses it, then it hardly matters what
is done (die, warn, nothing).

Abigail

To be sure, there is some subjectivity to this whole thing, and
reasonable people can come to different conclusions.

I agree with Abigail, but also, it's obvious what -p would mean, because
p is a boolean flag. But it's not obvious what -a would mean. It is a
character set modifier, and these are not booleans. One of them is
always in effect, /a /d /l /u or /aa. To change the character set you
say which one you now want; you can't just deselect one, because it's
not obvious what you're selecting instead. So, it's not clear what -a
means at all, and therefore it should be an error.

@p5pRT
Copy link
Author

p5pRT commented Oct 8, 2015

From victor@drawall.cc

What's the gain if /(?-p)/ started dying?

The gain would be consistency. However, your reasoning in [1]
convinced me​: the pragmatic thing to do is to leave the code as is.
This can be closed.

@p5pRT
Copy link
Author

p5pRT commented Oct 8, 2015

From @khwilliamson

OP said to close this ticket
--
Karl Williamson

@p5pRT p5pRT closed this as completed Oct 8, 2015
@p5pRT
Copy link
Author

p5pRT commented Oct 8, 2015

@khwilliamson - Status changed from 'open' to 'rejected'

@p5pRT
Copy link
Author

p5pRT commented Oct 8, 2015

From @demerphq

On 7 October 2015 at 23​:36, Karl Williamson <public@​khwilliamson.com> wrote​:

On 10/07/2015 03​:13 PM, Abigail wrote​:

On Wed, Oct 07, 2015 at 10​:02​:22PM +0200, Victor ADAM wrote​:

I think that the consequences are mild, so it was the right design
decision to be a warning.

In that case we should fix all the equivalent but inconsistent errors​:
/(?-a)/ dies with “Regexp modifier "a" may not appear after the "-" in
regex; marked by <-- HERE in m/(?-a <-- HERE )/ at -e line 1.”

Unless I’m missing something, the two are essentially the same, and
should thus have the same behavior. Having /(?-a)/ die but /(?-p)/
warn doesn’t sound like a “right design decision” to me.

Yeah, but we have to be practical. /a is fairly new, and /(?-a)/ has
died since /a was introduced. /(?-p)/ has been allowed for some time.
We don't know whether there's code out there which has it. Or how much.
[1]
Considering that it's pretty harmless, I don't think dying is appropriate.
Perhaps we should not even add a warning now. Now, if /p was new, I
would not object to it dying.

What's the gain if /(?-p)/ started dying?

[1] If there's a lot of code out there which uses /(?-p)/, then making
it die (or even warn) now hurts a lot of people. If there isn't a
lot of code out there which uses it, then it hardly matters what
is done (die, warn, nothing).

Abigail

To be sure, there is some subjectivity to this whole thing, and reasonable
people can come to different conclusions.

I agree with Abigail, but also, it's obvious what -p would mean, because p
is a boolean flag. But it's not obvious what -a would mean.

While I realize Victor already agreed that this ticket can be
closed[1] I wanted to add some comments.

(?p) is a bit of an odd case as far as regex flags go. In some ways
it is the platypus of the regex modifiers. :-)

Unlike most of the other modifiers it shouldn't exist at all (arguably
like /o), and that it does is purely to work around performance issues
in our implementation. Thus its presence or absence shouldn't change
anything. If we reworked other bits of the implementation it could be
made a complete no-op, and ${^MATCH} and friends would just be an
always available long form for $& and friends, or perhaps deprecated
outright in some future version.

Furthermore regex modifier flags are divided into two groups, parse
modifiers and execution modifiers. Parse modifiers are used to change
the meaning of parts of the pattern, such as (?i) making the following
text case-insensitive, so they must be embeddable. Execution modifiers
are things like /g which control how the pattern will be executed, and
are not embeddable for the obvious reason that it makes no sense to
have the pattern control something like the /g flag.

On the face of things, /p flag is an execution modifier like /g, which
would then argue that it should not be embeddable. *BUT*, unlike the
other execution modifiers it has a side effect that is visible inside
of (?{ ... }) and (??{ ... }), which means it must be embeddable, or
it would not be possible to use ${^MATCH} and friends inside of things
like (??{ }). For instance, imagine injecting a qr// object with
appropriate behaviour into a library that accepts a pattern as a
debugging aid, like in the following toy example​:

perl -le'sub do_match { my ($string,$pattern)= @​_; my $count=0; while
($string=~/$pattern/gc) { $count++ } return $count} my $qr= qr/fo+(?{
print "${^PREMATCH}|${^MATCH}|${^POSTMATCH}" })[ab]/p;
do_match("foofooooomfooob",$qr)'
|foo|fooooomfooob
|fo|ofooooomfooob
foo|fooooo|mfooob
foo|foooo|omfooob
foo|fooo|oomfooob
foo|foo|ooomfooob
foo|fo|oooomfooob
foofooooom|fooo|b

Once an execution flag becomes embeddable you have to decide what to
do about the "minus case", and because it did no harm I chose to warn,
not die, if someone used it. In hindsight I think I was wrong to do
so. Depending on what you think we should do in the future when/if the
preserve flag becomes unnecessary, it should either die, or be a
silent no-op. But that is the "deprecation debate" in disguise and is
better left I think to a different thread.

Whatever we chose to do about (?-p), it should be because it makes
sense in context with (?p), and not /just/ to be consistent with what
(?-a) does. (?a) is a parse modifier, and it is not obvious that (?-a)
is senseless[1]. Perhaps in the future we will give it meaning
(however unlikely), which would then change the semantics of the
match. So making it die now keeps that option open for the future.

Cheers,
Yves
[1] I could imagine /(?a)blah(?-a)foo/ to mean the same thing as
/(?a​:blah)foo/, but it is not clear what /(?a)(?​:foo|(?-a)blah)foo/
should do. None of this should be taken as me disagreeing with Karl
here, I am just trying to say that to me (?-a) /could/ have a sane
meaning given what (?a) means, but that to me (?-p) can't have a sane
meaning given what (?p) means.

--
perl -Mre=debug -e "/just|another|perl|hacker/"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant