www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Wrong selection of opEquals for objects.

reply Alexandru Ermicioi <alexandru.ermicioi gmail.com> writes:
Hi everyone,

there is https://issues.dlang.org/show_bug.cgi?id=21180 bug, 
anyone knows how to avoid it?

Test case:
-------------
import std;

class Silly {
     bool opEquals(const Silly silly) const  safe {
         return silly is this;
     }

     alias opEquals = Object.opEquals;
}

bool comp(T)()  safe {
     return new T() == new T();
}

void main()
{
     comp!Silly.writeln;
     comp!(const Silly).writeln;
     comp!(immutable Silly).writeln;
}
-------------

It always tries to call Object.opEquals, when narrower overload 
should've been selected.

- Alex.
Aug 28 2020
next sibling parent Alexandru Ermicioi <alexandru.ermicioi gmail.com> writes:
On Friday, 28 August 2020 at 08:16:01 UTC, Alexandru Ermicioi 
wrote:
 Hi everyone,
 ....
Would be glad at least to pointers, where in dmd is logic for operator overloading happens, as well as for overloading rules, so I could fix it myself, if no-one is able to pick up it.
Aug 28 2020
prev sibling parent reply Simen =?UTF-8?B?S2rDpnLDpXM=?= <simen.kjaras gmail.com> writes:
On Friday, 28 August 2020 at 08:16:01 UTC, Alexandru Ermicioi 
wrote:
 Hi everyone,

 there is https://issues.dlang.org/show_bug.cgi?id=21180 bug, 
 anyone knows how to avoid it?

 Test case:
 -------------
 import std;

 class Silly {
     bool opEquals(const Silly silly) const  safe {
         return silly is this;
     }

     alias opEquals = Object.opEquals;
 }

 bool comp(T)()  safe {
     return new T() == new T();
 }

 void main()
 {
     comp!Silly.writeln;
     comp!(const Silly).writeln;
     comp!(immutable Silly).writeln;
 }
 -------------

 It always tries to call Object.opEquals, when narrower overload 
 should've been selected.

 - Alex.
Essentially, this boils down to the issues described in https://issues.dlang.org/show_bug.cgi?id=1824, and a host of other bugzilla issues. When you do a == b with a and b being class objects, it's lowered to object.opEquals(a, b)[0], which casts both a and b to Object before doing the comparison. So, you'll need to override Object.opEquals to have the right function called: class Silly { int field; this(int f) { field = f; } override bool opEquals(Object o) { auto silly = cast(Silly)o; // If cast returns null, it's not a Silly instance if (!silly) return false; // Compare Silly objects return field == silly.field; } } unittest { Silly a = new Silly(1); assert(a == new Silly(1)); assert(a != new Silly(2)); } That takes care of choosing the correct overload, but as you may have noticed, there's another issue: your safe function comp() can't call the system function object.opEquals. Since object.opEquals operates on Object instances, not your specific subclass, it has to assume the worst, and is system. There's no real good solution to that in the language as of now, and some of us have been pulling our hair for years because of it. What you'll need to do is mark every function that does compare two class objects with == as trusted or system. -- Simen [0]: https://github.com/dlang/druntime/blob/master/src/object.d#L166
Aug 28 2020
parent reply Alexandru Ermicioi <alexandru.ermicioi gmail.com> writes:
On Friday, 28 August 2020 at 10:28:07 UTC, Simen Kjærås wrote:

 What you'll need to do is mark every function that does compare 
 two class objects with == as  trusted or  system.
No that is not a solution at all, in template code that requires safety. You basically will have to sacrifice safety for rest of types, such as structs, unions & enums for the sake of objects being able to compare. Could we just template that opEquals in this manner: ------- bool opEquals(T : Object, X : Object)(T lhs, X rhs) { if (lhs is rhs) return true; if (lhs is null || rhs is null) return false; if (!lhs.opEquals(rhs)) return false; if (typeid(lhs) is typeid(rhs) || !__ctfe && typeid(lhs).opEquals(typeid(rhs))) { return true; } return rhs.opEquals(lhs); } ------- That would at least allow us to define an overload which is safe and would be picked up by new implementation. I'm wondering why it wasn't done yet, are there any reasons for that? - Alex.
Aug 28 2020
next sibling parent Alexandru Ermicioi <alexandru.ermicioi gmail.com> writes:
On Friday, 28 August 2020 at 10:42:09 UTC, Alexandru Ermicioi 
wrote:
 ...
