Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.98k stars 559 forks source link

Eliminate modifiable variables in constants #17020

Closed p5pRT closed 5 years ago

p5pRT commented 5 years ago

Migrated from rt.perl.org#134138 (status was 'pending release')

Searchable as RT134138$

p5pRT commented 5 years ago

From @jkeenan

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

p5pRT commented 5 years ago

From @jkeenan

Summary of my perl5 (revision 5 version 31 subversion 0) configuration​:   Commit id​: 58f4626762668e1c1948832073998af84b2c85d0   Platform​:   osname=linux   osvers=4.15.0-50-generic   archname=x86_64-linux   uname='linux zareason 4.15.0-50-generic #54-ubuntu smp mon may 6 18​:46​:08 utc 2019 x86_64 x86_64 x86_64 gnulinux '   config_args='-des -Dusedevel -Dusemymalloc=y'   hint=recommended   useposix=true   d_sigaction=define   useithreads=undef   usemultiplicity=undef   use64bitint=define   use64bitall=define   uselongdouble=undef   usemymalloc=y   default_inc_excludes_dot=define   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='7.4.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-linux-gnu/7/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /lib64 /usr/lib64   libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc   perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc   libc=libc-2.27.so   so=so   useshrplib=false   libperl=libperl.a   gnulibc_version='2.27'   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'

Characteristics of this binary (from libperl)​:   Compile-time options​:   HAS_TIMES   MYMALLOC   PERLIO_LAYERS   PERL_COPY_ON_WRITE   PERL_DONT_CREATE_GVSV   PERL_MALLOC_WRAP   PERL_OP_PARENT   PERL_PRESERVE_IVUV   PERL_USE_DEVEL   USE_64_BIT_ALL   USE_64_BIT_INT   USE_LARGE_FILES   USE_LOCALE   USE_LOCALE_COLLATE   USE_LOCALE_CTYPE   USE_LOCALE_NUMERIC   USE_LOCALE_TIME   USE_PERLIO   USE_PERL_ATOF   Built under linux   Compiled at May 22 2019 14​:58​:04   %ENV​:   PERL2DIR="/home/jkeenan/gitwork/perl2"   PERLBREW_HOME="/home/jkeenan/.perlbrew"   PERLBREW_MANPATH="/home/jkeenan/perl5/perlbrew/perls/perl-5.28.0/man"   PERLBREW_PATH="/home/jkeenan/perl5/perlbrew/bin​:/home/jkeenan/perl5/perlbrew/perls/perl-5.28.0/bin"   PERLBREW_PERL="perl-5.28.0"   PERLBREW_ROOT="/home/jkeenan/perl5/perlbrew"   PERLBREW_SHELLRC_VERSION="0.84"   PERLBREW_VERSION="0.84"   PERL_WORKDIR="/home/jkeenan/gitwork/perl"   @​INC​:   lib   /usr/local/lib/perl5/site_perl/5.31.0/x86_64-linux   /usr/local/lib/perl5/site_perl/5.31.0   /usr/local/lib/perl5/5.31.0/x86_64-linux   /usr/local/lib/perl5/5.31.0

p5pRT commented 5 years ago

From @jkeenan

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much. -- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 5 years ago

From @jkeenan

