-
-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rethinking the default class hierarchy #219
Conversation
|
Thanks!. A few points:
Please refer to the template. |
|
Done. Thanks! |
DIPs/1NNN-RA.md
Outdated
| | Stringify | string toString(); | | ||
| | Hash | size_t toHash(); | | ||
| | Ordered | int opCmp(const Object); | | ||
| | Equals | bool opEquals(const Object); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reconsider the naming of the interfaces. Three of these are verbs (Hash can be alternatively seen as a substantive), one is an adjective. I think adjectives and substantives are okay but verbs, not so much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those I suggest something like: Stringable, Hashable, Equatable.
2 inconsistencies:
- Here Ordered has method
opCmpbut the definition below hascmp. - Here Equals has method
opEqualsbut the definition below hasequals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, those are typos.
The methods should have the name opCmp and opEquals
|
|
||
|
|
||
| ### Breaking Changes and Deprecations | ||
| No breaking changes are anticipated because this provides an alternative for users, not a complete redesign of the class hierarchy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that introduction of ProtoObject will not cause breakage. However, adding attributes to Object:s member functions will. Any classes doing, for example, an unsafe comparison, will break.
Unless you meant that the Object will not implement the four interfaces you're suggesting to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object will not implement the four interfaces. It won't be changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, that clarifies a lot. Though, did the DIP say it to did I just read too quickly?
| interface Equals | ||
| { | ||
| const @nogc nothrow pure @safe scope | ||
| int equals(scope const ProtoObject rhs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the nothrow and @nogc attributes worth it here? Sure you can implement most comparisons easily enough with those attributes, but there are likely classes around that reliy on exceptions and the gc for this. The migration would be painful for such code. You rarely use classes without the gc anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for equality should not throw, nor use the GC. I don’t see a problem here. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was assuming object would implement Equals but if it does not, it's not so much a problem. Old code can simply not implement Equals. I guess at least the nothrow is justified then.
| interface Hash | ||
| { | ||
| const @nogc nothrow pure @safe scope | ||
| size_t toHash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concerns as for the equals attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Computing the hash of something (even Object) should not throw, nor use the GC. I don’t see a problem here. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since old code won't break unlike how I assumed, yeah I guess nothrow is justified. I still do question @nogc though. An ideal hash function can work in stack only, but it may sometimes be easier to use the heap for the hash calculations. Requiring no garbage collection might set the bar a bit too high.
| { | ||
| if (p1 is p2) return 0; | ||
| Ordered o1 = cast(Ordered) p1; | ||
| if (o1 is null) return -1; // we can't compare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, if (Ordered o2 = cast(Ordered)p2) return -o2.cmp(p1) ?
| interface Equals | ||
| { | ||
| const @nogc nothrow pure @safe scope | ||
| int equals(scope const ProtoObject rhs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not 'bool'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please document why int or change too bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.opEquals returns bool:
https://dlang.org/phobos/object.html#.Object.opEquals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will be bool
DIPs/1NNN-RA.md
Outdated
|
|
||
| Again, we provide the user with default implementations for a hashing function in the form of `ImplementHash` and `ImplementHashExcept`. | ||
|
|
||
| Implementations of `Ordered`, `Equals` and `Hash` must agree with each other. That is, `a.cmp(b) == 0` if and only if `(a == b) && (a.toHash == b.toHash)`. It's easy to accidentally make them disagree by mixing in some of the interface implementations and manually implementing others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Ordered should inherit from Equals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case Ordered should be renamed to Comparable then. Comparable means that something can be compared to something. It can be less, equal or bigger, while Ordered in my understanding, is something that is before something, at same level, or after it.
|
I have made some minor tweaks here: https://github.com/Robert-Aron293/DIPs/pull/1 |
|
The names chosen may conflict with existing libraries or user code. It may be needed to prefix the names of the interfaces into something like |
|
The DIP needs to contain a discussion about the performance impact of the interfaces proposed (e.g.
|
| * The `opCmp` and `opEquals` objects need to take `const Object` parameters, not `const ImprovedObject`. This is because overriding with covariant parameters would be unsound and is therefore not allowed. Using the weaker type `const Object` in the signature defers checks to runtime that should be done during compilation. | ||
| * Overriding `opEquals` must also require the user to override `toHash` accordingly: two objects that are equal, must have the same hash value. | ||
| * `opCmp` reveals an outdated design and implementation. Its presence was historically required by built-in associative arrays, which used binary trees needing ordering. The current implementation of associative arrays uses hashtables that lift the requirement. In | ||
| addition, not all objects can be meaningfully ordered, so the best approach is to make comparison opt-in. Ordering comparisons in other class-based languages are done by means of interfaces, e.g. [`Comparable<T>` (Java)](https://docs.oracle.com/javase/7/docs/api/java/lang/Comparable.html) or [`IComparable<T>` (C#)](https://msdn.microsoft.com/en-us/library/4d7sx9hd.aspx). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This DIP might need to explain why Java and C# have a generic interface whereas this proposes a non-generic interface Ordered and runtime type checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java's Object.equals method predates generics. They later introduced https://docs.oracle.com/javase/8/docs/api/java/lang/Comparable.html, which is a good prior art to cite as evidence backing up this DIP.
|
Your abstract justification is virtually all factually false and moreover, even if it were true, ProtoObject does nothing to change that. Fixing most these issues is trivially easy with existing Object, even without breaking changes. (And even if you were to do the breaking changes, they'd probably be trivial to fix.)
Not true. There's COM and C++ classes that don't.
"fist" typo
Technically, those properties don't work with Object per se, but since you can add them to other interfaces or child classes (remember, you can always tighten restrictions not present in the base), their presence in Object are irrelevant. And since ProtoObject also requires you to add interfaces or child classes, it doesn't actually change anything. The rationale is much better, actually getting into it. Though I still don't see the need for the new thing. Like it could just remove factory() and the mutex from the current Object. Fixing this breakage in user code would be utterly trivial, you just add the explicit opt in during the deprecation period. The toString thing is already done in the Throwable class in today's druntime. Today's writeln already uses it if it can. The gc one is easily implemented in terms of the delegate one. It is fair to say the problem of having two is if you override one but not the other, the result can change just based on which function calls it. There's also existing problems with the attributes being dependent on the passed delegate, which is currently difficult to express in terms of an interface (you can with overloads but it is a pain and then you've got even more override potential trouble.) I see no toString in any list of the ProtoObject interfaces, so I guess that just conveniently sidesteps it. It is true, you can just DBI it, but then you have the troubles of that like a seemingly minor typo leads to it just mysteriously not working. But regardless, if you're sidestepping it anyway, you haven't really gained much over the status quo. opCmp and opEquals missing const scope is legit, not much we can do about that (except just.... add it and let people's overrides catch up). opHash... meh, status quo doesn't really hurt anything, but the fact it must agree with opEquals is a legit pitfall (though the default implementation of opHash could handle this). factory indeed sucks, I'd be in favor of removing it entirely. Keeping it there but having a workaround to avoid it doesn't gain as much as just killing it. Well, that's my objection to this thing existing at all. There's better approaches. But if we are going forward with it anyway, here's some stuff to improve the dip by itself:
I don't see how. Nothing in current Object is comparable to the problems described in the link. Note that current Object has no state aside from that mutex, and none of the interface methods affect that mutex. Perhaps an example would be illustrative.
ALL classes? Or just extern(D) classes? What about COM and extern(C++)? The reason they aren't compatible right now is a vtable slot incompatibility (D things put TypeInfo in slot 1, they do not), so I imagine they would continue to be separate. I'm inclined to agree with the others that this is probably over restrictive (personally, I find the lack of these in current Object to be a feature, not a bug. You can always add more later, but you cannot take away what is already restricted.) But in practice it'd probably be fine anyway.
Why does this function exist? Same on __equals. And the existence of these functions is what actually causes the pain of equality and whatnot in I'll go ahead and tell you: it is to inject a An alternative is to template the call to __cmp, so it uses the concrete static type passed. This avoids the dynamic casts and respects the interface given, attributes and all. I think another saoc participant is looking at this (though I don't know if they're doing these functions in particular). See, this is why I'm against this whole ProtoObject push. It is based on a flawed analysis from the beginning. But anyway back to assuming it is the way. Consider the following: You get a spew of errors. But notice the root: demo.d(11): Error: The module, not the class. Change it to: And now it complies without trouble. The covariant parameter thing is a legit problem still - I had to take If we had a template top-level opEquals: Well well, we have null checks, we can overload (not override) on the What happens if you compared to Object? Well, then the template would use the common base type - Object - and be unable to infer the attributes. But that's fine, if you want the more strict interface, just use the more strict interface in your static type; pass Derived instead of Base. Then it all just works. So the real problem is object.opEquals, NOT Object.opEquals. If we're switching to ProtoObject.... maybe it should actually fix the real cause while you're at it. Change those module-level functions to take the types actually passed to it without the intermediate implicit cast. This eliminates the disgusting internal dynamic cast, allows user-defined attributes to be added as-needed, maintains the convenience null check at the boundary, and just as the cherry on top, works with normal Object-derived things too. On a similar vein, I think defining the Then you say Much nicer to implement, and the static type is now meaningful - you can't compare it against a I have a hard time thinking of a case where you'd want arrays of "comparable" or "orderable" objects that cannot actually be compared or ordered against each other since they're different things. But I suppose I might be wrong here and you do want to sort a And with the template, you might be ordered against something that isn't a class at all, like
Is there any way to enforce, or at least encourage that through design? If not, so be it, but it would be nice. But again this seems to not actually have significant improvements over the status quo. |
We have a PR that adds such utilities and so far there aren't any name clashes. |
What does that PR have to do with |
Nothing. You're right. Disregard my previous comment |
|
The interfaces can easily be defined in |
|
|
||
| In order to be able to compare two objects, we define the `Ordered` interface. | ||
| ```D | ||
| interface Ordered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this interface a templated one?
Smth like this:
interface Ordered(T = typeof(this)) {
... int cmp(.... T rhs);
}The __cmp hook can also be a template that will accept only types that extend Ordered iface, of same type.
This would make possible to declare a ordering relationship between two different types, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case when the interface is not known at compile time, the __cmp hook can try cast the object to a generic version of ordered: Ordered!ProtoObject, and if implemented, then do the comparisons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question for Equals interface.
| { | ||
| class Widget : ProtoObject, Ordered | ||
| { | ||
| mixin ImplementOrdered!("x", "y", (int a, int b) => a - b, "z", (int a, int b) => a - b + 1, "t"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth investigating java's builder api for comparators: https://docs.oracle.com/javase/8/docs/api/java/util/Comparator.html as an alternative, that offers fluent style api for declaring how to compare the objects.
|
I don't understand why these four interfaces ( |
The problem with Adam's approach is: a child class cannot implement these methods, pass around the parent of said class and then try to do any of these operations. At least without using something like TypeInfo to store function pointers. |
|
The parent doesn't implement the interface, so passing it is (correctly) a static type error. If you're using it, you need to use the base that actually implements the interface. |
|
Might be worth mentioning swift protocols which are similar to rust's traits approach. It would be nice if the mixins took alias parameters rather than strings if possible |
Yes, I was referring to the D classes.
Introducing
extern(D) classes
Few points here:
Users can use the interfaces provided in druntime or define other interfaces similar to those. If you want one that is less restrictive, you can just define it.
Interfaces take
We were thinking of something similar to this: |
|
@mdparker the DIP is ready for review |
|
The implementations of the global int __cmp(ProtoObject p1, ProtoObject p2)
{
if (p1 is p2) return 0;
Ordered o1 = cast(Ordered) p1;
if (o1 is null) return -1; // we can't compare
return o1.cmp(p2);
}Returning bool __equals(ProtoObject p1, ProtoObject p2)
{
if (p1 is p2) return true;
Equals o1 = cast(Equals) p1;
Equals o2 = cast(Equals) p2;
if (o1 is null || o2 is null) return false;
return o1.equals(o2) && o2.equals(o1);
}Returning |
If the intent is for these functions to use templates, then the DIP should be revised to reflect that. Currently, |
|
On Fri, Nov 12, 2021 at 07:44:56AM -0800, Robert-Aron293 wrote:
Yes, I was referring to the D classes.
The DIP text should be precise.
Introducing `ProtoObject` provides the users an alternative, while also avoiding the breakage in user code.
My big problem with this is that it trades long term pain for short term convenience. That's a bad deal.
1. yes, object.opEquals should use templates
We should probably just go ahead and make that change. It doesn't even
change the language.
3. yes, all functions(equals, cmp, hash, etc.) should use templates
Those other functions probably simply shouldn't even exist. You can just
call the method directly as needed - no need to do those ridiculous
dynamic casts at all.
Interfaces take `ProtoObject` because we don't want to force the users to use our interfaces. They can use ours or define other, according to their needs.
This gives up static type safety and provides no actual generic benefit
(you still cast to your interface, not to the user interfaces, so they
cannot actually define other things according to their needs).
Deleting these functions entirely would be more useful. Then the user
would define their own functions that work in their own interface.
|
Your solution will break existing code. Which apparently @andralex here wants to avoid. |
|
On Sun, Nov 14, 2021 at 12:50:45PM -0800, 12345swordy wrote:
Your solution will break existing code.
If removing the functions is not acceptable (and frankly, there's no
rational reason why not, since the migration is *utterly* trivial and
a one time investment for greater long term returns, whereas this
proposal avoids the small current cost in favor of a recurring long term
cost), you can also just leave them alone.
Their presence doesn't actually break anything.
|
|
@pbackus wrote:
Can we please admit that not all types that are ordered have a (total) linear order? If we want to take this seriously, please let's not misstep on the first meter. Ordering is difficult. What is intuitively an order can be a very, very general construct. TL;DR: Don't assume orderings are nice and ugly orderings are few and niche. |
DIPs/1NNN-RA.md
Outdated
| assert(a == [c, c, c]); | ||
| } | ||
| ``` | ||
| It fails because the it calls the non-safe `Object.opEquals` method in a safe function. In fact, just comparing 2 classes with no user-defined opEquals - `assert (c == c)` - will issue an error in @safe code: "`@safe` function `D main` cannot call `@system` function `object.opEquals`". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a Romanianism I often use too: repeated references to "it". English only tolerates short-range references to "it", and seldom twice in the same sentence. In addiotion, here the two uses of "it" refer to distinct things: first time it's about the compilation, second time it's about the function if I understand correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now fixed this and 2 other minor fixes from your comments in https://github.com/Robert-Aron293/DIPs/pull/1
|
Was brought here by a rant that was since deleted, only to find @adamdruppe 's :). I agree with his comment that DIPs need to be precise ("every statement in the DIP should be defensible in a court of law in front of a lawyer who'd make one million dollars if they ruined you, representing your vengeful ex"). A simple corollary is, whatever needs explanation and qualification, don't put it here, paste it in the DIP instead. As to what changes are acceptable and what aren't, my take is that if anything easier was possible (such as removing methods of Object), it would have been done by now because it's easiest implementation-wise. This DIP is immediately actionable and breaks a long-overdue stalemate. Anything and everything against it arguing X would be easier should mind the simple "but why hasn't X been done in the past 15 years?" comeback. |
|
Well, it is obvious that not many people have even correctly diagnosed the problem - this DIP (and all the associated dconf talks over the years) certainly fail to do so. I'll fix it myself over the Christmas break perhaps. |
No description provided.