Jump to page: 1 2
Thread overview
Feedback Thread: DIP 1042--ProtoObject--Community Review Round 1
Jan 10, 2022
Mike Parker
Jan 10, 2022
Elronnd
Jan 11, 2022
RazvanN
Jan 11, 2022
Elronnd
Jan 11, 2022
RazvanN
Jan 10, 2022
Elronnd
Jan 10, 2022
Bruce Carneal
Jan 10, 2022
Adam D Ruppe
Jan 11, 2022
RazvanN
Jan 13, 2022
Dukc
Jan 13, 2022
Dukc
Jan 13, 2022
David Gileadi
Jan 14, 2022
RazvanN
Jan 14, 2022
RazvanN
Jan 14, 2022
Adam D Ruppe
Jan 15, 2022
Adam D Ruppe
Jan 25, 2022
Mike Parker
January 10, 2022

Feedback Thread

This is the feedback thread for the first round of Community Review of DIP 1042, "ProtoObject".

THIS IS NOT A DISCUSSION THREAD

I will be deleting posts that do not follow the Feedback Thread rules outlined at the following link:

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

The rules are also repeated below. Recently, I have avoided deleting posts that violate the rules if they still offer feedback, but I'm going to tighten things up again. Please
adhere to the feedback thread rules.

The place for freeform discussion is in the Discussion Thread at:

https://forum.dlang.org/post/clkvzkxobrcqcelzwnej@forum.dlang.org

You can find DIP 1042 here:

https://github.com/dlang/DIPs/blob/2e6d428f42b879c0220ae6adb675164e3ce3803c/DIPs/DIP1042.md

The review period will end at 11:59 PM ET on January 24, or when I make a post declaring it complete. At that point, this thread will be considered closed and any subsequent feedback may be ignored at the DIP author's discretion.

Feedback Thread Rules

Posts in this thread that do not adhere to the following rules will be deleted at the DIP author's discretion:

  • All posts must be a direct reply to the DIP manager's initial post, with the following exceptions:
    • Any commenter may reply to their own posts to retract feedback contained in the original post;
    • The DIP author may (and is encouraged to) reply to any feedback solely to acknowledge the feedback with agreement or disagreement (preferably with supporting reasons in the latter case);
    • If the DIP author requests clarification on any specific feedback, the original commenter may reply with the extra information, and the DIP author may in turn reply as above.
  • Feedback must be actionable, i.e., there must be some action the DIP author can choose to take in response to the feedback, such as changing details, adding new information, or even retracting the proposal.
  • Feedback related to the merits of the proposal rather than to the contents of the DIP (e.g., "I'm against this DIP.") is allowed in Community Review (not Final Review), but must be backed by supporting arguments (e.g., "I'm against this DIP because..."). The supporting arguments must be reasonable. Obviously frivolous arguments waste everyone's time.
  • Feedback should be clear and concise, preferably listed as bullet points (those who take the time to do an in-depth review and provide feedback in the form of answers to the questions in the documentation linked above will receive much gratitude). Information irrelevant to the DIP or which is not provided in service of clarifying the feedback is unwelcome.
January 10, 2022
This does not retain backwards compatibility, because 'Object' can no longer be counted upon to be a supertype of all class instances.

Suppose I maintain library X, which does some work on Objects.  It simply treats Object as a top type (for classes), without using any special functionality.  Somebody else maintains library Y, which defines a few classes.  This DIP is approved, and the library Y maintainer decides they don't need hashes, so they redefine their classes in terms of ProtoObject rather than Object.

This is fine; hashing was not an advertised part of their API, and no one was using it anyway.  No one's code broke as a result of this.  I would nominally feel very comfortable making such a change and would not consider it a compatibility break.  _Except_ that, when a user tries to combine libraries X and Y, their code will break.

Hence, while this change does avoid breaking any code as such, it forces that breakage straight into userspace, and does so in a forceful fashion which does not really permit deprecation periods.  Moreover, I do not have any way to update library X in a manner which retains compatibility, as even if I switch from processing Objects to ProtoObjects, this breaks compatibility if I ever return such an object.

------------------------------