0001-Eliminate-modifiable-variables-in-constants.patch ```diff From 083d8eb22b2e6884eb8d99b7677d3ff4b0f1f5f6 Mon Sep 17 00:00:00 2001 From: James E Keenan Date: Sat, 25 May 2019 21:40:00 -0400 Subject: [PATCH] Eliminate modifiable variables in constants Transform previously deprecated cases into exceptions. Update diagnostic; change D to F For: RT 134138 --- pad.c | 8 +------- pod/perldiag.pod | 7 +++---- t/op/const-optree.t | 44 ++++++++++++++++++-------------------------- 3 files changed, 22 insertions(+), 37 deletions(-) diff --git a/pad.c b/pad.c index f73fc550f9..f8ccea5ec2 100644 --- a/pad.c +++ b/pad.c @@ -2156,13 +2156,7 @@ S_cv_clone_pad(pTHX_ CV *proto, CV *cv, CV *outside, HV *cloned, ) == o && !OpSIBLING(o)) { - Perl_ck_warner_d(aTHX_ - packWARN(WARN_DEPRECATED), - "Constants from lexical " - "variables potentially " - "modified elsewhere are " - "deprecated. This will not " - "be allowed in Perl 5.32"); + Perl_croak(aTHX_ "Constants from lexical variables potentially modified elsewhere are no longer permitted"); /* We *copy* the lexical variable, and donate the copy to newCONSTSUB. Yes, this is ugly, and should be killed. We need to do this for the diff --git a/pod/perldiag.pod b/pod/perldiag.pod index f69b1b8367..b05ab6fd8c 100644 --- a/pod/perldiag.pod +++ b/pod/perldiag.pod @@ -1833,10 +1833,9 @@ The message indicates the type of reference that was expected. This usually indicates a syntax error in dereferencing the constant value. See L and L. -=item Constants from lexical variables potentially modified elsewhere are -deprecated. This will not be allowed in Perl 5.32 +=item Constants from lexical variables potentially modified elsewhere are no longer permitted -(D deprecated) You wrote something like +(F) You wrote something like my $var; $sub = sub () { $var }; @@ -1853,7 +1852,7 @@ breaks the behavior of closures, in which the subroutine captures the variable itself, rather than its value, so future changes to the variable are reflected in the subroutine's return value. -This usage is deprecated, and will no longer be allowed in Perl 5.32, +This usage was deprecated, and as of Perl 5.32 is no longer allowed, making it possible to change the behavior in the future. If you intended for the subroutine to be eligible for inlining, then diff --git a/t/op/const-optree.t b/t/op/const-optree.t index 4d897d247e..3a8181beb8 100644 --- a/t/op/const-optree.t +++ b/t/op/const-optree.t @@ -8,7 +8,7 @@ BEGIN { require './test.pl'; set_up_inc('../lib'); } -plan 168; +plan 148; # @tests is an array of hash refs, each of which can have various keys: # @@ -25,6 +25,11 @@ plan 168; # deprecated - whether the sub returning a code ref will emit a depreca- # tion warning when called # method - whether the sub has the :method attribute +# exception - sub now throws an exception (previously threw +# deprecation warning) + +my $exception_134138 = 'Constants from lexical variables potentially modified ' + . 'elsewhere are no longer permitted'; # [perl #63540] Don’t treat sub { if(){.....}; "constant" } as a constant sub blonk { ++$blonk_was_called } @@ -47,11 +52,7 @@ push @tests, { push @tests, { nickname => 'sub with simple lexical modified elsewhere', generator => sub { my $x = 5; my $ret = sub(){$x}; $x = 7; $ret }, - retval => 5, # change to 7 when the deprecation cycle is over - same_retval => 0, - inlinable => 1, - deprecated => 1, - method => 0, + exception => $exception_134138, }; push @tests, { @@ -184,11 +185,7 @@ push @tests, { my $sub1 = sub () { $x++ }; $ret; }, - retval => 5, - same_retval => 0, - inlinable => 1, - deprecated => 1, - method => 0, + exception => $exception_134138, }; push @tests, { nickname => 'complex lexical op tree before an lvalue closure', @@ -307,11 +304,7 @@ push @tests, { eval '$outer++'; $ret; }, - retval => 43, - same_retval => 0, - inlinable => 1, - deprecated => 1, - method => 0, + exception => $exception_134138, }; push @tests, { nickname => 'sub () { $x } with s///ee in scope', @@ -322,11 +315,7 @@ push @tests, { $dummy =~ s//$dummy/ee; $ret; }, - retval => 43, - same_retval => 0, - inlinable => 1, - deprecated => 1, - method => 0, + exception => $exception_134138, }; push @tests, { nickname => 'sub () { $x } with eval not in scope', @@ -414,11 +403,7 @@ push @tests, { push @tests, { nickname => 'sub closing over state var++', generator => sub { state $x++; sub () { $x } }, - retval => 1, - same_retval => 0, - inlinable => 1, - deprecated => 1, - method => 0, + exception => $exception_134138, }; @@ -426,6 +411,12 @@ use feature 'refaliasing'; no warnings 'experimental::refaliasing'; for \%_ (@tests) { my $nickname = $_{nickname}; + if (exists $_{exception} and $_{exception}) { + local $@; + eval { my $sub = &{$_{generator}}; }; + like($@, qr/$_{exception}/, "$nickname: now throws exception (RT 134138)"); + next; + } my $w; local $SIG{__WARN__} = sub { $w = shift }; my $sub = &{$_{generator}}; @@ -492,3 +483,4 @@ pass("No assertion failure when turning on PADSTALE on lexical shared by" $z = &$sub; is $z, $y, 'inlinable sub ret vals are not swipable'; } + -- 2.17.1 ```
p5pRT commented 5 years ago

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

