Change in behaviour of "$foo::$bar" in 5.18 #15408

p5pRT · 2016-06-25T04:51:12Z

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

Searchable as RT128478$

p5pRT · 2016-06-25T04:51:13Z

From @cpansprout

In perl 5.16 and earlier, "$foo::$bar" parsed as:

$foo:: . $bar

In perl 5.18, it accidentally started being parsed as:

$foo . "::" . $bar

(I say accidentally, because commit 07f7264, which made the change, does not appear to have been made for this purpose, and its commit message does not even hint at this particular change in parsing.)

Now one module, Sub::Attribute (admittedly brand new and still at 0.01), has inadvertently started relying on the new behaviour. (See <https://rt.cpan.org/Ticket/Display.html?id=115570>.)

The *real* problem is that we now have a discrepancy between "$foo::$bar" and "$foo::@bar":

$ ./perl -Ilib -MO=Deparse,-q -e '"$foo::$bar"'
$foo . '::' . $bar;
-e syntax OK
$ ./perl -Ilib -MO=Deparse,-q -e '"$foo::@bar"'
$foo:: . join($", @bar);
-e syntax OK

So, what should we do about this? Should we (a) revert to the old behaviour? (Fixing that one module is easy enough.) Or should we (b) make "$foo::@bar" consistent with "$foo::$bar".

I prefer option a. Option b is harder, in that we have to decide where exactly to draw the line. Do we stop $foo:: from being interpolated at all? Do we only cut it off at $foo before $ and @? Etc.

The change, BTW, did not only affect double-quoted strings, but also print $foo::$bar:

$ perl5.14.4 -e 'print $foo::$bar'
Scalar found where operator expected at -e line 1, near "$foo::$bar"
(Missing operator before $bar?)
Can't use an undefined value as a symbol reference at -e line 1.

(The ‘Scalar found where operator expected’ is bogus. [I reported that in another ticket some years ago.] The fact that it does with ‘Can't use an undefined value’ shows that the program compiles and runs.)

$ perl5.18.1 -e 'print $foo::$bar'
Bad name after :: at -e line 1.

Now we have a very unhelpful error message. (The same unhelpful message you get for ‘foo::bar::$baz’, at least as far back as 5.8.7.)

Also, B::Deparse is wrong now:

$ ./perl -Ilib -MO=Deparse -e '"${foo::}$a"'
"$foo::$a";
-e syntax OK

--

Father Chrysostomos

p5pRT · 2016-06-27T04:53:21Z

From @cpansprout

So far no response....

On Fri Jun 24 21:51:13 2016, sprout wrote:

So, what should we do about this? Should we (a) revert to the old
behaviour? (Fixing that one module is easy enough.) Or should we (b)
make "$foo::@bar" consistent with "$foo::$bar".

I prefer option a.

And here is a patch for it. Should I apply it?

--

Father Chrysostomos

p5pRT · 2016-06-27T04:53:21Z

From @cpansprout

From b00321c Mon Sep 17 00:00:00 2001
From: Father Chrysostomos <sprout@cpan.org>
Date: Sun, 26 Jun 2016 21:45:22 -0700
Subject: [PATCH] [perl #128478] Restore former "$foo::$bar" parsing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The function scan_word, in toke.c, is used to parse barewords. The
scan_ident function is used to scan an identifier after a sigil.

Prior to v5.17.9-108-g07f7264, both functions had their own parsing
loops, and scan_ident actually had two, one for $foo and another
for ${foo}.

The state purpose of 07f7264 was to fix discrepancies in the parsing
of $foo vs ${foo}, by making the two forms use the same parsing code.
In accomplishing this, the commit in question merged not only the
two loops in scan_ident, but all three loops, including the one in
scan_word, by introducing a new function, parse_ident, that the
others call.

One result was that some logic appropriate only to scan_word started
to be applied also to scan_ident; namely, that ::$ would be explicitly
checked for and disallowed (the parsing would stop before the ::), for
the sake of the “Bad name after Foo::” error.

The consequence was that "$foo::$bar" started to be parsed as
$foo."::".$bar, instead of $foo:: . $bar, as previously.

Now, "$foo::@bar" was unaffected, so by fixing one form of inconsis-
tency we ended up form, including B::Deparse bugs (because B::Deparse
was not consistent with the core).

This commit restores the previous behaviour by giving parse_ident an
extra parameter, making the ::$ check optional.

Inline Patch

diff --git a/embed.fnc b/embed.fnc
index cd4b0ff..fe5eb68 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -2548,7 +2548,7 @@ s	|int	|deprecate_commaless_var_list
 s	|int	|ao		|int toketype
 s  |void|parse_ident|NN char **s|NN char **d \
                      |NN char * const e|int allow_package \
-                     |bool is_utf8
+				|bool is_utf8|bool check_dollar
 #  if defined(PERL_CR_FILTER)
 s	|I32	|cr_textfilter	|int idx|NULLOK SV *sv|int maxlen
 s	|void	|strip_return	|NN SV *sv
diff --git a/embed.h b/embed.h
index 84f647e..de5a590 100644
--- a/embed.h
+++ b/embed.h
@@ -1784,7 +1784,7 @@
 #define lop(a,b,c)		S_lop(aTHX_ a,b,c)
 #define missingterm(a)		S_missingterm(aTHX_ a)
 #define no_op(a,b)		S_no_op(aTHX_ a,b)
-#define parse_ident(a,b,c,d,e)	S_parse_ident(aTHX_ a,b,c,d,e)
+#define parse_ident(a,b,c,d,e,f)	S_parse_ident(aTHX_ a,b,c,d,e,f)
 #define pending_ident()		S_pending_ident(aTHX)
 #define scan_const(a)		S_scan_const(aTHX_ a)
 #define scan_formline(a)	S_scan_formline(aTHX_ a)
diff --git a/proto.h b/proto.h
index 42d78cb..9bdbac7 100644
--- a/proto.h
+++ b/proto.h
@@ -5496,7 +5496,7 @@ STATIC SV*	S_new_constant(pTHX_ const char *s, STRLEN len, const char *key, STRL
 STATIC void	S_no_op(pTHX_ const char *const what, char *s);
 #define PERL_ARGS_ASSERT_NO_OP	\
 	assert(what)
-STATIC void	S_parse_ident(pTHX_ char **s, char **d, char * const e, int allow_package, bool is_utf8);
+STATIC void	S_parse_ident(pTHX_ char **s, char **d, char * const e, int allow_package, bool is_utf8, bool check_dollar);
 #define PERL_ARGS_ASSERT_PARSE_IDENT	\
 	assert(s); assert(d); assert(e)
 STATIC int	S_pending_ident(pTHX);
diff --git a/t/base/lex.t b/t/base/lex.t
index fe46f14..4ac2b5b 100644
--- a/t/base/lex.t
+++ b/t/base/lex.t
@@ -1,6 +1,6 @@
 #!./perl
 
-print "1..105\n";
+print "1..107\n";
 
 $x = 'x';
 
@@ -528,3 +528,10 @@ eval q|s##[}#e|;
  eval ('/@0{0*->@*/*]');
  print "ok $test - 128171\n"; $test++;
 }
+
+$foo = "WRONG"; $foo:: = "bar"; $bar = "baz";
+print "not " unless "$foo::$bar" eq "barbaz";
+print qq|ok $test - [perl #128478] "\$foo::\$bar"\n|; $test++;
+@bar = ("baz","bonk");
+print "not " unless "$foo::@bar" eq "barbaz bonk";
+print qq|ok $test - [perl #128478] "\$foo::\@bar"\n|; $test ++;
diff --git a/toke.c b/toke.c
index 327d984..aebeebb 100644
--- a/toke.c
+++ b/toke.c
@@ -8819,7 +8819,8 @@ S_new_constant(pTHX_ const char *s, STRLEN len, const char *key, STRLEN keylen,
 }
 
 PERL_STATIC_INLINE void
-S_parse_ident(pTHX_ char **s, char **d, char * const e, int allow_package, bool is_utf8) {
+S_parse_ident(pTHX_ char **s, char **d, char * const e, int allow_package,
+                    bool is_utf8, bool check_dollar) {
     PERL_ARGS_ASSERT_PARSE_IDENT;
 
     for (;;) {
@@ -8855,7 +8856,7 @@ S_parse_ident(pTHX_ char **s, char **d, char * const e, int allow_package, bool
             * the code path that triggers the "Bad name after" warning
             * when looking for barewords.
             */
-           && (*s)[2] != '$') {
+           && !(check_dollar && (*s)[2] == '$')) {
             *(*d)++ = *(*s)++;
             *(*d)++ = *(*s)++;
         }
@@ -8877,7 +8878,7 @@ S_scan_word(pTHX_ char *s, char *dest, STRLEN destlen, int allow_package, STRLEN
 
     PERL_ARGS_ASSERT_SCAN_WORD;
 
-    parse_ident(&s, &d, e, allow_package, is_utf8);
+    parse_ident(&s, &d, e, allow_package, is_utf8, TRUE);
     *d = '\0';
     *slp = d - dest;
     return s;
@@ -8925,7 +8926,7 @@ S_scan_ident(pTHX_ char *s, char *dest, STRLEN destlen, I32 ck_uni)
 	}
     }
     else {  /* See if it is a "normal" identifier */
-        parse_ident(&s, &d, e, 1, is_utf8);
+        parse_ident(&s, &d, e, 1, is_utf8, FALSE);
     }
     *d = '\0';
     d = dest;
@@ -8994,7 +8995,7 @@ S_scan_ident(pTHX_ char *s, char *dest, STRLEN destlen, I32 ck_uni)
                (the later check for } being at the expected point will trap
                cases where this doesn't pan out.)  */
             d += is_utf8 ? UTF8SKIP(d) : 1;
-            parse_ident(&s, &d, e, 1, is_utf8);
+            parse_ident(&s, &d, e, 1, is_utf8, TRUE);
 	    *d = '\0';
             tmp_copline = CopLINE(PL_curcop);
             if (s < PL_bufend && isSPACE(*s)) {
@@ -11875,7 +11876,8 @@ S_parse_opt_lexvar(pTHX)
     s = PL_bufptr;
     d = PL_tokenbuf + 1;
     PL_tokenbuf[0] = (char)sigil;
-    parse_ident(&s, &d, PL_tokenbuf + sizeof(PL_tokenbuf) - 1, 0, cBOOL(UTF));
+    parse_ident(&s, &d, PL_tokenbuf + sizeof(PL_tokenbuf) - 1, 0,
+		cBOOL(UTF), FALSE);
     PL_bufptr = s;
     if (d == PL_tokenbuf+1)
 	return NULL;

p5pRT · 2016-06-27T04:53:21Z

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

p5pRT · 2016-06-27T07:09:29Z

From @Hugmeir

On 27 June 2016 at 06:53, Father Chrysostomos via RT
<perlbug-followup@perl.org> wrote:

So far no response....

On Fri Jun 24 21:51:13 2016, sprout wrote:

So, what should we do about this? Should we (a) revert to the old
behaviour? (Fixing that one module is easy enough.) Or should we (b)
make "$foo::@bar" consistent with "$foo::$bar".

I prefer option a.

And here is a patch for it. Should I apply it?

++

I entirely missed this when writing that commit; the fix looks spot-on to me.

p5pRT · 2016-06-27T13:33:51Z

From @cpansprout

On Mon Jun 27 00:09:29 2016, Hugmeir wrote:

On 27 June 2016 at 06:53, Father Chrysostomos via RT
<perlbug-followup@perl.org> wrote:

So far no response....

On Fri Jun 24 21:51:13 2016, sprout wrote:

So, what should we do about this? Should we (a) revert to the old
behaviour? (Fixing that one module is easy enough.) Or should we
(b)
make "$foo::@bar" consistent with "$foo::$bar".

I prefer option a.

And here is a patch for it. Should I apply it?

++

I entirely missed this when writing that commit; the fix looks spot-on
to me.

Thank you. I have pushed it as d9d2b74.

--

Father Chrysostomos

p5pRT · 2016-06-27T13:33:52Z

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

p5pRT · 2016-06-29T20:26:02Z

From @xsawyerx

On 06/27/2016 03:33 PM, Father Chrysostomos via RT wrote:

On Mon Jun 27 00:09:29 2016, Hugmeir wrote:

On 27 June 2016 at 06:53, Father Chrysostomos via RT
<perlbug-followup@perl.org> wrote:

So far no response....

On Fri Jun 24 21:51:13 2016, sprout wrote:

So, what should we do about this? Should we (a) revert to the old
behaviour? (Fixing that one module is easy enough.) Or should we
(b)
make "$foo::@bar" consistent with "$foo::$bar".

I prefer option a.
And here is a patch for it. Should I apply it?
++

I entirely missed this when writing that commit; the fix looks spot-on
to me.
Thank you. I have pushed it as d9d2b74.

I didn't get to respond, but mine was ++ as well. If we start with
making "$foo::@bar" do something else, we'll have to also add
"$foo::%bar", and then bikeshed any DWIM angle for it. Can of $something. :)

Thank you.

p5pRT · 2016-06-30T05:54:13Z

From @andk

On Mon, 27 Jun 2016 06:33:51 -0700, "Father Chrysostomos via RT" <perlbug-followup@perl.org> said:

> Thank you. I have pushed it as d9d2b74.

Since this ticket is not closed yet, we probably need no BBC ticket.

Please look into http://matrix.cpantesters.org/?dist=Tie-SecureHash-1.10

Does the report of my smoker
http://www.cpantesters.org/cpan/report/7d363fa4-3dc0-11e6-b130-d1b85bc2a771
match your expectations?

--
andreas

p5pRT · 2016-06-30T22:27:41Z

From @cpansprout

On Wed Jun 29 22:54:13 2016, andreas.koenig.7os6VVqR@franz.ak.mind.de wrote:

On Mon, 27 Jun 2016 06:33:51 -0700, "Father Chrysostomos via RT"
<perlbug-followup@perl.org> said:

Thank you. I have pushed it as d9d2b74.

Since this ticket is not closed yet, we probably need no BBC ticket.

Please look into http://matrix.cpantesters.org/?dist=Tie-SecureHash-
1.10

Does the report of my smoker
http://www.cpantesters.org/cpan/report/7d363fa4-3dc0-11e6-b130-
d1b85bc2a771
match your expectations?

Yes, it does. I have filed a ticket at <https://rt.cpan.org/Ticket/Display.html?id=115772>.

--

Father Chrysostomos

p5pRT · 2017-05-30T20:14:09Z

From @khwilliamson

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

With the release today of Perl 5.26.0, this and 210 other issues have been
resolved.

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

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

p5pRT · 2017-05-30T20:14:09Z

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

p5pRT closed this as completed May 30, 2017

p5pRT added Severity Low distro-All type-parsing labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change in behaviour of "$foo::$bar" in 5.18 #15408

Change in behaviour of "$foo::$bar" in 5.18 #15408

p5pRT commented Jun 25, 2016

p5pRT commented Jun 25, 2016

p5pRT commented Jun 27, 2016

p5pRT commented Jun 27, 2016

p5pRT commented Jun 27, 2016

p5pRT commented Jun 27, 2016

p5pRT commented Jun 27, 2016

p5pRT commented Jun 27, 2016

p5pRT commented Jun 29, 2016

p5pRT commented Jun 30, 2016

p5pRT commented Jun 30, 2016

p5pRT commented May 30, 2017

p5pRT commented May 30, 2017

Change in behaviour of "$foo::$bar" in 5.18 #15408

Change in behaviour of "$foo::$bar" in 5.18 #15408

Comments

p5pRT commented Jun 25, 2016

p5pRT commented Jun 25, 2016

From @cpansprout

p5pRT commented Jun 27, 2016

From @cpansprout

p5pRT commented Jun 27, 2016

From @cpansprout

p5pRT commented Jun 27, 2016

p5pRT commented Jun 27, 2016

From @Hugmeir

p5pRT commented Jun 27, 2016

From @cpansprout

p5pRT commented Jun 27, 2016

p5pRT commented Jun 29, 2016

From @xsawyerx

p5pRT commented Jun 30, 2016

From @andk

p5pRT commented Jun 30, 2016

From @cpansprout

p5pRT commented May 30, 2017

From @khwilliamson

p5pRT commented May 30, 2017