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

Empty pattern-match modifiers should be documented #16413

Closed
p5pRT opened this issue Feb 11, 2018 · 18 comments
Closed

Empty pattern-match modifiers should be documented #16413

p5pRT opened this issue Feb 11, 2018 · 18 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 11, 2018

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

Searchable as RT132851$

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2018

From @choroba

Created by @choroba

"perlre" introduces the (?) construct with

  Zero or more embedded pattern-match modifiers, to be turned on (or
  turned off...)

But, in fact, it's possible to specify an empty list of modifiers.

See http​://www.perlmonks.org/?node_id=1208872 for more details.

Patch to the documentation attached.

Perl Info

Flags:
     category=docs
     severity=low

Site configuration information for perl 5.27.9:

Configured by choroba at Sun Feb 11 00:04:23 CET 2018.

Summary of my perl5 (revision 5 version 27 subversion 9) configuration:
   Commit id: 199fc8cde4ec30a56626c0765b3b0efacb327664
   Platform:
     osname=linux
     osvers=4.4.114-42-default
     archname=x86_64-linux-thread-multi
     uname='linux pilgrim 4.4.114-42-default #1 smp tue feb 6 10:58:10 utc 2018 (b6ee9ae) x86_64 x86_64 x86_64 gnulinux '
     config_args='-des -Dusethreads -Dprefix=/home/choroba/blead -Dusedevel'
     hint=recommended
     useposix=true
     d_sigaction=define
     useithreads=define
     usemultiplicity=define
     use64bitint=define
     use64bitall=define
     uselongdouble=undef
     usemymalloc=n
     default_inc_excludes_dot=define
     bincompat5005=undef
   Compiler:
     cc='cc'
     ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
     optimize='-O2'
     cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
     ccversion=''
     gccversion='4.8.5'
     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 -L/usr/local/lib'
     libpth=/usr/local/lib /usr/lib64/gcc/x86_64-suse-linux/4.8/include-fixed /usr/lib64/gcc/x86_64-suse-linux/4.8/../../../../x86_64-suse-linux/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib /lib64 /usr/lib64 /usr/local/lib64
     libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
     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 -O2 -L/usr/local/lib -fstack-protector'



@INC for perl 5.27.9:
     /home/choroba/blead/lib/perl5/site_perl/5.27.9/x86_64-linux-thread-multi
     /home/choroba/blead/lib/perl5/site_perl/5.27.9
     /home/choroba/blead/lib/perl5/5.27.9/x86_64-linux-thread-multi
     /home/choroba/blead/lib/perl5/5.27.9


Environment for perl 5.27.9:
     HOME=/home/choroba
     LANG=en_US.utf8
     LANGUAGE (unset)
     LC_CTYPE=en_US.UTF-8
     LD_LIBRARY_PATH (unset)
     LOGDIR (unset)
     PATH=/home/choroba/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/home/choroba/perl5/bin:.
     PERL_BADLANG (unset)
     SHELL=/bin/bash


@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2018

From @choroba

diff --git a/pod/perlre.pod b/pod/perlre.pod
index 74f44fedc3..bd1e226bd5 100644
--- a/pod/perlre.pod
+++ b/pod/perlre.pod
@​@​ -1369,7 +1369,7 @​@​ an escape sequence. Examples​:
=item C<(?^alupimnsx)>
X<(?)> X<(?^)>