p5pRT commented 5 years ago

From @tonycoz

On Sat\, 25 May 2019 19​:30​:56 -0700\, jkeenan wrote​:

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much.

I'm not sure this is the desired behaviour\, though maybe we do want it fatal for a release or two to ensure any code that depends on the old behaviour is updated.

As to your patch\, you've left code in after the croak() that will no longer be executed (and since copied can no longer be true\, some other code can be removed.)

So there's two issues​:

a) should sub () { $x } return the current value of $x or croak at compile-time

b) if we croak at compile-time\, the patch needs work

Tony

p5pRT commented 5 years ago

From @jkeenan

On Wed\, 05 Jun 2019 01​:10​:06 GMT\, tonyc wrote​:

On Sat\, 25 May 2019 19​:30​:56 -0700\, jkeenan wrote​:

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much.

I'm not sure this is the desired behaviour\, though maybe we do want it fatal for a release or two to ensure any code that depends on the old behaviour is updated.

As to your patch\, you've left code in after the croak() that will no longer be executed (and since copied can no longer be true\, some other code can be removed.)

So there's two issues​:

a) should sub () { $x } return the current value of $x or croak at compile-time

b) if we croak at compile-time\, the patch needs work

Well\, I was just coding to see if I could change the deprecation to a fatalization. I don't really understand the code\, so we don't have to go forward with this patch.

But with (a) you seem to be suggesting that we need a better understanding of what we *should* be doing. What would that be?

Thank you very much.

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

p5pRT commented 5 years ago

From @tonycoz

On Tue\, 04 Jun 2019 18​:22​:38 -0700\, jkeenan wrote​:

On Wed\, 05 Jun 2019 01​:10​:06 GMT\, tonyc wrote​:

On Sat\, 25 May 2019 19​:30​:56 -0700\, jkeenan wrote​:

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much.

I'm not sure this is the desired behaviour\, though maybe we do want it fatal for a release or two to ensure any code that depends on the old behaviour is updated.

As to your patch\, you've left code in after the croak() that will no longer be executed (and since copied can no longer be true\, some other code can be removed.)

So there's two issues​:

a) should sub () { $x } return the current value of $x or croak at compile-time

b) if we croak at compile-time\, the patch needs work

Well\, I was just coding to see if I could change the deprecation to a fatalization. I don't really understand the code\, so we don't have to go forward with this patch.

But with (a) you seem to be suggesting that we need a better understanding of what we *should* be doing. What would that be?

I expect making it fatal will be the right choice at least for now.

Making 'sub () {$x}' behave as the code reads may break code that expects the old behaviour where the warnings have been suppressed or possibly only seen by end users.

Making it fatal (especially at compile-time\, as we do) will prevent that.

But the const-ifying behaviour has been deprecated for a while now.

I don't know what the intent was when it was originally deprecated.

Tony

p5pRT commented 5 years ago

From @jkeenan

On Wed\, 05 Jun 2019 01​:35​:47 GMT\, tonyc wrote​:

On Tue\, 04 Jun 2019 18​:22​:38 -0700\, jkeenan wrote​:

On Wed\, 05 Jun 2019 01​:10​:06 GMT\, tonyc wrote​:

On Sat\, 25 May 2019 19​:30​:56 -0700\, jkeenan wrote​:

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much.

I'm not sure this is the desired behaviour\, though maybe we do want it fatal for a release or two to ensure any code that depends on the old behaviour is updated.

As to your patch\, you've left code in after the croak() that will no longer be executed (and since copied can no longer be true\, some other code can be removed.)

So there's two issues​:

a) should sub () { $x } return the current value of $x or croak at compile-time

b) if we croak at compile-time\, the patch needs work

Well\, I was just coding to see if I could change the deprecation to a fatalization. I don't really understand the code\, so we don't have to go forward with this patch.

But with (a) you seem to be suggesting that we need a better understanding of what we *should* be doing. What would that be?

I expect making it fatal will be the right choice at least for now.

