Bug 16657 – alias this interacts with generated opCmp and opEquals

When adding an "alias this" to a type, the automatic opEquals and opCmp that compare the object lexicographically are removed and instead only the alias this portion of the type is compared via its opEquals/opCmp. struct A { int x; } struct B { A a, b; } static assert(B(A(1), A(1)) != B(A(1), A(2))); // Works struct C { A a, b; alias a this; } static assert(C(A(1), A(1)) != C(A(1), A(2))); // Fails! Adding alias this should NOT cause a preference for the base type's comparators.

Minor correction: opCmp is not auto-generated anyway and opEquals should just be exhaustive and not lexicographic.

Since the auto-generated opEquals() theoretically always exists, having it take precedence makes it a problem to wrap other types and defer to it. But the problem does come in when the auto-generated opEquals() would be non-trivial, as in: --- struct A { int x; bool opEquals(int y) { return y == x; } } struct C { int a; A b; alias a this; } static assert(C(1, A(1)) != C(1, A(2))); --- The current state of affairs is that you'll need to write an explicit C.opEquals() if using an 'alias this' and do not wish the operation to be forwarded to it, even if the other fields have non-trivial opEquals() implementations. I suspect that this is the best solution if only because it is easy to understand. It doesn't have any special cases and exceptions.

It seems a case can be made for either behavior. * The "alias this" feature emulates subtyping of the kind typically achieved by class inheritance. If that is the default behavior to emulate, the current behavior is up to the task. Consider a moral equivalent: class A { int x; this(int x) { this.x = x; } override bool opEquals(Object rhs) { return x == (cast(A) rhs).x; } } class B : A { this(int x, int y) { super(x); this.y = y; } int y; } void main() { assert(new A(1) == new B(1, 2)); // pass } This "slices" B for the purpose of comparison. * Yes, "alias this" does emulate subtyping, but we have an autogenerated opEquals for every struct. Why should there be the case that such generation automatically disappears in the case "alias this" is used? * Another argument for changing behaviors has to do with practicality. Even though a subtyping-based argument can be created in favor of preserving existing compatibility, a better argument is that in fact the _current_ OOP behavior (illustrated by the example with classes above) is incorrect and error-prone. There's good evidence for that (e.g. http://stackoverflow.com/questions/12239344/why-should-i-not-use-equals-with-inheritance of many examples). The theoretical underpinnings for correctly defining comparison in conjunction with subtyping is bounded quantification (https://en.wikipedia.org/wiki/Bounded_quantification). The short of it all is the current behavior can and should be improved. Changing behavior silently is not a good choice, so we should probably issue a warning when the situation occurs. The warning can be extinguished by defining opEquals explicitly. With time the warning becomes an error following a schedule similar to deprecation.

I just spent another half a day debugging a very cryptic bug that crashed our QA runs, and resulted from an incorrect comparison of just an inner field that had "alias this" :-(

We had something like this: if(x != y) { x = y; // expensive assignment } // assume all x fields equal y fields here, we assign them all -- ouch! The semantics now are very surprising: x = y // assigns the whole of "x", not just the "alias this" part x == y // compares just the "alias this" part, wat? Well-behaved assignments and equalities should apply to the same part of the object. That the auto-generated assignment/equals code is NOT well-behaved is surely to be considered a bug. Implementation-wise: It is quite simple to always auto-generate opEquals that compares all fields with == recursively calling opEquals. The auto-generated opEquals overrides the "alias this" opEquals for the (Subtype,Subtype) case. I don't see where the implementation difficulty comes in here. In Walter's example, auto-gen'd C.opEquals would use == on both fields, implicitly invoking opEquals of A. No special cases or exceptions.

Upon further consideration we need to fix this. Requiring people who use the "alias this" feature to also remember to define "opEquals" is the kind of long-distance dependency that is the subject of "Effective D" tidbits of advice of the exact kind we do our best to avoid.

Awesome, thanks!

PR: https://github.com/dlang/dmd/pull/9289

Question for everyone: is this worth a deprecation cycle, and if so how would it behave? It's kinda awkward.

I think a new "transition" compiler flag [1] can be added to warn about the changes in behavior here. It is possible to make these warnings the default for a few versions (with a flag to suppress them), but I think code relying on this bug for correctness is probably very rare. One can always opt-in to all transition warnings via -transition=all when upgrading the compiler. This seems in line with how D warns about other language changes. [1] https://dlang.org/dmd-linux.html#switch-transition

@RazvanN7 updated dlang/dmd pull request #9289 "Fix Issue 16657 and 11161 - alias this interacts with generated opCmp and opEquals" fixing this issue: - Fix Issue 16657 and 11161 - alias this interacts with generated opCmp and opEquals https://github.com/dlang/dmd/pull/9289

dlang/dmd pull request #9289 "Fix Issue 16657 and 11161 - alias this interacts with generated opCmp and opEquals" was merged into master: - 6716c02dae44d6a186ac96af8b0e1f331bcb3069 by RazvanN7: Fix Issue 16657 and 11161 - alias this interacts with generated opCmp and opEquals https://github.com/dlang/dmd/pull/9289

dlang/dmd pull request #11473 "test: Add ICE testcase for issue 16657" was merged into master: - 77eb36275218f6552c7f4c77712dfc433085cdb5 by Iain Buclaw: test: Add ICE testcase for issue 16657 https://github.com/dlang/dmd/pull/11473