Issue 24864 - hasElaborateDestructor incorrectly true for structs with anonymous unions
Summary: hasElaborateDestructor incorrectly true for structs with anonymous unions
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: druntime (show other issues)
Version: D2
Hardware: All All
: P1 normal
Assignee: No Owner
URL:
Keywords: pull
Depends on:
Blocks:
 
Reported: 2024-11-18 09:18 UTC by Jonathan M Davis
Modified: 2024-11-18 14:35 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Jonathan M Davis 2024-11-18 09:18:39 UTC
This code

---
void main()
{
    import std.traits;

    static struct Member
    {
        ~this() {}
    }

    static struct S
    {
        union
        {
            Member member;
            int i;
        }
    }

    static assert(!hasElaborateDestructor!S);
}
---

fails to compile, because the static assertion fails. However, S does not have a destructor. Member does, and member is a member variable of S, but it's within a union, so S itself doesn't get a destructor, and member's destructor is not supposed to be called - and if you do something like

---
void main()
{
    static struct Member
    {
        ~this() { assert(false); }
    }

    static struct S
    {
        union
        {
            Member member;
            int i;
        }
    }

    S s;
}
---

the assertion does not fail, because S does not have a destructor, and therefore, member's destructor is not called.

Of course, well-written code would keep track of which union member is the valid one and call destroy on member when appropriate from inside a user-defined destructor on S, but that's irrelevant to the question of whether S as presented here has elaborate destruction. It doesn't, and as such, hasElaborateDestructor shouldn't claim that it does.
Comment 1 Jonathan M Davis 2024-11-18 10:00:53 UTC
hasElaborateAssign used to have this same problem, but the fix for https://issues.dlang.org/show_bug.cgi?id=24835 inadvertently fixed it.
Comment 2 Dlang Bot 2024-11-18 12:00:16 UTC
@jmdavis updated dlang/dmd pull request #17075 "Fix bugzilla issue# 24864: hasElaborateDestructor wrong with anonymous unions" fixing this issue:

- Fix bugzilla issue 24864: hasElaborateDestructor wrong with anonymous unions
  
  hasElaborateDestructor should only be true if the type actually has a
  destructor which is called when the object leaves scope. Anonymous
  unions do not give a destructor to their containing type even if one or
  more of their members has one (the same with copy constructors, postblit
  constructors, and assignment operators). For that sort of thing to work
  properly, such functions need to be added manually to the struct such
  that they call the functions appropriately on whichever member of the
  union is the valid one.
  
  As such, hasElaborateDestructor should not be true based on the member
  of a union, and there is no need to check the fields of a struct at all,
  because the ultimate question is whether the struct itself has a
  destructor. So, the fact that the code has been checking the struct's
  members is unnecessary in general and wrong in the case of anonymous
  unions.
  
  The change to checking for __xdtor is because __dtor is an explicitly
  declared destructor, whereas __xdtor is generated by the compiler
  (either because the struct has a destructor or because it has at least
  one member variable which does - and which isn't in a union). So, the
  check for whether the member variables had a __dtor member was probably
  to try to catch the cases where the struct hadn't declared an explicit
  destructor but had had one generated because of its member variables.
  However, simply checking for __xdtor catches that along with explicit
  destructors, and there's no need to instantiate any additional templates
  to check the member variables.
  
  In addition to fixing this issue with hasElaborateDestructor, I've
  improved the tests for hasElaborateCopyConstructor and
  hasElaborateAssign to catch the same issue for them, though they don't
  currently have the bug.

https://github.com/dlang/dmd/pull/17075
Comment 3 Dlang Bot 2024-11-18 14:35:17 UTC
dlang/dmd pull request #17075 "Fix bugzilla issue# 24864: hasElaborateDestructor wrong with anonymous unions" was merged into master:

- 74e630d98547ca21eb0ce983c27141d641e46908 by Jonathan M Davis:
  Fix bugzilla issue 24864: hasElaborateDestructor wrong with anonymous unions
  
  hasElaborateDestructor should only be true if the type actually has a
  destructor which is called when the object leaves scope. Anonymous
  unions do not give a destructor to their containing type even if one or
  more of their members has one (the same with copy constructors, postblit
  constructors, and assignment operators). For that sort of thing to work
  properly, such functions need to be added manually to the struct such
  that they call the functions appropriately on whichever member of the
  union is the valid one.
  
  As such, hasElaborateDestructor should not be true based on the member
  of a union, and there is no need to check the fields of a struct at all,
  because the ultimate question is whether the struct itself has a
  destructor. So, the fact that the code has been checking the struct's
  members is unnecessary in general and wrong in the case of anonymous
  unions.
  
  The change to checking for __xdtor is because __dtor is an explicitly
  declared destructor, whereas __xdtor is generated by the compiler
  (either because the struct has a destructor or because it has at least
  one member variable which does - and which isn't in a union). So, the
  check for whether the member variables had a __dtor member was probably
  to try to catch the cases where the struct hadn't declared an explicit
  destructor but had had one generated because of its member variables.
  However, simply checking for __xdtor catches that along with explicit
  destructors, and there's no need to instantiate any additional templates
  to check the member variables.
  
  In addition to fixing this issue with hasElaborateDestructor, I've
  improved the tests for hasElaborateCopyConstructor and
  hasElaborateAssign to catch the same issue for them, though they don't
  currently have the bug.

https://github.com/dlang/dmd/pull/17075