Eliminate modifiable variables in constants #17020
Comments
From @jkeenanpod/perldeprecation.pod contains the following entry: ##### You wrote something like my $var; $sub = sub () { $var }; but $var is referenced elsewhere and could be modified Traditionally, Perl has captured the value of the variable If you intended for the subroutine to be eligible for my $var2 = $var; $sub = sub () { $var2 }; If you do want this subroutine to be a closure that my $var; $sub = sub () { return $var }; This usage has been deprecated, and will no longer be The entry was made in this commit: ##### Deprecating the modifyable variables in constants by 5.32. It was already deprecated, but we're now adding a version number. Make it so. |
From @jkeenanSummary of my perl5 (revision 5 version 31 subversion 0) configuration: Characteristics of this binary (from libperl): |
From @jkeenanOn Sat, 25 May 2019 02:03:40 GMT, jkeenan@pobox.com wrote:
Please review patch attached, which is smoking in this branch: smoke-me/jkeenan/rt-134138-constants-in-variable Thank you very much. |
From @jkeenan0001-Eliminate-modifiable-variables-in-constants.patchFrom 083d8eb22b2e6884eb8d99b7677d3ff4b0f1f5f6 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
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<perlsub/"Constant Functions"> and L<constant>.
-=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
|
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Sat, 25 May 2019 19:30:56 -0700, jkeenan wrote:
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 |
From @jkeenanOn Wed, 05 Jun 2019 01:10:06 GMT, tonyc wrote:
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. -- |
From @tonycozOn Tue, 04 Jun 2019 18:22:38 -0700, jkeenan wrote:
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 |
From @jkeenanOn Wed, 05 Jun 2019 01:35:47 GMT, tonyc wrote:
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. |
From @tonycozOn Fri, 09 Aug 2019 14:29:51 -0700, jkeenan wrote:
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 |
From @tonycoz0002-remove-now-irrelevant-code-and-reflow-the-error-mess.patchFrom b99017d3e3ae6fd39547550323ef159b1d9bede1 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
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
|
From @jkeenanOn Sat, 10 Aug 2019 05:23:47 GMT, tonyc 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. |
From @tonycozOn Sat, 10 Aug 2019 06:05:35 -0700, jkeenan wrote:
Applied as 30fc7a2. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
Migrated from rt.perl.org#134138 (status was 'pending release')
Searchable as RT134138$
The text was updated successfully, but these errors were encountered: