Implement PPC0030: undef aware equality

[leonerd]

Now we're in the change freeze ahead of 5.44's release, this is an excellent time to start thinking about and reviewing changes for the 5.45.x cycle. With that in mind, this PR is currently still in draft, as I have a number of things to finish off first.

This adds a set of new operators for comparing values for equality, which consider undef to be a distinct value and not equal to either the empty string, or zero. These are specified in PPC0030.

Still TODO:

• Merge perldelta fragment into actual file
• Squash commits down to one main one + prereq support

• This set of changes requires a perldelta entry, and it will be included by the time I undraft it

[tonycoz]

• Specifically, document the interaction with operator overloading

I think they would need their own amg values, and I guess should fallback to the original op after the definedness comparison, if the fallback is enabled and the original op has an overload.

If fallback isn't enabled (edit) and the overload isn't defined (/edit) the overload should throw an exception like it does for any other op.

If fallback is enabled by neither the undef-aware nor the base op is overloaded the normal path should be taken (try_amagic_bin() numifies the result for the numeric comparisons and the normal op implementation does the right thing.

[leonerd]

@tonycoz :

Right then. This new approach defines a whole new set of op codes and pp funcs for the undef-aware versions. I'm undecided about the names of them - OP_SEQU is a bit opaque and doesn't have to match the Perl-visible operator name of equ so I might rename it to something more obvious like OP_SEQ_UNDEF or somesuch. That will have knock-on effects on several other internal names.

The newly-added numerical comparison codes are annoyingly no longer in ordered sequence with the other ones, so it breaks the operation of OP_IS_NUMCOMPARE. I think the only fix for that is to disobey the top line of regen/opcodes which tells us to add new ones at the end, and instead add these four new ones directly after the non-undef versions of the same; where arguably people will expect to see them. I know that does upset the numbers of existing ones but I feel that's probably fine between release versions, right?

There is however one big snag that I haven't thought of a good solution to, and that's the intended operation of the try_AMAGIC_2 call that would need to be added, in order to support overloaded operators. If the (currently-commented) code were simply activated, it would result in two sets of GETMAGIC() calls for every use of the operator - once while testing for seq_undef_amg and once again while testing for seq_amg. This would also duplicate a bunch of "does this value have amagic and if so where's the overload table?" behaviour. I can imagine two solutions to this:

1. Don't call the standard try_AMAGIC_2 from these pp funcs and instead inline and expand the required logic so it can have the right semanics re: GETMAGIC and efficiency. I don't like it, because it involves a lot of copypaste duplication of that logic, plus also means now the undef-aware operators can't just chain call to the non-undef versions for the "both sides are defined" case, and have to duplicate their logic here also.

2. Implement the entire fallback semantics as part of the huge and sprawling mass of code that is Perl_amagic_call(), taking into account the tests for whether either argument is undef, falling back to regular string case, and so on... I also don't like that because it involves duplication of the "are the arguments undef?" logic, and further hides semantics away in that giant monstrosity known as Perl_amagic_call(). ((Did I ever mention, I don't like the design of Perl_amagic_call()? Can you tell? ;) ))

Aside from those I don't have any other ideas, nor do I have a good feel for which one would be preferrable. Something to think on perhaps...

(Oh and also, yes I know the branch fails sanity currently; this is due to the amagic tests I mention above)

[tonycoz]

Another thing to consider is use integer;:

tony@venus:.../git/perl6$ ./perl -Ilib -Minteger -E 'say 123 == 123.1'
1
tony@venus:.../git/perl6$ ./perl -Ilib -Minteger -E 'say 123 === 123.1'
 
tony@venus:.../git/perl6$

[tonycoz]

There is however one big snag...

I don't think the code in amagic_call() would be recursively calling the pp funcs.

I don't see a way to avoid messing with amagic_call() and it doesn't seem too bad:

First is the switch statement that handles finding fallbacks:

https://github.com/leonerd/perl5/blob/e27f6109f8a3cde7a6f67866f7c88b0aaf7ab0ee/gv.c#L3967-L3970

This checks for fallback ops, so we might add:

   case equ_amg:
       /* missing stack stuff in these returns? */
       if (!SvOK(left) && !SvOK(right))
           return &PL_sv_yes;
       if (!SvOK(left) || !SvOK(right))
           return &PL_sv_no;
       /* off is further checked below the switch */
       if (ocvp[eq_amg]) /* == */
           off = eq_amg;
       else
           off = cmp_amg;
       break;

Maybe put the undef checks in a macro.

Another switch further down.

https://github.com/leonerd/perl5/blob/e27f6109f8a3cde7a6f67866f7c88b0aaf7ab0ee/gv.c#L4271-L4275