Making 'sub () {$x}' behave as the code reads may break code that expects the old behaviour where the warnings have been suppressed or possibly only seen by end users.

Making it fatal (especially at compile-time\, as we do) will prevent that.

But the const-ifying behaviour has been deprecated for a while now.

I don't know what the intent was when it was originally deprecated.

Tony

Can we reformulate the "ask" in this ticket so that we know what we want to deliver in perl-5.32?

Thank you very much. -- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 5 years ago

From @tonycoz

On Fri\, 09 Aug 2019 14​:29​:51 -0700\, jkeenan wrote​:

On Wed\, 05 Jun 2019 01​:35​:47 GMT\, tonyc wrote​:

On Tue\, 04 Jun 2019 18​:22​:38 -0700\, jkeenan wrote​:

On Wed\, 05 Jun 2019 01​:10​:06 GMT\, tonyc wrote​:

On Sat\, 25 May 2019 19​:30​:56 -0700\, jkeenan wrote​:

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much.

I'm not sure this is the desired behaviour\, though maybe we do want it fatal for a release or two to ensure any code that depends on the old behaviour is updated.

As to your patch\, you've left code in after the croak() that will no longer be executed (and since copied can no longer be true\, some other code can be removed.)

So there's two issues​:

a) should sub () { $x } return the current value of $x or croak at compile-time

b) if we croak at compile-time\, the patch needs work

Well\, I was just coding to see if I could change the deprecation to a fatalization. I don't really understand the code\, so we don't have to go forward with this patch.

But with (a) you seem to be suggesting that we need a better understanding of what we *should* be doing. What would that be?

I expect making it fatal will be the right choice at least for now.

Making 'sub () {$x}' behave as the code reads may break code that expects the old behaviour where the warnings have been suppressed or possibly only seen by end users.

Making it fatal (especially at compile-time\, as we do) will prevent that.

But the const-ifying behaviour has been deprecated for a while now.

I don't know what the intent was when it was originally deprecated.

Tony

Can we reformulate the "ask" in this ticket so that we know what we want to deliver in perl-5.32?

Thank you very much.

Right now I'm inclined to make it fatal.

I don't know whether it should remain so forever.

The attached applies on top of your patch to clean up the no longer needed code and re-wrap the croak.

Tony

p5pRT commented 5 years ago

From @tonycoz

0002-remove-now-irrelevant-code-and-reflow-the-error-mess.patch ```diff From b99017d3e3ae6fd39547550323ef159b1d9bede1 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Sat, 10 Aug 2019 15:15:04 +1000 Subject: remove now irrelevant code and reflow the error message --- pad.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/pad.c b/pad.c index f6f0e78341..7854678928 100644 --- a/pad.c +++ b/pad.c @@ -2127,7 +2127,6 @@ S_cv_clone_pad(pTHX_ CV *proto, CV *cv, CV *outside, HV *cloned, * from the parent */ if (const_sv && SvREFCNT(const_sv) == 2) { const bool was_method = cBOOL(CvMETHOD(cv)); - bool copied = FALSE; if (outside) { PADNAME * const pn = PadlistNAMESARRAY(CvPADLIST(outside)) @@ -2156,22 +2155,15 @@ S_cv_clone_pad(pTHX_ CV *proto, CV *cv, CV *outside, HV *cloned, ) == o && !OpSIBLING(o)) { - Perl_croak(aTHX_ "Constants from lexical variables potentially modified elsewhere are no longer permitted"); - /* We *copy* the lexical variable, and donate the - copy to newCONSTSUB. Yes, this is ugly, and - should be killed. We need to do this for the - time being, however, because turning on SvPADTMP - on a lexical will have observable effects - elsewhere. */ - const_sv = newSVsv(const_sv); - copied = TRUE; + Perl_croak(aTHX_ + "Constants from lexical variables potentially modified " + "elsewhere are no longer permitted"); } else goto constoff; } } - if (!copied) - SvREFCNT_inc_simple_void_NN(const_sv); + SvREFCNT_inc_simple_void_NN(const_sv); /* If the lexical is not used elsewhere, it is safe to turn on SvPADTMP, since it is only when it is used in lvalue con- text that the difference is observable. */ -- 2.11.0 ```
p5pRT commented 5 years ago

From @jkeenan

On Sat\, 10 Aug 2019 05​:23​:47 GMT\, tonyc wrote​:

On Fri\, 09 Aug 2019 14​:29​:51 -0700\, jkeenan wrote​:

On Wed\, 05 Jun 2019 01​:35​:47 GMT\, tonyc wrote​:

On Tue\, 04 Jun 2019 18​:22​:38 -0700\, jkeenan wrote​:

On Wed\, 05 Jun 2019 01​:10​:06 GMT\, tonyc wrote​:

On Sat\, 25 May 2019 19​:30​:56 -0700\, jkeenan wrote​:

On Sat\, 25 May 2019 02​:03​:40 GMT\, jkeenan@​pobox.com wrote​:

pod/perldeprecation.pod contains the following entry​:

##### Constants from lexical variables potentially modified elsewhere

You wrote something like

my $var; $sub = sub () { $var };

but $var is referenced elsewhere and could be modified after the "sub" expression is evaluated. Either it is explicitly modified elsewhere ("$var = 3") or it is passed to a subroutine or to an operator like "printf" or "map"\, which may or may not modify the variable.

Traditionally\, Perl has captured the value of the variable at that point and turned the subroutine into a constant eligible for inlining. In those cases where the variable can be modified elsewhere\, this breaks the behavior of closures\, in which the subroutine captures the variable itself\, rather than its value\, so future changes to the variable are reflected in the subroutine's return value.

If you intended for the subroutine to be eligible for inlining\, then make sure the variable is not referenced elsewhere\, possibly by copying it​:

my $var2 = $var; $sub = sub () { $var2 };

If you do want this subroutine to be a closure that reflects future changes to the variable that it closes over\, add an explicit "return"​:

my $var; $sub = sub () { return $var };

This usage has been deprecated\, and will no longer be allowed in Perl 5.32. #####

The entry was made in this commit​:

##### commit 9840d1d66ee1648f6d7fb1554101450158cfee16 Author​: Abigail \abigail@​abigail\.be Date​: Sat Jan 14 18​:25​:48 2017 +0100

Deprecating the modifyable variables in constants by 5.32.

It was already deprecated\, but we're now adding a version number. #####

Make it so.

Please review patch attached\, which is smoking in this branch​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Thank you very much.

I'm not sure this is the desired behaviour\, though maybe we do want it fatal for a release or two to ensure any code that depends on the old behaviour is updated.

As to your patch\, you've left code in after the croak() that will no longer be executed (and since copied can no longer be true\, some other code can be removed.)

So there's two issues​:

a) should sub () { $x } return the current value of $x or croak at compile-time

b) if we croak at compile-time\, the patch needs work

Well\, I was just coding to see if I could change the deprecation to a fatalization. I don't really understand the code\, so we don't have to go forward with this patch.

But with (a) you seem to be suggesting that we need a better understanding of what we *should* be doing. What would that be?

I expect making it fatal will be the right choice at least for now.

Making 'sub () {$x}' behave as the code reads may break code that expects the old behaviour where the warnings have been suppressed or possibly only seen by end users.

Making it fatal (especially at compile-time\, as we do) will prevent that.

But the const-ifying behaviour has been deprecated for a while now.

I don't know what the intent was when it was originally deprecated.

Tony

Can we reformulate the "ask" in this ticket so that we know what we want to deliver in perl-5.32?

Thank you very much.

Right now I'm inclined to make it fatal.

I don't know whether it should remain so forever.

The attached applies on top of your patch to clean up the no longer needed code and re-wrap the croak.

Tony

Thanks. I have applied your patch in my local branch and pushed the result for another round of smoking​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Additional eyeballs on the patches would be welcome. Assuming no problems\, in about 7 days I'll squash the commits and merge into blead in time for the monthly release.

Thank you very much. -- James E Keenan (jkeenan@​cpan.org)

p5pRT commented 5 years ago

From @tonycoz

On Sat\, 10 Aug 2019 06​:05​:35 -0700\, jkeenan wrote​:

Thanks. I have applied your patch in my local branch and pushed the result for another round of smoking​:

smoke-me/jkeenan/rt-134138-constants-in-variable

Additional eyeballs on the patches would be welcome. Assuming no problems\, in about 7 days I'll squash the commits and merge into blead in time for the monthly release.

Thank you very much.

Applied as 30fc7a2809e5a175e2d9bb94d765b2039f270d91.

Tony

p5pRT commented 5 years ago

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