Incompatible change in 5.42: builtin::indexed and two-variable foreach loop

[jkeenan]

(The following bug report was originally posted by Michael Schapp on the perl5-porters mailing list as "Incompatible change in 5.42".)

In perldelta for 5.42, under Performance Enhancements:

Code that uses the indexed function from the builtin module to generate a list of index/value pairs out of an array or list which is then passed into a two-variable foreach list to unpack those again is now optimised to be more efficient.

        my @array = (...);

        foreach my ($idx, $val) (builtin::indexed @array) {
            ...
        }

        foreach my ($idx, $val) (builtin::indexed LIST...) {
            ...
        }

In particular, a temporary list twice the size of the original is no longer generated. Instead, the loop iterates down the original array or list in-place directly, in the same way that foreach (@array) or foreach (LIST) would do.

Although this is a nice enhancement, isn't that an incompatible change?

    $ /opt/perl-5.40/bin/perl -E 'my @a = (1..5); for my ($i,$x) (indexed @a) { $x += $i }; say for @a;'
    1
    2
    3
    4
    5

    $ /opt/perl-5.42/bin/perl -E 'my @a = (1..5); for my ($i,$x) (indexed @a) { $x += $i }; say for @a;'
    1
    3
    5
    7
    9

[leonerd]

Ugh, yes that is an unfortunate side-effect of the "alias rather than temporary copy" behaviour of the foreach ARRAY optimisation. I didn't quite think about that.

It's too late for 5.42 now but we should think about whether that ought to be the behaviour going forward, or whether we should put it back to being a copy of the value, and note that "oops we messed up for 5.42, will be fixed later".

[jkeenan]

For reference: Bisecting with the following invocation:

$ perl Porting/bisect.pl --start=v5.40.0 --end=v5.42.0 -e 'use builtin qw(indexed); my @a = (1..5); for my ($i,$x) (indexed @a) { $x += $i }; die if $a[1] == 3;'

... points to a8394b4 where perl's behavior changed.

a8394b4e8cb3545141487cc9f17c08f312ec635c is the first bad commit
commit a8394b4e8cb3545141487cc9f17c08f312ec635c
Author: Paul "LeoNerd" Evans <leonerd@leonerd.org.uk>
Date:   Wed Oct 2 17:34:57 2024 +0100
Commit:     Paul Evans <leonerd@leonerd.org.uk>
CommitDate: Wed Nov 6 23:21:06 2024 +0000

    Optimise foreach on a list from builtin::indexed @array into two lexicals
    
...

[Grinnz]

I'm not sure theoretically whether I would consider this incompatible unless there's an example of it happening in the real world. In the example given, that code does not do anything so no one would ever write it intentionally.

[mscha]

I'm not sure theoretically whether I would consider this incompatible unless there's an example of it happening in the real world.

Here's some code I wrote yesterday to process results from open-meteo.com:

    my $fcst = {};
    for my ($i, $date) (indexed $daily->{time}->@*) {
        $date =~ s{-}{}g;
        next unless defined($daily->{temperature_2m_max}[$i]);
        $fcst->{$date} = { min=>$daily->{temperature_2m_min}[$i],
                           max=>$daily->{temperature_2m_max}[$i] };
        $fcst->{$date}{sun} = nearest(0.1, $daily->{sunshine_duration}[$i]/3600) if defined($daily->{sunshine_duration}[$i]);
        $fcst->{$date}{prec} = $daily->{precipitation_sum}[$i] if defined($daily->{precipitation_sum}[$i]);
    }

The dates in daily/time are in the form YYYY-MM-DD and I needed YYYYMMDD, so I s{-}{}ged the -es away.
Then I remembered the 5.42 perldelta and checked it out.

(I wrote this in 5.42 so it didn't break existing code, and in effect didn't break at all since I never use daily/time after that. But this could cause surprises.)

Edit: Personally, I am of two minds about this.
On the one hand, it is actually a nice feature that for my ($idx, $val) (indexed @array) loops in-place, just like for my $val (@array) does.
On the other hand, it is very much a special case. Put any other function than builtin::indexed in its place, and suddenly it doesn't loop in-place anymore. [This turns out to be incorrect. Thanks, ap, learned something new.]
I don't know which hand should win. But at the very least this should be explicitly documented.

[Grinnz]

Thank you, that convinces me that some care needs to be taken dealing with this. At the same time, it has been un-experimental with the non-aliasing behavior for only one release, though it is still documented that way, which is also unfortunate. Considering what's already been changed, perhaps it would make sense to re-mark the function as experimental for a couple releases?

[ap]

On the other hand, it is very much a special case. Put any other function than builtin::indexed in its place, and suddenly it doesn't loop in-place anymore.

Incorrect.

for ( values %birthday ) { tr[-][]d }

And it’s not just builtins like values, as I’ll get to below.

Personally I use the aliasing behavior for foreach (LIST) quite a bit and would find myself avoiding indexed specifically to get it. So if I’d been paying attention when indexed was initially proposed, I’d’ve argued for this from the start.

And then builtin::compat would have to switch from a very obvious

sub indexed (@) {
  my $i = 0;
  map +($i++, $_), @_;
}

to the much more cryptic

sub indexed (@) : lvalue {
  ( 0..$#_, @_ )[ map +($_, $_+@_), 0..$#_ ];
}

to replicate this semantic. But it can.