www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Feedback Thread: DIP 1042--ProtoObject--Community Review Round 1

reply Mike Parker <aldacron gmail.com> writes:


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.


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.
Jan 10 2022
next sibling parent reply Elronnd <elronnd elronnd.net> writes:
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.
Jan 10 2022
parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
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
Jan 11 2022
parent reply Elronnd <elronnd elronnd.net> writes:
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).
Jan 11 2022
parent RazvanN <razvan.nitu1305 gmail.com> writes:
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.
Jan 11 2022
prev sibling next sibling parent Elronnd <elronnd elronnd.net> writes:
'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.
Jan 10 2022
prev sibling next sibling parent Bruce Carneal <bcarneal gmail.com> writes:
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.
Jan 10 2022
prev sibling next sibling parent reply Adam D Ruppe <destructionator gmail.com> writes:
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.
Jan 10 2022
parent RazvanN <razvan.nitu1305 gmail.com> writes:
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.
Jan 11 2022
prev sibling next sibling parent reply Dukc <ajieskola gmail.com> writes:
On Monday, 10 January 2022 at 13:48:42 UTC, Mike Parker wrote:



 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` `malloc`s or `alloca`s. 
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.
Jan 13 2022
parent Dukc <ajieskola gmail.com> writes:
On Thursday, 13 January 2022 at 17:18:47 UTC, Dukc wrote:

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



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


 I still think [snip]


Oh, I forgot my overall opinion. I guess it's positive. Yes, even 
now you can make your own object to accomplish mostly the same 
and yes, afterthought additions like this do make the object tree 
to look ugly. But the real value of this DIP is that we're going 
to have a standard "better object" instead of a dozen different 
ones in DUB packages.

Another opinion would have been to make a new object that 
inherits the old one, instead of vice-versa. But this option 
makes it possible to statically reject `opCmp` invocations, as 
opposed to throwing an exception at runtime. And we also get to 
kill the mandatory monitor without needing to break code.
Jan 13 2022
prev sibling next sibling parent reply David Gileadi <gileadisNOSPM gmail.com> writes:
Based on RazvanN's reply to Elronnd (quoted for context):


 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.


If this change leads each of my dependencies to make breaking changes 
then its backwards compatibility certainly isn't the "no breaking 
changes" claimed by the DIP. This effect on users of third-party 
libraries should be called out in the DIP's Breaking Changes and 
Deprecations section.

(I'm replying to the original post here because my original reply 
violated Feedback rules; sorry about that).
Jan 13 2022
next sibling parent RazvanN <razvan.nitu1305 gmail.com> writes:
On Thursday, 13 January 2022 at 17:57:24 UTC, David Gileadi wrote:

 Based on RazvanN's reply to Elronnd (quoted for context):


 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.


 If this change leads each of my dependencies to make breaking 
 changes then its backwards compatibility certainly isn't the 
 "no breaking changes" claimed by the DIP. This effect on users 
 of third-party libraries should be called out in the DIP's 
 Breaking Changes and Deprecations section.

 (I'm replying to the original post here because my original 
 reply violated Feedback rules; sorry about that).


I think that this case is being exaggerated here. I find it hard 
to believe
that people write libraries where they simply type a parameter as 
being
Object. Most library code is templated which means that the 
received types
can be either class, struct or basic types. As such, I would 
expect that
folks would at least test for the existence of the likes of 
toHash, opCmp etc.
before using them.

My expectation is that, in most cases, updating to ProtoObject 
will not incur any breaking changes, however, there might be a 
few situations where users (arguably, wrongfully) were using the 
builtin object methods of class instances from library X. In this 
scenario, you will end up with a compile error in user code after 
upgrading. I would argue that this is for the best if the library 
owner did not intend to expose those facilities.
Jan 14 2022
prev sibling parent RazvanN <razvan.nitu1305 gmail.com> writes:
On Thursday, 13 January 2022 at 17:57:24 UTC, David Gileadi wrote:

 Based on RazvanN's reply to Elronnd (quoted for context):



 If this change leads each of my dependencies to make breaking 
 changes then its backwards compatibility certainly isn't the 
 "no breaking changes" claimed by the DIP. This effect on users 
 of third-party libraries should be called out in the DIP's 
 Breaking Changes and Deprecations section.

 (I'm replying to the original post here because my original 
 reply violated Feedback rules; sorry about that).


I think that this breaking change, although possible, has minimal
chances of happening. The reason I believe this is that libraries
typically write templated code to be able to deal with classes,
structs and templated types. As a consequence, you cannot blindly
call the likes of opCmp, toHash etc.

Even if this scenario is met (which I think it is highly 
unprobable),
the failure will be clear and arguably it would be justifiable 
because
the defaults for opCmp/toHash simply return the address of the 
class.
Jan 14 2022
prev sibling next sibling parent Adam D Ruppe <destructionator gmail.com> writes:
 At best, class factory registration should be opt-in because 
 only a small number of classes (none in the standard library) 
 require the feature


If making factory opt in is the best choice (which it is), why 
does the DIP not do this?
Jan 14 2022
prev sibling next sibling parent Adam D Ruppe <destructionator gmail.com> writes:
This old bug came up overnight:

https://issues.dlang.org/show_bug.cgi?id=15246

ponce said: "Hopefully fixed in ProtoObject! ;)"

Dr. Alexandrescu said in reply: "Yah, the DIP should definitely 
amended to include ~this() on the list of functions to look at. 
Assigning to Razvan so we don't forget about this."


Of course, ProtoObject wouldn't fix this anyway - just like with 
the other attributes, they have *nothing to do with Object* so no 
amount of changes there would actually fix the problem - but now 
we have yet another thing this DIP was supposed to fix that was 
completely unaddressed.

This DIP must be rejected. There is no hope to salvage it; it is 
completely broken from top to bottom.
Jan 15 2022
prev sibling next sibling parent reply Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= <ola.fosheim.grostad gmail.com> writes:
The DIP is jumping into details without acknowledging that the 
class concept is a high level construct related to OO 
methodology. The design should be rooted in a rationale based on 
specific OO modelling principles (maybe read conference  
proceeding for ideas) and usability.

Since it is a high level concept you essentially should strive to 
make it easy on the programmer and put the burden of optimization 
on the compiler. This must be discussed. For instance, an 
implementation can remove the mutex if it isnt used. It can even 
do so in separate compilation by putting the mutex at a negative 
offset. If you need more details, feel free to ask.

All other OO languages in the Simula family use «object» as the 
root (if they provide one). Deviating from this norm will cause 
confusion and create friction and annoyance among people who have 
an OO background.  The motivation for deviating from this is not 
convincing.

It is desirable to have one root, yet, you propose 3 roots? This 
needs more motivation. D needs to be made simpler, not more 
convoluted.

I am not getting into the specifics of interfaces, but as 
implemented they are to be avoided as they cost 8 bytes per 
instanced object per interface. So it is better to adorn the root 
object with abstract virtuals which are free. If you want to 
force interfaces on developers then you need to provide an 
interface mechanism that is usable (the current one is too 
expensive).

The list of popular languages at the end does not seem motivated. 
Why only these commercial languages and why listing them? By not 
referencing more academic OO resources the reader will get the 
impression that you did not do a proper survey and that the DIP 
as written is more of an early sketch than a final design.
Jan 15 2022
parent Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= <ola.fosheim.grostad gmail.com> writes:
One more thing: a root object redesign must take into account 
adding a reference count and discuss how that can be merged with 
a monitor mutex into a single field.

Not covering this would be a serious omission given the desire to 
provide an alternative to GC ownership.
Jan 15 2022
prev sibling parent Mike Parker <aldacron gmail.com> writes:
On Monday, 10 January 2022 at 13:48:42 UTC, Mike Parker wrote:


 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.


This round of review is now complete. Thanks to everyone who left 
feedback.
Jan 24 2022