Partial fix for issue# 9769 (templated opEquals)

[jmdavis]

Okay. I'm taking another stab at this. Previously, compiler bugs made this not work, but at least some of those have been fixed, and it's working for me locally on FreeBSD. Hopefully, the autotester won't choke on it on Windows or whatnot.

This makes it so that the global opEquals function is templated. It is
therefore now possible for classes to implement opEquals without
overiding Object's opEquals method, including giving it different
attributes (such as const).

However, the hack version of the global opEquals which takes const
Objects and casts away const has been left for the moment, and the
opEquals on Object has not been touched. At minimum, before the hack
version can go away, work will need to be done on the other classes in
object.d to make them handle const properly. Also, the opEquals on
Object will probably have to go away before that if we don't want to
break code, as it's the only reason that const Objects have been able to
be compared before now, and until types stop using Object's opEquals,
that's still the only way that they'll be able to be compared. But at
least with these changes, it'll be possible for classes to be compared
without it if they change their signatures for opEquals to not take
Object.

[DmitryOlshansky]

Ow come on use nested static foreach:

foreach(T1; AliasSeq!(T,U, const T, const U))
foreach(T2; AliasSeq!(T,U, const T, const U))
{ ...
}

[DmitryOlshansky]

@jmdavis Ping!

[jmdavis]

Okay. I fixed the first one so that TypeTuple is used with a foreach on the types (we can fix it so that core.internal.traits uses AliasSeq in another PR), but I don't see how to make the other two use foreach like you suggest. The comparisons are combinatorial in nature, that's true, but the results aren't. The true/false argument gets in the way. There might be a way to do it, but I can't think of anything reasonable at the moment.

[jmdavis]

Okay. I updated the PR to fix the merge conflict, and I added the full set of attributes onto the constructors and opEquals of the test classes for good measure, but they can't be added to the unittest block, because TypeInfo's opEquals doesn't have them, and TypeInfo's opEquals is used in the free function version of opEquals.

[MetaLang]

LGTM, but I agree with Dmitry's suggestion to use foreach loops in the tests.

[marler8997]

Is the goal here only to templatize the opEquals global function? For some reason I thought that the end goal was to templatize the virtual method on the Object class. Was I mistaken?

[jmdavis]

LGTM, but I agree with Dmitry's suggestion to use foreach loops in the tests.

I really don't see how. As I said before, while the comparisons themselves may be combinatorial, the results are not.

Is the goal here only to templatize the opEquals global function? For some reason I thought that the end goal was to templatize the virtual method on the Object class. Was I mistaken?

The end goal is to remove opEquals from Object completely. It's not possible to templatize a virtual function, and as soon as the base class function has a particular set of attributes, all of the derived classes are stuck with them.

Templatizing the free function, opEquals, makes it so that opEquals on a class can have whatever attributes it wants and so that it doesn't have to take Object specifically (instead, it'll probably take whatever your base class is), but ultimately, you're still going to end up with a non-templatized opEquals in your base class that derived classes are going to have to override just like you would with Object's opEquals now - except that then you have control over what the attributes are, whereas now everyone is stuck with whatever was chosen for Object, which can't possibly be a good fit for all uses cases no matter what they are.

[marler8997]

When object.opEquals is removed, how do you ensure that this doesn't result in a recursive call to this template again?

[jmdavis]

Hmm. Good catch. There probably should be a check that the type has opEquals. Certainly, we're not trying to use UFCS for anything here, and having it kick in would be problematic.

[jmdavis]

Okay. It now checks that both of the types being compared have a member named opEquals in addition to checking that the result of calling it is bool like it did before.

[marler8997]

It's been a long time, but I looked over this again and I think this could still result in a recursive call.

class Foo
{
    bool opEquals(int x);
}

In this case, Foo has an opEquals member, however, if you call foo1.opEquals(foo2), it will recursively call opEquals here. I think we could force it to call the classes opEquals function with this:

return __traits(getMember, lhs, "opEquals")(rhs) && __traits(getMember, rhs, "opEquals")(lhs);

[marler8997]

Update. I tried to see if this would in fact result in a recursive call and it didn't seem to. My guess is that if an object contains the member opEquals, and you call obj.opEquals, then it will ALWAYS call the member function even if the arguments are invalid (it will never fall back to calling a UFCS method). Can anyone confirm this? I attempted to find this in the spec but was unsuccessful.

[jmdavis]

It is my understanding that UFCS is only ever attempted if there is no member function with that name. The spec implies that when it says

When UFCS rewrite is necessary, compiler searches the name on accessible module level scope, in order from the innermost scope.

but it isn't very specific about it. There was a complaint about it recently in D.Learn, because someone wanted to overload a range's front with a front function that took arguments, and it didn't work. So, all the evidence that I've seen seems to indicate that UFCS is only attempted when it needs to be attempted, which presumably reduces symbol look-up times (since it only has to look at the member functions if there's one with that name), and it avoids hijacking when someone screws up calling a member function.

[marler8997]

Created an issue to clarify this behavior in the spec https://issues.dlang.org/show_bug.cgi?id=18523

So long as this is the correct behavior, the change in the PR looks correct.

[marler8997]

Is there no way that the rhs.equals(lhs) can be removed?

[jmdavis]

Is there no way that the rhs.equals(lhs) can be removed?

It's there to ensure commutativity, and the code immediately above it (which doesn't show up in the diff) only needs lhs.opEquals(rhs), because it's for when the types of the underlying objects are the same. So, rhs.opEqualn(lhs) is only needed when the underlying objects are different types (or in CTFE due to a bug with typeid).

Andrei explains the reasons for why opEquals is written the way it is in TDPL, and this PR does not try and change how opEquals fundamentally works. It's just making it so that opEquals doesn't have to be implemented in Object or override the implementation in Object. The basic logic is the same as it's been for years.

[marler8997]

Is the ! character supposed to be here?

[jmdavis]

Yes. The whole point of this horrific overload is to cast away const when opEquals is called on a const object to allow for const objects to be compared. It's a hack that opens the door for badly behaving opEquals implementations breaking const, but without it, there's no way to compare const objects, because Object's opEquals isn't const.

With the tempaltized opEquals, if the opEquals declared on a base class is const, then it'll call the main opEquals overload without needing this one, and const won't be cast away. And when opEquals on Object is deprecated, we can deprecate this overload. Because lhs and rhs are const, the ! means that this will only compile if the member function opEquals being used isn't const. In the long run, once this overload is gone, such a comparison won't even be legal, but we can't do that as long as the opEquals on Object is still what's being used.

[jmdavis]

Pinging @WalterBright because this is potentially related to SafeObject, so he'll probably want to see it.

[dlang-bot]

Thanks for your pull request, @jmdavis!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	9769	enhancement	Remove opEquals from Object

[wilzbach]

I just went through the history of this PR and its predecessors and it's really a shame that after several years we are still no step closer to const opEquals or (@safe opEquals).

I don't see anything blocking this and heck, it's only a "partial" fix with a few hacks, but we need to start somewhere...

The only thing (apart from the two nits) is that this change could be accompanied by a changelog entry, but then again, it might be a good idea to wait until it's fully fixed (and the other opCmp and friends have been fixed too)

[wilzbach]

We have made bad experiences with version(unittest) in the past and I think Andrei doesn't really like version(unittest) anymore. There was an effort for StdUnittest (and CoreUnittest), but it seems that didn't work out either, so I suggest to move this into the unittest block.

[jmdavis]

When I originally implemented this, I don't think that it worked to have templated functions inside a unittest (IIRC, it only worked with one instantiation, making it pointless). Fortunately, that's been fixed. I'll update it accordingly.

[MetaLang]

Testers are green. @andralex let's pull the trigger.

[andralex]

