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