Suppose that, instead, we make 'Object' the empty top type as well as the default superclass.  This is a compatibility break, and is therefore much scarier.  But practically, there will be much less silent breakage, because problems will manifest where they are actually located.
January 10, 2022
'cmp' should not return -1 when it can not compare the specified classes.  It should not pretend to have a significant result when it does not.

The DIP text implies that the given ordering is total, but it is not.  Consider what happens when you compare two instances of ProtoObject, neither of which implements Ordered and at least one of which is non-null.

@nogc nothrow are overly strong attributes for a comparator.  It may very well be desirable to allocate when performing a complex comparison.  And what should a comparator do when it is unable to compare with another object?

toHash should take a seed value.
January 10, 2022

The cmp method of the Book class appears to be incorrect. For reference (per google) after 2006 ISBNs have 13 digits.

As discussed elsewhere, something like (ulonga > ulongb) - (ulongb > ulonga) should work.

January 10, 2022
This DIP is built from flawed analysis and provides a flawed solution that fails to fix the original problem and creates problems of its own. I went into detail in the pull request thread, so I won't repeat it all, but let me post something stronger than words:

https://github.com/dlang/druntime/pull/3665

There's no need to add anything nor break anything in druntime over attributes.

The monitor is a bit different, but a standard deprecation path could migrate that to opt-in composition. The DIP doesn't even acknowledge this possibility.
January 11, 2022
On Monday, 10 January 2022 at 14:32:54 UTC, Elronnd wrote:

> This is fine; hashing was not an advertised part of their API, and no one was using it anyway.  No one's code broke as a result of this.  I would nominally feel very comfortable making such a change and would not consider it a compatibility break.  _Except_ that, when a user tries to combine libraries X and Y, their code will break.
>

Are you referring to the case where a method from library Y returns
an instance of something that inherits ProtoObject (but not Object)
and then when the instance is passed to some_function in library X,
some_function might make assumptions that are false about the instance
(for example, assumes that the instance has a toHash)? Indeed, this is
a bit nasty, but what could be done to ease the transition: library Y owner
can put some deprecated toHash function in his classes that inherit
ProtoObject. Even better, we will provide some utilities [1] that make it
trivial to implement the methods that Object provides. Eventually, Object
should be deprecated and that will make everyone upgrade their code to
ProtoObject.


[1] https://github.com/dlang/phobos/pull/7049


January 11, 2022
On Tuesday, 11 January 2022 at 09:33:16 UTC, RazvanN wrote:
> Are you referring to the case where a method from library Y returns
> an instance of something that inherits ProtoObject (but not Object)
> and then when the instance is passed to some_function in library X,
> some_function might make assumptions that are false about the instance
> (for example, assumes that the instance has a toHash)?

Not quite.  I am referring to the case where library X uses Objects _without_ expecting them to have a toHash (because it predates ProtoObject; that is, it does not use templates); and library Y used to return Object instances, but then switches to returning ProtoObject instances.

This seems like a very reasonable change from library Y's perspective, because nobody was relying on any particular behaviour from its hash.  The issue is that this actually breaks compatibility, but that's far from obvious to library Y's maintainers.

Additionally, library X can not transition to using ProtoObjects without breaking compatibility.  If either of library X and library Y transitions to using ProtoObjects, code using them may break.  The only way forward is if _both_ transition.  This is why I don't think the DIP delivers on its promise of backwards compatibility.  Is it worse to pin your compiler version than to pin a substantial subgraph of your dependencies, when you can reasonably expect the former solution to result in less time spent on old versions?

If instead we make it so that Object is the empty root object, then more code will be broken, but it won't happen that a change in library Y can break library X (modulo templates, of course).
January 11, 2022
On Monday, 10 January 2022 at 17:09:59 UTC, Adam D Ruppe wrote:
> This DIP is built from flawed analysis and provides a flawed solution that fails to fix the original problem and creates problems of its own. I went into detail in the pull request thread, so I won't repeat it all, but let me post something stronger than words:
>
> https://github.com/dlang/druntime/pull/3665
>
> There's no need to add anything nor break anything in druntime over attributes.
>
> The monitor is a bit different, but a standard deprecation path could migrate that to opt-in composition. The DIP doesn't even acknowledge this possibility.