Also, why it is limited to just objects? It seems that this function enforces symmetry between two objects. What about rest of the possible types, such as structs, unions?
Aug 28 2020
prev sibling parent reply Simen =?UTF-8?B?S2rDpnLDpXM=?= <simen.kjaras gmail.com> writes:
On Friday, 28 August 2020 at 10:42:09 UTC, Alexandru Ermicioi 
wrote:
 No that is not a solution at all, in template code that 
 requires safety. You basically will have to sacrifice safety 
 for rest of types, such as structs, unions & enums for the sake 
 of objects being able to compare.
Yup. There's a reason I'm saying it's suboptimal. FWIW, you can limit the taint by using trusted lambdas. One of the reasons this hasn't been fixed is idiomatic D code very rarely uses classes, but that does not mean they're always the wrong tool.
 Could we just template that opEquals in this manner:
 -------
 bool opEquals(T : Object, X : Object)(T lhs, X rhs)
 {
     if (lhs is rhs) return true;

     if (lhs is null || rhs is null) return false;

     if (!lhs.opEquals(rhs)) return false;

     if (typeid(lhs) is typeid(rhs) ||
         !__ctfe && typeid(lhs).opEquals(typeid(rhs)))
     {
         return true;
     }

     return rhs.opEquals(lhs);
 }
 -------

 That would at least allow us to define an overload which is 
 safe and would be picked up by new implementation.

 I'm wondering why it wasn't done yet, are there any reasons for 
 that?
The template solution may look like it solves every problem, but it really doesn't. Consider this code: class A { bool opEquals(A) { return false; } } class B : A { bool opEquals(B) { return true; } } unittest { B b1 = new B(); B b2 = new B(); A a1 = b1; A a2 = b2; assert(b1 == b2); assert(a1 != a2); // WTF? } With the template solution, the function in B would be ignored when stored in a variable with static type A. This solution would sometimes do the right thing, other times it would silently do the wrong thing.
 Also, why it is limited to just objects? It seems that this 
 function enforces symmetry between two objects. What about rest 
 of the possible types, such as structs, unions?
That's an issue. The spec clearly states (https://dlang.org/spec/operatoroverloading.html#equals): 2. [T]he expressions a.opEquals(b) and b.opEquals(a) are tried. If both resolve to the same opEquals function, then the expression is rewritten to be a.opEquals(b). 3. If one is a better match than the other, or one compiles and the other does not, the first is selected. 4. Otherwise, an error results. This is clearly not the case: struct S1 { bool opEquals(S2 a) { return true; } } struct S2 { bool opEquals(S1 a) { return false; } } unittest { S1 a; S2 b; assert((a == b) == (b == a)); // Fails } I didn't find a bugzilla entry on this, but I'm pretty sure there is one. As for why there's no global function like for classes, that's because there's no need - there is no disconnect between the static and dynamic types of non-class variables (or, if there is, it's explicitly programmed in, like in std.variant.Variant). -- Simen
Aug 28 2020
parent reply Alexandru Ermicioi <alexandru.ermicioi gmail.com> writes:
On Friday, 28 August 2020 at 12:29:20 UTC, Simen Kjærås wrote:
 ....
Seems that these methods should be rooted out from Object, and placed in respective interfaces like: ----- interface Equatable(T) { bool opEquals(T value); } ----- Then it would be a lot more simple. People who want equality check, will implement interface with right type, for example Equatable!Object. - Alex
Aug 28 2020
parent Simen =?UTF-8?B?S2rDpnLDpXM=?= <simen.kjaras gmail.com> writes:
On Friday, 28 August 2020 at 13:35:43 UTC, Alexandru Ermicioi 
wrote:
 On Friday, 28 August 2020 at 12:29:20 UTC, Simen Kjærås wrote:
 ....
Seems that these methods should be rooted out from Object, and placed in respective interfaces like: ----- interface Equatable(T) { bool opEquals(T value); } ----- Then it would be a lot more simple. People who want equality check, will implement interface with right type, for example Equatable!Object.
Yup, it's been proposed, but nothing's come of it yet. Here's Andrei's DIP on ProtoObject, which apparently is untouched for over two years now: https://github.com/andralex/DIPs/blob/ProtoObject/DIPs/DIPxxxx.md The main problem with solving this issue is doing so will break all code that uses the current system. This is clearly suboptimal. -- Simen
Aug 28 2020