Fix Issue 17674 - [REG 2.064] Simultaneous opBinary and opBinaryRight is not rejected

[RazvanN7]

The problem is that the same Match object is used to check both opBinary and opBinaryRight and the last match will always be taken into account without comparing the result with the previous match. functionResolve cannot be used successively with the same Match object if the parameters differ, because the match count will always be reset to 1 when a better one is found.

The fix is to use different match objects and then aggregate them accordingly.

[dlang-bot]

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	17674	regression	[REG 2.064] Simultaneous opBinary and opBinaryRight is not rejected

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#8922"

[thewilsonator]

Needs to target stable though.

[RazvanN7]

@thewilsonator Done.

[RazvanN7]

phobos fails because of this pattern (reduced):

struct S
{
       int opBinary(string op, T)(T a) { return 1; }
       int opBinaryRight(string op, T)(T a) { return 2; }
}

void main()
{
     S a, b;
     int k = a + b;      // opBinary and opBinaryRight both match.
}

[thewilsonator]

The phobos usage is

    real opBinary(string op,T)(T b)
        if (__traits(compiles, mixin(`get!real`~op~`b`)))
    {
        return mixin(`get!real`~op~`b`);
    }

    real opBinaryRight(string op,T)(T a)
        if ( __traits(compiles, mixin(`a`~op~`get!real`)) &&
            !__traits(compiles, mixin(`get!real`~op~`b`)))
    {
        return mixin(`a`~op~`get!real`);
    }

It looks like opBinaryRight should be called only when opBinary fails. So I'm not sure why its getting this deprecation.

Ooh, is it a reflexive transformation?
Seems like another overload constrained by
if (__traits(compiles, mixin(get!real~op~b.get!real))) with the others exclusive to it should work.

[thewilsonator]

I'll do up a phobos PR tomorrow if you don't beat me to it.

[thewilsonator]

@RazvanN7 please rebase.

[thewilsonator]

I'm going to resuscitate this and see if it fixes anything.

[thewilsonator]

Hmm, this also breaks taggedalgebraic

unittest { // Binary op between two TaggedAlgebraic values
	union U { int i; }
	alias TA = TaggedAlgebraic!U;

	TA a = 1, b = 2;
	static assert(is(typeof(a + b) == int)); //  <<< Fails
}

cc @s-ludwig

[s-ludwig]

I've pushed a fix and a new version (0.10.13) of taggedalgeraic now. (PR: s-ludwig/taggedalgebraic#21)

[thewilsonator]

Thanks!

[s-ludwig]

Still failing for some reason. BTW, I can imagine situations where two independent types have legitimate generic opBinary+opBinaryRight implementations, but using them together would result in an ambiguity (something like two different matrix types maybe). Maybe it would be better to define opBinary taking precedence instead of making this an error?

This is also kind of obvious in my attempt to fix the error for TaggedAlgebraic - I need to explicitly check with the other operand is also a TaggedAlgebraic, but what about if this is something like Phobos' Algebraic instead?