-One or more embedded pattern-match modifiers, to be turned on (or
+Zero or more embedded pattern-match modifiers, to be turned on (or
turned off if preceded by C<"-">) for the remainder of the pattern or
the remainder of the enclosing pattern group (if any).

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2018

From @jkeenan

On Sun, 11 Feb 2018 00​:30​:52 GMT, choroba@​cpan.org wrote​:

This is a bug report for perl from choroba@​cpan.org,
generated with the help of perlbug 1.41 running under perl 5.27.9.

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

"perlre" introduces the (?) construct with

Zero or more embedded pattern-match modifiers, to be turned on (or
turned off...)

But, in fact, it's possible to specify an empty list of modifiers.

See http​://www.perlmonks.org/?node_id=1208872 for more details.

Patch to the documentation attached.

Assuming that this behavior is acceptable and should be documented as such, we should probably also add a unit test of the behavior.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2018

From @demerphq

How are zero modifiers different from the empty list? Seems like this
documented already.

On 11 Feb 2018 02​:13, "James E Keenan via RT" <perlbug-followup@​perl.org>
wrote​:

On Sun, 11 Feb 2018 00​:30​:52 GMT, choroba@​cpan.org wrote​:

This is a bug report for perl from choroba@​cpan.org,
generated with the help of perlbug 1.41 running under perl 5.27.9.

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

"perlre" introduces the (?) construct with

Zero or more embedded pattern-match modifiers, to be turned on (or
turned off...)

But, in fact, it's possible to specify an empty list of modifiers.

See http​://www.perlmonks.org/?node_id=1208872 for more details.

Patch to the documentation attached.

Assuming that this behavior is acceptable and should be documented as
such, we should probably also add a unit test of the behavior.

--
James E Keenan (jkeenan@​cpan.org)

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

@p5pRT
Copy link
Author

p5pRT commented Feb 12, 2018

From @Smylers

demerphq writes​:

How are zero modifiers different from the empty list? Seems like this
documented already.

perlre in blead currently says “One or more”. The patch in this ticket
is to change “One” to “Zero”.

I suspect that the reporter inadvertently copied the ‘current’ wording
with their patch already applied, thereby making it look like this is
already documented, when actually it isn't.

Smylers

@p5pRT
Copy link
Author

p5pRT commented Feb 12, 2018

From @demerphq

On 12 Feb 2018 20​:46, "Smylers" <smylers@​stripey.com> wrote​:

demerphq writes​:

How are zero modifiers different from the empty list? Seems like this
documented already.

perlre in blead currently says “One or more”. The patch in this ticket
is to change “One” to “Zero”.

I suspect that the reporter inadvertently copied the ‘current’ wording
with their patch already applied, thereby making it look like this is
already documented, when actually it isn't.

Ah. Good catch. Well then let's fix the code here, not the doc.

Yves

@p5pRT
Copy link
Author

p5pRT commented Feb 13, 2018

From @demerphq

On 12 February 2018 at 15​:00, demerphq <demerphq@​gmail.com> wrote​:

On 12 Feb 2018 20​:46, "Smylers" <smylers@​stripey.com> wrote​:

demerphq writes​:

How are zero modifiers different from the empty list? Seems like this
documented already.

perlre in blead currently says “One or more”. The patch in this ticket
is to change “One” to “Zero”.

I suspect that the reporter inadvertently copied the ‘current’ wording
with their patch already applied, thereby making it look like this is
already documented, when actually it isn't.

Ah. Good catch. Well then let's fix the code here, not the doc.

For example​:

$ git diff

Inline Patch
diff --git a/regcomp.c b/regcomp.c
index 9d19d3e..49f1b71 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -10889,7 +10889,9 @@ S_reg(pTHX_ RExC_state_t *pRExC_state, I32
paren, I32 *flagp,U32 depth)   \}   ret = NULL; /\* For look\-ahead/behind\. \*/   switch \(paren\) \{ \- \+ case '\)'​: \+ vFAIL\("\(?\) without any modifiers is illegal\."\); \+ break;   case 'P'​: /\* \(?P\.\.\.\) variants for those used to PCRE/Python \*/   paren = \*RExC\_parse;   if \( paren == '\<'\) \{ /\* \(?P\<\.\.\.>\) named capture \*/

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

@p5pRT
Copy link
Author

p5pRT commented Feb 13, 2018

From @iabyn

On Tue, Feb 13, 2018 at 05​:08​:46AM +0100, demerphq wrote​:

Ah. Good catch. Well then let's fix the code here, not the doc.

+ case ')'​:
+ vFAIL("(?) without any modifiers is illegal.");
+ break;

I think the point being made in the original perlmonks post was that
perl *should* support zero flags, because as our docs say​:

  This is particularly useful for dynamic patterns, such as those read
  in from a configuration file, taken from an argument, or specified in
  a table somewhere.

And "dynamically generated" can easily result in zero flags. So the OP
wanted to clarify whether it was safe to assume that zero flags was
supported.

--
If life gives you lemons, you'll probably develop a citric acid allergy.

@p5pRT
Copy link
Author

p5pRT commented Feb 13, 2018

From @demerphq

On 13 Feb 2018 16​:59, "Dave Mitchell" <davem@​iabyn.com> wrote​:

On Tue, Feb 13, 2018 at 05​:08​:46AM +0100, demerphq wrote​:

Ah. Good catch. Well then let's fix the code here, not the doc.

+ case ')'​:
+ vFAIL("(?) without any modifiers is illegal.");
+ break;

I think the point being made in the original perlmonks post was that
perl *should* support zero flags, because as our docs say​:

  This is particularly useful for dynamic patterns, such as those read
  in from a configuration file, taken from an argument, or specified in
  a table somewhere.

And "dynamically generated" can easily result in zero flags. So the OP
wanted to clarify whether it was safe to assume that zero flags was
supported.

I'm fine either way but for the record I was thinking it should be
forbidden so as to catch typos.

Yved

@p5pRT
Copy link
Author

p5pRT commented Mar 14, 2018

From @khwilliamson

On Tue, 13 Feb 2018 03​:09​:29 -0800, demerphq wrote​:

On 13 Feb 2018 16​:59, "Dave Mitchell" <davem@​iabyn.com> wrote​:

On Tue, Feb 13, 2018 at 05​:08​:46AM +0100, demerphq wrote​:

Ah. Good catch. Well then let's fix the code here, not the doc.

+ case ')'​:
+ vFAIL("(?) without any modifiers is illegal.");
+ break;

I think the point being made in the original perlmonks post was that
perl *should* support zero flags, because as our docs say​:

 This is particularly useful for dynamic patterns\, such as those read
 in from a configuration file\, taken from an argument\, or specified in
 a table somewhere\.

And "dynamically generated" can easily result in zero flags. So the OP
wanted to clarify whether it was safe to assume that zero flags was
supported.

I'm fine either way but for the record I was thinking it should be
forbidden so as to catch typos.

Yved

I'm in favor of catching typos (such as the one in the line above :) )
I see two other alternatives. One is to warn instead of die, and the other is
to warn or die only under 'use re "strict"'
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 14, 2018

From @cpansprout

On Wed, 14 Mar 2018 09​:21​:38 -0700, khw wrote​:

On Tue, 13 Feb 2018 03​:09​:29 -0800, demerphq wrote​:

On 13 Feb 2018 16​:59, "Dave Mitchell" <davem@​iabyn.com> wrote​:

On Tue, Feb 13, 2018 at 05​:08​:46AM +0100, demerphq wrote​:

Ah. Good catch. Well then let's fix the code here, not the doc.

+ case ')'​:
+ vFAIL("(?) without any modifiers is illegal.");
+ break;

I think the point being made in the original perlmonks post was that
perl *should* support zero flags, because as our docs say​:

This is particularly useful for dynamic patterns, such as those read
in from a configuration file, taken from an argument, or specified in
a table somewhere.

And "dynamically generated" can easily result in zero flags. So the
OP
wanted to clarify whether it was safe to assume that zero flags was
supported.

I'm fine either way but for the record I was thinking it should be
forbidden so as to catch typos.

Yved

I'm in favor of catching typos (such as the one in the line above :) )
I see two other alternatives. One is to warn instead of die, and the
other is
to warn or die only under 'use re "strict"'

I would prefer that we keep it loose. It is longstanding behaviour, and fixing it provides little gain. (How likely is someone to type (?) by mistake without noticing it? If the ) was added by mistake the pattern won’t compile.)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Mar 14, 2018

From @demerphq

On 14 March 2018 at 17​:45, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Wed, 14 Mar 2018 09​:21​:38 -0700, khw wrote​:

On Tue, 13 Feb 2018 03​:09​:29 -0800, demerphq wrote​:

On 13 Feb 2018 16​:59, "Dave Mitchell" <davem@​iabyn.com> wrote​:

On Tue, Feb 13, 2018 at 05​:08​:46AM +0100, demerphq wrote​:

Ah. Good catch. Well then let's fix the code here, not the doc.

+ case ')'​:
+ vFAIL("(?) without any modifiers is illegal.");
+ break;

I think the point being made in the original perlmonks post was that
perl *should* support zero flags, because as our docs say​:

This is particularly useful for dynamic patterns, such as those read
in from a configuration file, taken from an argument, or specified in
a table somewhere.

And "dynamically generated" can easily result in zero flags. So the
OP
wanted to clarify whether it was safe to assume that zero flags was
supported.

I'm fine either way but for the record I was thinking it should be
forbidden so as to catch typos.

Yved

I'm in favor of catching typos (such as the one in the line above :) )
I see two other alternatives. One is to warn instead of die, and the
other is
to warn or die only under 'use re "strict"'

I would prefer that we keep it loose. It is longstanding behaviour, and fixing it provides little gain. (How likely is someone to type (?) by mistake without noticing it? If the ) was added by mistake the pattern won’t compile.)