Whatever we will modify in Object is going to be a breaking change.
A change of this magnitude would impose a version bump of the
language (let's not discuss here if that is desirable or not, but,
such changes come with large overhead in terms of migration).

ProtoObject, however, provides a less disruptive alternative where
users can just opt-in. If you want to get rid of the unnecessary
bloat of Object, fine, you can just inherit ProtoObject; if you
are fine with using Object you can continue doing that. We end
up with having the best of both worlds, depending on one's need.

I fail to see how deprecating and breaking code is a better alternative
than providing a clean and flexible alternative.
January 11, 2022
On Tuesday, 11 January 2022 at 12:35:33 UTC, Elronnd wrote:
> On Tuesday, 11 January 2022 at 09:33:16 UTC, RazvanN wrote:
>> Are you referring to the case where a method from library Y returns
>> an instance of something that inherits ProtoObject (but not Object)
>> and then when the instance is passed to some_function in library X,
>> some_function might make assumptions that are false about the instance
>> (for example, assumes that the instance has a toHash)?
>
> Not quite.  I am referring to the case where library X uses Objects _without_ expecting them to have a toHash (because it predates ProtoObject; that is, it does not use templates); and library Y used to return Object instances, but then switches to returning ProtoObject instances.
>
> This seems like a very reasonable change from library Y's perspective, because nobody was relying on any particular behaviour from its hash.  The issue is that this actually breaks compatibility, but that's far from obvious to library Y's maintainers.
>
> Additionally, library X can not transition to using ProtoObjects without breaking compatibility.  If either of library X and library Y transitions to using ProtoObjects, code using them may break.  The only way forward is if _both_ transition.  This is why I don't think the DIP delivers on its promise of backwards compatibility.  Is it worse to pin your compiler version than to pin a substantial subgraph of your dependencies, when you can reasonably expect the former solution to result in less time spent on old versions?
>
> If instead we make it so that Object is the empty root object, then more code will be broken, but it won't happen that a change in library Y can break library X (modulo templates, of course).

I think that the fundamental idea here is that when the library owner
switches from Object to ProtoObject that is a breaking change that should
be advertised, because he is changing the API. The library owner should
either release a new major version or provide the same utilies as Object.
The idea here is that you do not know what your users are doing; even without
library X, the user can simply call toHash because Object used to have it.

January 13, 2022

On Monday, 10 January 2022 at 13:48:42 UTC, Mike Parker wrote:

>

Feedback Thread

This is the feedback thread for the first round of Community Review of DIP 1042, "ProtoObject".

I still think that requiring comparisons and toHash functions to be @nogc is too strict. It is true that an ideally they should always be such, but the new interfaces should also be usable by not-so-successful code. It's likely the users will just continue to use the old object. Or worse, they will hack their functions to be @nogc with @trusted mallocs or allocas. Classes are not used much in @nogc scenarios anyway, so it's not a big loss that hashableObject.toHash() does not guarantee no gc.

I do think pure and @safe are justified in the new interfaces. Those are attributes that the large majority of code already has or can easily have. For nothrow, not sure. OTOH it isn't a breaking change and can be worked around with try{/*former implementation*/} catch (Exception) assert(0), OTOH there is still risk of continued usage of old Object or using the mentioned workaround carelessly.

What does interface Stringify look like? Didn't spot that in the DIP.

The _cmp function immeditely returns -1 if the first argument is not Ordered. Why? I'd expect it to first try casting the second argument to Ordered and use it's cmp against the first argument.

The Implement* template mixins are not worth including in this DIP. The users aren't worse off without them that with the present Object default implementations - it's dead easy to ape them. You might want to include simple functions that do what Object does (but no function for cmp please - Object just throws an exception), but that's it. I'm not saying the template mixins are a bad idea, just that they are orthogonal to what this DIP proposes.

>

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.

I agree with the "only if" but not with the "if" part here. Hash is only 32 bits long on 32-bit platforms, meaning a hash collision with only 64000 different values if using a perfectly chaotic hash function. For chaotic hashes, strictly no hash collisions is impossible on 32 bits and probably way too difficult in practice even on 64 bits.

« First   ‹ Prev
1 2