I have a different plan for solving this problem, on my long "DIPs to write" list. I'll summarize it below.

Currently Object fulfills two roles simultaneously:

• The root of all class types.
• The implicit base of newly defined classes - i.e. if no inheritance is present, : Object is assumed.

A key observation is that these two roles need not be fulfilled by the same type. And here comes the idea - we introduce ProtoObject as the base of Object and the root of all types. Object with its (problematic) methods remains the default base, so we don't break existing code. But new code can inherit ProtoObject, which defines no methods to saddle everybody else. We can even design ProtoObject to not have a monitor. The possibilities are endless because we have Object as a backward compatibility buffer.

Going the ProtoObject route renders efforts like this PR unnecessary. @jmdavis would you want to pen the DIP together?

[marler8997]

I really like the idea. I'm not seeing the connection between the name "ProtoObject" and "the root of all objects". Heres some other ideas for names because you know we got to make that "bike shed" look pretty:)
NullBase
SuperBase
VoidBase
Void
RootType
RootClass
ClassRoot
...

[jmdavis]

I'm certainly up for helping to work on a DIP for a replacement for Object, though it does concern me if we forever keep that cast that removes const from Object when doing comparisons, since that breaks the type system if anyone does something in opEquals to mutate the object (e.g. lazy initialization), and the fact that that cast is happening is hidden makes it's not all that hard for an ignorant programmer to get undefined behavior - and I don't see how to fix that without either removing opEquals from Object or screwing over const Objects by making comparison illegal. I'm not sure that it's actually much a problem in practice, but having something built-in like that which is breaking the type system isn't exactly ideal. Now, mucking with Object will probably be easier if we have some kind of ProtoObject, and we may just be stuck with that problem with opEquals in the long run because of how long it's been there, but I sure don't like it.

In principle, I'd actually like to see something like ProtoObject added where stuff like the monitor could be added on via attributes or the use of synchronized or something and then outright remove Object from the language, but I don't know how we'd actually do that cleanly even though most code really doesn't care about Object. Defining some kind of ProtoObject would be the first step though and would be valuable even if we couldn't actually nix Object as would be ideal.

Depending on what we wanted to do, what this PR does could actually still be part of the solution, but it's just one small step in what needs to be done to fix the overall problem. So, even if the plan were specifically to remove opEquals, opCmp, toHash, and toString from Object rather than work towards providing an alternative or replacement to Object, this PR is not enough on its own. At best, it's just the beginning.

[andralex]

@marler8997 per https://www.merriam-webster.com/dictionary/Prot: "a: first in time (protohistory)
b: beginning : giving rise to (protoplanet) [...] first formed : primary (protoxylem)" - very good fit!

@jmdavis:

I'm certainly up for helping to work on a DIP for a replacement for Object, though it does concern me if we forever keep that cast that removes const from Object when doing comparisons, since that breaks the type system if anyone does something in opEquals to mutate the object (e.g. lazy initialization), and the fact that that cast is happening is hidden makes it's not all that hard for an ignorant programmer to get undefined behavior - and I don't see how to fix that without either removing opEquals from Object or screwing over const Objects by making comparison illegal.

