Skip to content

Fetch operands of binary operators in left-to-right order#23108

Merged
khwilliamson merged 3 commits into
Perl:bleadfrom
t-a-k:uninit-warning-order
Jul 15, 2025
Merged

Fetch operands of binary operators in left-to-right order#23108
khwilliamson merged 3 commits into
Perl:bleadfrom
t-a-k:uninit-warning-order

Conversation

@t-a-k

@t-a-k t-a-k commented Mar 14, 2025

Copy link
Copy Markdown
Contributor

I noticed that perl emits "Use of uninitialized value ..." warnings in right-to-left
order if both operands of a binary operator are uninitialized:

% perl -wle 'my ($a, $b); print $a + $b'
Use of uninitialized value $b in addition (+) at -e line 1.
Use of uninitialized value $a in addition (+) at -e line 1.
0

I think this is not intuitive and inconsistent with Perl's general rule of left-to-right evaluation order (mentioned in perlop).

This change will fix the order of this warning messages by reordering operand fetch.


  • This set of changes requires a perldelta entry, and it is included.

@jkeenan jkeenan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given (a) the depth in the perl guts of the change requested, (b) the point we are at in the current development cycle, (c) the current unreliability of cpantesters.org for accepting and reporting results, I do not believe we should consider this patch for the current dev cycle. I recommend it be labeled 'defer-next-dev'.

@tonycoz tonycoz added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Mar 17, 2025
@tonycoz

tonycoz commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

I think this is a reasonable change.

I recommend it be labeled 'defer-next-dev'.

I agree.

@iabyn

iabyn commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

I'm not necessarily opposed to this change, but note that it affects more than just the ordering of 'uninit' warnings. Since it changes the ordering of retrieving various ops' args, it will also affect things like the order in which FETCH() is called if both args are tied, and similarly for other magic types. Thus it could change the behaviour of existing code (which would arguably be relying on undefined behaviour).

@t-a-k

t-a-k commented Mar 17, 2025

Copy link
Copy Markdown
Contributor Author

Since it changes the ordering of retrieving various ops' args, it will also affect things like the order in which FETCH() is called if both args are tied, and similarly for other magic types.

I think that this is not the case, as this change will only transpose SvXV_nomg() which do not handle magic types, and for these binary operators, magic types (including tied variables) are handled in Perl_try_amagic_bin() (via rpp_try_AMAGIC_2) which already calls FETCH() in left-to-right order.

So, I think that this change should not change the behavior of tied variables.

@iabyn

iabyn commented Mar 18, 2025 via email

Copy link
Copy Markdown
Contributor

@tonycoz

tonycoz commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

So, I think that this change should not change the behavior of tied variables.

It can change the order of "" and 0+ overloading calls.

@t-a-k

t-a-k commented Mar 21, 2025

Copy link
Copy Markdown
Contributor Author

It can change the order of "" and 0+ overloading calls.

Current implementation seems to handle 0+ overloading calls also in Perl_try_amagic_bin and already call in left-to-right order even before this change:

# test.pl
use v5.10;
use strict;
use warnings;

package Foo {
    use overload
	fallback => 1,
	'0+' => sub { warn "0+ called on ${$_[0]}"; 1 };
    sub new { bless \ (my $a = $_[1]) }
}

my $a = Foo->new('a');
my $b = Foo->new('b');

say '$a + $b = ', $a + $b;
$ perl -wle 'print $^V'
v5.32.1
$ perl test.pl
0+ called on a at test.pl line 9.
0+ called on b at test.pl line 9.
$a + $b = 2

And this patch changes only numeric operators which will not involve "" overloading calls.

@tonycoz

tonycoz commented Mar 25, 2025

Copy link
Copy Markdown
Contributor

Current implementation seems to handle 0+ overloading calls also in Perl_try_amagic_bin and already call in left-to-right order even before this change:

For most cases the non-integer path, ie. when we can't see that both arguments are integers, uses left to right order and there's no change in evaluation ordering.

I did find some cases where it did change - the use integer mode ops:

tony@venus:.../git/perl6$ cat ../23108-over.pl
use warnings;
use strict;
use integer;

my $l = Foo->new(1);
my $r = Foo->new(2);

my $x = $l + $r;
$x = $l * $r;
$x = $l < $r;

package Foo;
use overload '0+' =>
  sub {
    print "over ", $_[0]->$*, "\n";
    $_[0]->$*;
  },
  fallback => 1;

sub new {
  bless \(my $x = $_[1]), $_[0];
}
tony@venus:.../git/perl6$ perl ../23108-over.pl
over 2
over 1
over 2
over 1
over 2
over 1
# built from this PR:
tony@venus:.../git/perl6$ ./perl -Ilib ../23108-over.pl
over 1
over 2
over 1
over 2
over 1
over 2

