Improved semantics for destroy() #2126
Conversation
|
Thanks for your pull request, @andralex! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
| Detailed behavior of `destroy(obj)` depends on the type `T` as | ||
| follows: | ||
|
|
||
| $(UL $(LI If `T` is a primitive type, a `struct` types that does not |
| references any other objects. It does $(I not) initiate a GC cycle or free | ||
| any GC memory. | ||
| Destroys an object by means of calling its destructor, if present. For | ||
| types `T` that define a destructor, `destroy` then also fills the |
There was a problem hiding this comment.
probably best to use "... that have a destructor" here since that's the verbage used in the following "NOTE".
|
This is better than my prior understanding of @andralex's specification as, at least, reference types and value types are treated uniformly. However, I still don't understand why having a destructor or not having a destructor should result in different behavior. And, it looks like we'll have to do something about Higgs. cc @sbstp See the Jenkins "details". |
|
I'm not the author of Higgs, you should probably cc @maximecb. |
|
Oha @JinShil. So, it's been a long time since I've looked at this code, but this might be the source of your problem: https://github.com/higgsjs/Higgs/blob/master/source/runtime/gc.d#L914 |
|
no attempts of fixing bugs regarding the class deconstructor when it comes to attributes? |
Breaking HiggsI removed one destructor from Higgs in an attempt to make it compatible with this PR, but there are still 3 more that need attention: https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/object.d#L512 Alternate ProposalI propose decoupling destruction from re-initialization for all cases, and delegating the decision and method of re-initialization to the user. That puts the burden on the user to understand what they need to do and why, but I believe that's responsibility the user opts into when they choose to implement a destructor. Consider the following contrived examples. Method 1: Use a boolean flag to indicate whether or not the object has already been destroyed struct S
{
void* p;
bool destroyed = false;
this(size_t size)
{
p = allocateResource(size); // may not have the same semantics as `malloc`
}
~this()
{
if (!destroyed)
{
deallocateResource(p); // may not have the same semantics as `free`
destroyed = true;
}
}
}Method 2: Use a special state to indicate whether or not the object has already been destroyed struct S
{
void* p;
this(size_t size)
{
p = allocateResource(size); // may not have the same semantics as `malloc`
}
~this()
{
if (p == null)
{
deallocateResource(p); // may not have the same semantics as `free`
p = null;
}
}
}Method 3: Re-destuction is benign due to the semantics of the resource deallocation struct S
{
void* p;
this(size_t size)
{
p = allocateResource(size); // same semantics as `malloc`
}
~this()
{
deallocateResource(p); // same semantics as `free`
p = null;
}
}Learning from C#There may also be some wisdom to be found in C#'s dispose pattern documented here: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern. It is Microsoft's solution to basically the same problem. |
That's out of scope for now, and probably needs changes to be made in DMD. |
|
@JinShil the destructors in Higgs look reasonable. In what way are they broken? As a general disposition, we're going toward more, not less, safety. So the situation in which we make |
I believe it is their mere existence, causing the object to be re-initialized, that causes problems for the Higg's test suite. But, I need to look into it in more detail to be sure. |
|
@andralex How confident are you in this implementation? For some reason, noone from team DRuntime seems very interested in it (or maybe you're just too scary). Are you willing to play the dictator with this, or are you looking for feedback? |
|
@JinShil thanks for asking! :) I think the intent is what we need; of course it can be improved, and also the implementation may have issues. So feedback is welcome. Generally The mechanisms at our disposal are the following:
The fundamental approach of this PR is simple - calling I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves? |
See Mike's comment above. They can be seen on the Project Tester: https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fdruntime/detail/PR-2126/2/pipeline For local reproduction, do sth. like: git clone https://github.com/higgsjs/Higgs
cd Higgs
dub --compiler=/home/seb/dlang/dmd/generated/linux/release/64/dmd |
The symptom appears in the Higgs test suite where the unitttests are specifically testing if an object is in a certain state:
I believe this is caused simply by the existence of some of destructors in the Higgs code base. I already removed 1 empty constructor which solved one error, but another appeared. There are current 2 more empty constructors in Higgs: https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/object.d#L512 I tried to remove those as well, but it didn't solve the problem we're currently experiencing. The current error appears to be due to the following destructor, which is not empty: I tried to fix that also to make it compatible with this PR, but ultimately failed. I don't understand the code or the test suite logic well enough to make the change. If you are convinced that this PR is the way to go, and are willing to exercise your authority, given the lack of reviews, I propose the following procedure:
Note that I am not a member of team DRuntime, so if you don't want to merge your own PR, you'll still have to convince someone to review this PR. I'm not crazy about these semantics, but I also don't have any other solution. I'd be hesitant putting my name next to the merge notice even if I were on team DRuntime, but I'd do it if enough time passed without anyone else stepping up with an alternate solution. Pinging @klickverbot @schveiguy @MartinNowak @rainers @ZombineDev @DmitryOlshansky @maximecb for help. |
|
!ping |
|
Optimization: If a class/struct aggregate does not define a destructor itself, but one of its fields defines a destructor, we need to do Also, I like the new semantics of |
|
@ntrel we won't cater to people relying on undocumented behavior. This not only creates difficulties for this particular PR, but creates a dangerous precedent whereby we dig ourselves in bureaucracy. |
|
|
||
| $(UL $(LI If `T` is a primitive type, a `struct` types that does not | ||
| have a destructor, or a `class` type that does not have a destructor, | ||
| the call to has no effect.) |
There was a problem hiding this comment.
"call to has no effect" Maybe missing "destroy" in there?
Note, this is technically incorrect for a class type with no destructor, as destroying a class instance that has no destructor is still going to call rt_finalize, which at least sets the vtable to null.
| */ | ||
| void destroy(T)(T obj) if (is(T == class)) | ||
| void destroy(T)(T obj) if (is(T == class) || (is(T == interface))) |
There was a problem hiding this comment.
I think there is a pre-existing bug in here, as I think the static test will pass for extern(C++) classes, which is definitely NOT correct to call rt_finalize on. Something to think about as we move towards betterC.
There was a problem hiding this comment.
And Objective-C classes. There’s a recent change where __traits(getLinkage) supports classes.
| @@ -3145,9 +3231,8 @@ unittest | |||
|
|
|||
| /// ditto | |||
| void destroy(T)(ref T obj) | |||
| if (!is(T == struct) && !is(T == interface) && !is(T == class) && !_isStaticArray!T) | |||
| if (!is(T == struct) && !is(T == interface) && !is(T == class) && !_isStaticArray!T) | |||
There was a problem hiding this comment.
This is going to accept an lvalue dynamic array, which is potentially going to override the array code above. I'm not 100% sure, because it's a specialization.
This may fail:
static struct S
{
int x;
~this() {}
}
S[] arr = new S[5];
arr[] = S(42);
destroy(arr);
assert(arr[0].x == 0); // will this fail?
This supersedes #2124 and previous attempts. Apologies for the override but it seemed easier to simply write this than discuss what to do over several sessions. cc @JinShil