again, add a case:

    case equ_amg:
        if (off == cmp_amg)
            ans = SvIV(res) == 0;
        else  /* presumably was eq_amg */
            ans = SvTRUE(res); // not sure if we can avoid this
        break;

Thankfully the use integer ops use the base amg values.

[leonerd]

Rightthen. A new round of updates. Having followed those notes and read over the way gv.c implements overloading, I guess it's not too bad. I still dislike the "separation of concerns" of having this code make decisions about the undef-ness of arguments here as well as in the PP funcs, but that is likely unavoidable.

This latest update now implements most of the use overload ability on these new ops.

While I was there I did realise that the logic could use an overloaded equ operator to synthesize a missing neu (and likewise the numerics), but it currently doesn't for the reason that the older non-undef versions don't do that either. I raised a discussion about that - Perl/PPCs#84. But that can be looked at separately.

I haven't looked into the use integer case yet but I imagine that won't be much work; and at least it's only for the numerical ones and not the stringy ones.

[leonerd]

And now with use integer support:

$ ./perl -E 'use integer; say "OK" if 123.1 === 123'
OK

[leonerd]

Based on Perl/PPCs#87 I've shuffled the implementation here so it doesn't add entire new use overload categories for these new operators.

I added the AMGf_no_GETMAGIC flag because I couldn't think of another way to do this. It seems a potentially useful addition though.

[leonerd]

I've now also added some basic docs in perlop.pod. I feel maybe some words should be said in lib/loverload.pm to explain why these operators aren't separately overloadable, but other than that I don't think there's much else to be written about them.

I won't add a perldelta yet because it will just keep conflicting along rebases around release time. I'll instead add something in a comment here to copy out at the time we get around to thinking about merging this.

[leonerd]

Proposed perldelta:

=head1 Core Enhancements

=head2 Undef-aware equality operators

Four new operators have been added, which are similar to the regular equality operators except for their handling of C<undef>. Whereas the regular operators treat C<undef> as equal to the empty string or the number zero, these operators consider C<undef> to be a distinct value, equal to itself, but unequal to any defined value.

    if( $x equ $y ) {
     # $x and $y are both undef, or
     # $x and $y are both defined and equal
     ...
    }

This is approximately equal to the following, except that it is more efficient and avoids duplicate evaluation of operands or fetching of tied scalar values:

    if( (!defined $x and !defined $y) or
        (defined $x and defined $y and $x eq $y) ) {
      ...
    }

For more detail, see L<perlop/Equality Operators>.

[tonycoz]

The overload.pm docs could probably use a mention of the way overloading works for the new ops, possibly an extra =item in "Overloadable Operations" which seems to be where the special cases are discussed.

[leonerd]

The overload.pm docs could probably use a mention of the way overloading works for the new ops, possibly an extra =item in "Overloadable Operations" which seems to be where the special cases are discussed.

Added.

I also wonder, while we're there, if we should add specific mention about defined and ref as well

[leonerd]

I think if we're agreed we don't need a use feature flag for this (because it's all just new infix operators that previously were syntax errors), this might be ready to undraft.

One of the smoker tests is unhappy currently but that's only because at present, blead's patchlevel.h has not been updated after the release of 5.43.10, so it claims to fail a cmpversion.t check on the newly-updated B::Op_private. That problem will not be present by the time we come to merge this in 5.45.x.

[tonycoz]

I think if we're agreed we don't need a use feature flag for this (because it's all just new infix operators that previously were syntax errors), this might be ready to undraft.

I don't think it needs a feature flag, it may need an experimental warning.

[tonycoz]

Looks fine except for:

# Failed test 1979 - The '!==' operator is experimental at porting/diag.t line 590
#     Message 'The '!==' operator is experimental'
#     from toke.c line 7015 is not listed in pod/perldiag.pod
# Failed test 2030 - The 'equ' operator is experimental at porting/diag.t line 590
#     Message 'The 'equ' operator is experimental'
#     from toke.c line 8494 is not listed in pod/perldiag.pod
# Failed test 2037 - The 'neu' operator is experimental at porting/diag.t line 590
#     Message 'The 'neu' operator is experimental'
#     from toke.c line 8801 is not listed in pod/perldiag.pod
# Failed test 2053 - The '===' operator is experimental at porting/diag.t line 590
#     Message 'The '===' operator is experimental'
#     from toke.c line 9732 is not listed in pod/perldiag.pod
porting/diag.t ....................................................... 
Failed 4/2377 subtests 

[tonycoz]

There's been a few updates, but porting tests are still failing.

[tonycoz]

Now I just need to add this to sv_numeq()/sv_numne().