@t-a-k

t-a-k commented Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

I did find some cases where it did change - the use integer mode ops:
...

Oh, that makes sense.

I tested test.pl (from my previous comment) with perl v5.41.10 (before applying this patch) which shows that use integer changes the order to call 0+ overloading calls:

$ perl test.pl
0+ called on a at test.pl line 9.
0+ called on b at test.pl line 9.
$a + $b = 2
$ perl -Minteger test.pl
0+ called on b at test.pl line 9.
0+ called on a at test.pl line 9.
$a + $b = 2

I think that this behavior, changing the order of calling 0+ calls depending on use integer, is considered a bug to be fixed.

Anyway, this patch will (slightly) change the behavior of perl, not only changing the output ordering of warnings. I will modify the title and perldelta of this issue.

@tonycoz

tonycoz commented Mar 25, 2025

Copy link
Copy Markdown
Contributor

If it wasn't clear, this is approved, but for 5.43.

@t-a-k t-a-k changed the title Issue "Use of uninitialized value" warnings in left-to-right order Fetch operands of binary operators in left-to-right order Mar 28, 2025
@khwilliamson khwilliamson removed the defer-next-dev This PR should not be merged yet, but await the next development cycle label Jul 4, 2025
@khwilliamson

Copy link
Copy Markdown
Contributor

@t-a-k t-a-k. Please rebase and repush and it will get put into blead

Comment thread pp.c Outdated

/* If left operand is undef, treat as zero - value */
nv = useleft ? SvNV_nomg(svl) : 0.0;
/* Separate statements here to ensue SvNV_nomg(svl) is evaluated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/ensue/ensure

Comment thread pod/perldelta.pod
@@ -376,6 +376,16 @@ manager will later use a regex to expand these into links.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the commit message s/thange/change/

Comment thread pod/perldelta.pod Outdated

When both operands of arithmetic operators (C<+>, C<->, etc.) are
L<overload>ed objects which have no method for that operator but have
C<0+> method with C<fallback> option being set to TRUE,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a C<0+> method with the C option being set to TRUE,

Comment thread pod/perldelta.pod Outdated
L<overload>ed objects which have no method for that operator but have
C<0+> method with C<fallback> option being set to TRUE,
perl will call the C<0+> methods for these operands in the same order
with operand evaluation order, i.e. left-to-right normally,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/with/as/

Comment thread pod/perldelta.pod Outdated
perl will call the C<0+> methods for these operands in the same order
with operand evaluation order, i.e. left-to-right normally,
but if S<C<use integer;>> is in effect they used to be called
in reverse order.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what is meant here

…t order

Some binary (2-operand) operators, notably arithmetic add (+) and
subtract (-), used to fetch its RHS operand first then LHS one,
so they would issue "use of uninitialized value" warnings in
right-to-left order:

 % perl -wle 'print $a + $b'
 Use of uninitialized value $b in addition (+) at -e line 1.
 Use of uninitialized value $a in addition (+) at -e line 1.
 0
 %

I think this is not intuitive for users, as "perlop" says that
"Perl has a general rule that the operands of an operator are evaluated
in left-to-right order."  (3-operand case is more surprising;
"print $a + $b + $c" will warn in the order $b, $a, $c.)

This change reverses the operand fetch order in these operators
to issue warnings in left-to-right order.

t/lib/warnings/9uninit: Reorder expected warnings in left-to-right order.

pod/perldelta.pod: Add perldelta entry for this change.
t-a-k added 2 commits July 10, 2025 01:09
Before this change, pp_add() and pp_subtract() had two (or three
before the previous commit) calls of TARGn(<result>, 1) for each.
Consolidating these calls into single call will (slightly) reduce
the size of the compiled code.
…led in reverse order

The previous commit "pp.c, pp_hot.c: Reorder SvXV_nomg() to fetch operands
in left-to-right order" will not only change the order of "Use of
uninialized value" warnings, but actually also change the order of
the call of "0+" overloading methods for each operands.
@t-a-k t-a-k force-pushed the uninit-warning-order branch from d9b3bd5 to b06fe6b Compare July 9, 2025 16:21
@t-a-k

t-a-k commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

Sorry for delayed response, and thank you for rebasing and comments.

I've force-pushed the branch to:

  • correct typo in comments (s/ensue/ensure/ in pp.c, pp_hot.c)
  • correct typo in commit message (s/thange/change/)
  • reword the perldelta entry
  • rebase again to the current blead

@khwilliamson khwilliamson merged commit e78ee04 into Perl:blead Jul 15, 2025
33 checks passed
@t-a-k t-a-k deleted the uninit-warning-order branch July 16, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants