Re: [PATCH] Perl RT#3105 Mark elements of constant range as read only
Abigail
> Hi,
>
> This makes 1..5 no different than 1,2,3,4,5 in terms of functionality, and
> avoids some very subtle bugs.
>
It does however have the potential to break code.
Abigail
Eric Brine
Yes. Specifically, it'll break code (noisily) that modifies what Perl treats
as constants. I even had to fix such a bug in t/op/mydef.t. This is what the
submitter and the responders requested. If you'd rather I do it differently,
I'm up to giving it a go.
Eric Brine
Since the bug is due to an optimisation, I figured it would be worthwhile to
benchmark the usefulness of the optimisation.
There's almost no difference for small lists. For long lists, the
optimisation results in a substantial difference in performance.
$ git diff
diff --git a/op.c b/op.c
index d4f6fb3..32fa2b4 100644
--- a/op.c
+++ b/op.c
@@ -2590,6 +2590,8 @@ S_gen_constant_list(pTHX_ register OP *o)
register OP *curop;
const I32 oldtmps_floor = PL_tmps_floor;
+ return o;
+
list(o);
if (PL_parser && PL_parser->error_count)
return o; /* Don't attempt to run with errors */
$ cat a.pl
use Time::HiRes qw( time );
my $N = $ARGV[0] || 2000;
my $s_time = time;
my @a = map { my $p=$_; map { "$p:$_" } 1..$N } 1..$N;
my $e_time = time;
print($e_time - $s_time, "\n");
Blead (at 789c461534f3eb0346447f8127786b7da3017f6c):
$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 20 ; done
0.000880002975463867
0.000934123992919922
0.000874042510986328
0.000941991806030273
0.000870943069458008
0.000852108001708984
avg: 0.000892202059427897
$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 2000 ; done
6.18947696685791
6.17325901985168
6.38008713722229
6.51624393463135
6.2569899559021
6.23375201225281
avg: 6.29163483778636
Blead with optimisation removed:
$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 20 ; done
0.000860929489135742
0.000841140747070312
0.000859975814819336
0.000878810882568359
0.000877141952514648
0.000889062881469727
avg: 0.000867843627929687
$ for q in 1 2 3 4 5 6 ; do perl -Ilib a.pl 2000 ; done
9.09057521820068
8.82025122642517
9.41165494918823
8.80950093269348
9.01842403411865
8.80255198478699
avg: 8.99215972423553
- ELB
Abigail
I've no suggestion to do it differently. I'm just pointing out the patch
comes with a price - it may break code. I've no opinion on whether that
price is one we want to pay.
I've written code in past like this:
for (1 .. 3) {
$_ .= "foo";
say;
}
__END__
1foo
2foo
3foo
which just DWIM. I'd be surprised if others haven't done so either,
be it on purpose, or by accident.
OTOH, I had code that broke when
map {s/.//; $_} "ab", "bc", "cd";
no longer was possible, and that breakage didn't stop progress from happening.
But that was 10 years ago, and peoples opinion on backwards compatability
have changed.
I've never been happy with the restriction of not being able to modify
values - the fact the values are marked read-only is an implementation
issue - and I prefer programmers not to be hindered by implementation
issues.
Abigail
David Golden
> I've written code in past like this:
>
> for (1 .. 3) {
> $_ .= "foo";
> say;
> }
Yes, but if that was (1,2,3), it would fail. E.g., currently:
$ perl -E 'for (1, 2, 3) { $_.="foo"; say }'
Modification of a read-only value attempted at -e line 1.
$ perl -E 'for (1 .. 3) { $_.="foo"; say }'
1foo
2foo
3foo
So I think making constant *range* act like a constant *list* is
"fixing a bug" not "breaking a feature".
-- David
Eric Brine
> I've no suggestion to do it differently. I'm just pointing out the patch
> comes with a price - it may break code. I've no opinion on whether that
> price is one we want to pay.
>
Since fixing the bug can have negative effects, we could deprecate in 5.12
(by adding Set magic to the returned values), fix in 5.14.
Jesse
I'm afraid you've missed the cutoff for deprecating things in 5.12.
Certainly, it's worth continuing to clean up and get ready to propose a
fix for 5.13.
Rafael Garcia-Suarez
I'd rather have it the other way around : make the for(1,2,3) variant
work, with some sort of copy-on-write.
Nicholas Clark
> 2009/11/25 David Golden <xda...@gmail.com>:
> > So I think making constant *range* act like a constant *list* is
> > "fixing a bug" not "breaking a feature".
>
> I'd rather have it the other way around : make the for(1,2,3) variant
> work, with some sort of copy-on-write.
But currently C<for> is defined as aliasing the iterator variable to each item
of the list in turn. Scalar literals are (currently) constants. Would you
envisage this change being for C<for> only? Or for all literals, so that
this would no longer be an error:
$ perl -le 'sub a { $_[0]++ }; a(1)'
Modification of a read-only value attempted at -e line 1.
Nicholas Clark
Abigail
I probably missed it, but which bug is being fixed by forbidding to
modify the iterator in C<< for $iterator (RANGE) >>?
I've always considered C<< for (1 .. 3) { ... } >> to be more equivalent to
C<< for (my $x = 1; $x <= 3; $x ++) {local $_ = $x; ... } >> than to
C<< for (1, 2, 3) { ... } >>, ever since the range was no longer expanded to
a list at compile time. (5.004?)
Abigail
Eirik Berg Hanssen
> Abigail <abi...@abigail.be> writes:
>
>> I probably missed it, but which bug is being fixed by forbidding to
>> modify the iterator in C<< for $iterator (RANGE) >>?
>
> If nothing else, the bug that arises when someone replaces that
> C<for> with a C<map> (minimal example):
>
> perl -e 'sub a { print($_++) for 1..3; print $/ } a;a;a'
> 123
> 123
> 123
>
> perl -e 'sub a { map print($_++), 1..3; print $/ } a;a;a'
> 123
> 234
> 345
Or even, as I find upon reading ikegami's recent recent additions to
the RT ticket:
perl -e 'sub a { print($_++) for 1..3, @x; print $/ } a;a;a'
123
234
345
All I did was add C<, @x>, and suddenly my little example subroutine
is self-modifying. Stateful. Whatever. Point is, I didn't ask for it.
Or did I? :)
Eirik
--
"So this is the Sword of Immortality? Huh?
What's it doin' in a CRYPT?!"
--- John S. Novak, III, quoting an unnamed player
Eirik Berg Hanssen
> I probably missed it, but which bug is being fixed by forbidding to
> modify the iterator in C<< for $iterator (RANGE) >>?
If nothing else, the bug that arises when someone replaces that
C<for> with a C<map> (minimal example):
perl -e 'sub a { print($_++) for 1..3; print $/ } a;a;a'
123
123
123
perl -e 'sub a { map print($_++), 1..3; print $/ } a;a;a'
123
234
345
Eirik
--
"[..] the buzz about Ruby and Rails is the sound of a bunch of
Java programmers finally discovering how cool Perl is."
-- Piers Cawley
Eric Brine
> I probably missed it, but which bug is being fixed by forbidding to
> modify the iterator in C<< for $iterator (RANGE) >>?
>
C<< for $x (EXPR..EXPR) >> is special (optimised). It *doesn't* use the
range operator. It shouldn't be affected by the patch. (I'll test when I
can.) If we actually use the range operator, you'll see the problem if you
repeat the statment containing the range. For example,
for (1..2) {
for (1 .. 3,@x) {
$_ .= "foo";
say;
}
}
and
for (1..2) {
map {
$_ .= "foo";
say
} 1..3;
}
both output
1foo
2foo
3foo
1foofoo
2foofoo
3foofoo
Eric Brine
> I'd rather have it the other way around : make the for(1,2,3) variant
> work, with some sort of copy-on-write.
>
I'm going to worry about CONST..CONST first.
Behaviour in 5.12 and earlier:
CONST..CONST returns something that can be modified, but that you're not
allowed to modify.
What the bug report requests and what the patch provides:
CONST..CONST returns something that can't modified.
What I'd like:
CONST..CONST to return something that you can modify.
Means of achieving this:
Remove the optimisation that computes the list at compile-time.
Any other faster means?
I don't know. You mentioned copy on write. Is there an existing mechanism
for that?
Eric Brine
> On Thu, Nov 26, 2009 at 5:52 AM, Abigail <abi...@abigail.be> wrote:
>
>> I probably missed it, but which bug is being fixed by forbidding to
>> modify the iterator in C<< for $iterator (RANGE) >>?
>>
>
> C<< for $x (EXPR..EXPR) >> is special (optimised). It *doesn't* use the
> range operator. It shouldn't be affected by the patch. (I'll test when I
> can.)
>
Confirmed. This patch does NOT affect C<< for (EXPR..EXPR) >>.
$ perl -Ilib -E'for (1..2) { for (1..3) { ++$_; say } }'
2
3
4
2
3
4
Those loops are optimised into counting loops. The range never gets
flattened. Other patterns are affected:
$ perl -Ilib -E'for (1..2) { for (1..3,@x) { ++$_; say } }'
Modification of a read-only value attempted at -e line 1.
$ perl -Ilib -E'for (1..2) { for (reverse 1..3) { ++$_; say } }'
Zefram
>What the bug report requests and what the patch provides:
>CONST..CONST returns something that can't modified.
I believe this is the ideal behaviour. The optimisation of
for(CONST..CONST) should be modified to match, providing read-only values.
>What I'd like:
>CONST..CONST to return something that you can modify.
Eww. We have enough ways already to create mutable values, and most
of them are a lot clearer than this about the scope of the variable.
If something looks constant then it probably should be.
-zefram
ava...@gmail.com
> This makes 1..5 no different than 1,2,3,4,5 in terms of functionality, and
> avoids some very subtle bugs.
If this doesn't get accepted for 5.12 it would be very good to have a
version of your change t/op/range.t and other relevant tests that
passes with the current behavior in blead. Whatever the semantics end
up being they need to be tested.
Eric Brine
<ava...@gmail.com>wrote:
> On Wed, Nov 25, 2009 at 00:52, Eric Brine <ike...@adaelis.com> wrote:
> > This makes 1..5 no different than 1,2,3,4,5 in terms of functionality,
> and
> > avoids some very subtle bugs.
>
> If this doesn't get accepted for 5.12
It won't.
> it would be very good to have a
> version of your change t/op/range.t and other relevant tests that
> passes with the current behavior in blead. Whatever the semantics end
> up being they need to be tested.
>
Can do. Should it prove the bug like
is( ( join ' ', map { join '', map { ++$_ } 1..4 } 1..2 ), '2345 3456' );
or just should it simply be
is( ( join '', map { ++$_ } 1..4 ), '2345' );