FWIW, I am not convinced. I can easily someone making this mistake,
and then adding a new open paren to deal with it

Yves

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

@p5pRT
Copy link
Author

p5pRT commented Apr 17, 2018

From @khwilliamson

On Wed, 14 Mar 2018 10​:02​:34 -0700, demerphq wrote​:

On 14 March 2018 at 17​:45, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Wed, 14 Mar 2018 09​:21​:38 -0700, khw wrote​:

On Tue, 13 Feb 2018 03​:09​:29 -0800, demerphq wrote​:

On 13 Feb 2018 16​:59, "Dave Mitchell" <davem@​iabyn.com> wrote​:

On Tue, Feb 13, 2018 at 05​:08​:46AM +0100, demerphq wrote​:

Ah. Good catch. Well then let's fix the code here, not the
doc.

+ case ')'​:
+ vFAIL("(?) without any modifiers is illegal.");
+ break;

I think the point being made in the original perlmonks post was
that
perl *should* support zero flags, because as our docs say​:

This is particularly useful for dynamic patterns, such as those
read
in from a configuration file, taken from an argument, or specified
in
a table somewhere.

And "dynamically generated" can easily result in zero flags. So
the
OP
wanted to clarify whether it was safe to assume that zero flags
was
supported.

I'm fine either way but for the record I was thinking it should be
forbidden so as to catch typos.

Yved

I'm in favor of catching typos (such as the one in the line above :)
)
I see two other alternatives. One is to warn instead of die, and
the
other is
to warn or die only under 'use re "strict"'

I would prefer that we keep it loose. It is longstanding behaviour,
and fixing it provides little gain. (How likely is someone to type
(?) by mistake without noticing it? If the ) was added by mistake
the pattern won’t compile.)

FWIW, I am not convinced. I can easily someone making this mistake,
and then adding a new open paren to deal with it

Yves

Discussion here has stalled. Another option is to document and warn, but only under 're strict'. Then longstanding code will be unaffected, but there is a way for someone to find these.
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 23, 2019

From @khwilliamson

Given that 11 months have passed without comment on my latest proposal, I implemented it by applying E Choroba's patch, and making an empty (?) warn under re 'strict', and documenting that, adding a test in
d9a9148
a785e2a
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 23, 2019

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

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0, this and 160 other issues have been
resolved.

Perl 5.30.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2019

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

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