(This must be the longest sentence I've read in a while. If they put their minds together, Marcel Proust and David Foster Wallace would still have nothing on you.)

The DIP would not be for replacing Object. I, too, think we should close this PR because it casts const away.

[andralex]

if should be flush with the left edge

[andralex]

static if (is(typeof(lhs is rhs)) is more direct and handles cases like comparing an immutable object with a mutable object.

[jmdavis]

But it doesn't handle when one type is derived from another.

[andralex]

Has the option of adding a const overload of opEquals been explored?

[jmdavis]

I don't remember. The decision had been to remove opEquals and the other three functions entirely. It's just that we haven't been able to do that, because the built-in AAs use them. The work to templatize them has to be finished first. The other issue of course is that while we decided that the way to fix the attribute problems with these functions was to remove them from Object, almost no work has actually gone towards that. AFAIK, this and the work that's been towards fixing the AAs is it. It was a decision made without the work to back it up. And while I originally did the work for this PR years ago, it was blocked due to a compiler bug, and once that was fixed, and I recreated the PR, it just sat here. Adding a const overload wouldn't solve the overall attribute problem to allow classes to define these methods with whatever attributes were appropriate, and we were trying to solve that, not const specifically. As such, I'm not sure that overloading on const was ever really considered, though since one of the goals was to allow for classes to be able to have a non-const opEquals so that they could do stuff like lazy initialization, adding a const overload didn't really fix the problem even when just looking at const.

If we ignore the overall attribute problem and just look at const, then adding an overload for opEquals which is const sort of fixes the problem but sort of doesn't (even ignoring the issue of allowing classes to lazy initialize members). If a type just overrides the const overload, and == or != is used on a mutable object, then the mutable overload in Object would be called and not the const override. The same goes if a mutable override was declared, and a const object were used (in that case, you'd just be using the wrong overload instead of breaking the type system like we do now). Making the mutable Object.opEquals call the const one would fix the case where a derived class just declared the const override, but it wouldn't fix the case where the derived class just declared the mutable override. If we combined that with this template solution, then as long as folks weren't comparing Object, that wouldn't be a problem, because the derived class versions would be called directly, but any time that someone (like the current AA implementation) compared Objects, then there would be bugs. The result of that approach might be better than what we have now, because it wouldn't break the type system, but it would be buggy for any class that didn't override both overloads of opEquals, and it leaves the problem with allowing stuff like lazy initialiazation, but to fix that, opEquals has to be removed from Object, or we need a different root object for such a class. So, if we add ProtoObject, adding a const overload to Object then just solve the undefined behavior problem, but it causes other bugs unless the compiler forces you to override both overloads, which would be annoying but at least wouldn't be buggy.

The other question is how to then go about deprecating the current behavior. We'd basically have to add some sort of deprecation warning to the compiler about it, and then later change druntime, and break any code that hadn't been updated to have a const overload of opEquals. But if we were adding that plumbing to the compiler to give a deprecation warning, we could just make it permanently give an error about overriding only one of the two overloads. That's not exactly pretty given that in principle, it should be possible to only declare the const version if you don't need them to be different, but it would solve the problem with undefined behavior while making it possible to compare const Objects.

[andralex]

The most effective is to go with a DIP on ProtoObject and then make only non-breaking improvements to Object. ProtoObject essentially makes Object legacy code so its issues become moot.

[jmdavis]

The DIP would not be for replacing Object.

Yes. I get that it would be for adding a type underneath it, but if that were done, it then becomes possible to consider removing Object as some future step, though given the fallout of that, it would probably never happen. But what you're suggesting moves us forward regardless of whether we later look at removing Object, and it's not incompatible with removing Object at some point in the future. That could be a completely separate decision. We'd basically be looking at adding what Object should have been while still leaving Object on top of it.

I, too, think we should close this PR because it casts const away

It casts const away, because it can't do otherwise without breaking code. The idea was that with this PR, folks could start defining opEquals however they wanted, and then later, we could deprecate and then remove Object's opEquals, and the part here that casts away const would go away. But as long as Object.opEquals remains, it does have all of the problems that go with overloading Object.opEquals on const and derived classes which only overload one of the two. So, his solution arguably doesn't make sense if we're not planning to actual remove Object.opEquals in the future, regardless of the fact that it currently casts away const.

If we go forward with the ProtoObject idea, then we'll need to revisit this, because there will need to be a templated solution for comparing objects derived from ProtoObject using their versions of opEquals, but we can go there once that DIP is done. And if we go there, we can consider adding a const overload for Object.opEquals (with the appropriate compiler help as discussed above), which could then solve the undefined behavior problem even if it's in a somewhat ugly way - though at that point, anyone who wanted to avoid having to declare both overloads could just derive from ProtoObject instead.