Skip to content
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

druntime compiler hooks do not all support copy constructors properly #20970

Open
jmdavis opened this issue Mar 9, 2025 · 0 comments
Open

druntime compiler hooks do not all support copy constructors properly #20970

jmdavis opened this issue Mar 9, 2025 · 0 comments
Labels

Comments

@jmdavis
Copy link
Member

jmdavis commented Mar 9, 2025

I'm creating this issue so that we have a single issue to point to whereas right now, we have a bunch of related issues. I linked the ones that I found below.

The core problem is that the druntime compiler hooks were not updated to support copy constructors when they were introduced, and the only support that has been added since then is when some of the hooks have been templatized. TypeInfo was never updated to know about copy constructors, and so the only hooks that even can know about copy constructors are those which are templates and thus can actually do type introspection on the types being used. In order to fix this situation, either TypeInfo must be updated to know about copy constructors, and all of the hooks that aren't templates need to be updated accordingly, or all of the druntime hooks need to be templatized (with their implementations dealing with copy constructors correctly). And of course, as part of this, we need to actually be testing that all of these various hooks work correctly with types with copy constructors. That testing seems to have been missed completely when copy constructors were introduced - or it was known that it was an issue but just forgotten. Either way, as things stand, code using dynamic arrays or associative arrays cannot safely use copy constructors for any of the elements, keys, or values in those data structures, because their copy constructors will not be called properly in all cases.

@teodutu is the one who has done much of the work to templatize the druntime hooks, but as I understand it, he has become quite busy and is not currently working on them. So, we really need to figure out how we're going to solve this if we want this this issue to be solved in a timely manner, and IMHO, as things stand, we have to tell programmers to avoid using copy constructors, because they're broken for dynamic arrays and associative arrays. When I e-mailed Teodor about the hooks previously, this is part of what he told me:

Below is the list of all the hooks that have been templated:
_d_arrayctor
_d_arraysetctor
_d_arrayassign
_d_arrayassign_l
_d_arrayassign_r
_d_delstruct
_d_newThrowable
_d_arrayappendT
_d_arrayappendcTX
_d_arraycatT
_d_arraycatnTX
_d_newitemiT
_d_newitemT
_d_newitemU
_d_newclass
_d_newarrayiT
_d_newarrayT
_d_newarrayU
_d_newarraymTX
_d_newarrayOpT

Below is the list of all the hooks that need to be templated:
_d_arrayliteralTX
_d_assocarrayliteralTX (and the whole of aa.d because all associative
array-related functions require TypeInfo)
_d_arraysetcapacity
_d_arraysetlengthiT
_d_arraysetlengthT
_d_arrayshrinkfit
_d_interface_cast
_d_isbaseof
_d_isbaseof2

I started working on _d_arrayliteralTX, but stopped for a while
because array literals pop up in many places after semantic analysis
(due to optimisations) and I thought it would be better to move all
lowerings (eventually) to a new pass after the frontend has done all
its work (including constfold.d and optimize.d) and the AST is final.
But the blocker for this was exposing the Scope hierarchy. Scopes are
needed because introducing a hook requires running semantic analysis
on it, which needs a scope.

Another thing to note is that _d_arraysetlength{,i}T was also
templated by Dan Printzell quite a few years ago, but only regarding
its lowering [1], [2]. The template druntime implementation is just
calling the original hook. I'm planning to change this one to be fully
templated and not rely on code in rt/lifetime.d in the near future to
get back to working on runtime hooks.

[1] https://github.com/dlang/dmd/pull/10106
[2] https://github.com/dlang/druntime/pull/2656

Digging through the list of bugs, these are the bugs that I found which are caused by not all of the hooks supporting copy constructors:

#17487
#17212
#17202
#17454

The are of course other bugs related to copy constructors, postblit constructors, and copying in general, but those are the ones that I found which are clearly caused by the hooks not all dealing with copy constructors properly.

Related to this is that we're now getting move constructors, and TypeInfo doesn't understand those either - and I'm not sure that any of the druntime hooks have been updated accordingly either, so I expect that move constructors are in a similar boat, but the copy constructor situation is much more critical IMHO, since we've had them for years, and we've even been claiming in the spec that folks should prefer them to postblit constructors when in fact, they're broken - at least for anyone who's using proper D and not -betterC. And anyone using move constructors is likely going to want copy constructors anyway.

@jmdavis jmdavis added Druntime Specific to druntime P1 Severity:critical labels Mar 9, 2025
jmdavis added a commit to jmdavis/dlang.org that referenced this issue Mar 9, 2025
Until dlang/dmd#20970 is fixed, we need to be
warning against the use of copy constructors rather than warning
against the use of postblit constructors.

So, I fixed the warning and put both it and the description of what
happens when mixing postblit constructors and copy constructors together
on both the section for postblit constructors and the one for copy
constructors.

I suspect that move constructors merit a similar warning, but I'm not
sure what their current state is, so this just fixes the warning for
postblit constructors and copy constructors.
jmdavis added a commit to jmdavis/dlang.org that referenced this issue Mar 9, 2025
Until dlang/dmd#20970 is fixed, we need to be
warning against the use of copy constructors rather than warning
against the use of postblit constructors.

So, I fixed the warning and put both it and the description of what
happens when mixing postblit constructors and copy constructors together
on both the section for postblit constructors and the one for copy
constructors.

I suspect that move constructors merit a similar warning, but I'm not
sure what their current state is, so this just fixes the warning for
postblit constructors and copy constructors.
jmdavis added a commit to jmdavis/dlang.org that referenced this issue Mar 9, 2025
…ors.

Until dlang/dmd#20970 is fixed, we need to be
warning against the use of copy constructors rather than warning
against the use of postblit constructors.

So, I fixed the warning and put both it and the description of what
happens when mixing postblit constructors and copy constructors together
on both the section for postblit constructors and the one for copy
constructors.

I suspect that move constructors merit a similar warning, but I'm not
sure what their current state is, so this just fixes the warning for
postblit constructors and copy constructors.
jmdavis added a commit to jmdavis/dlang.org that referenced this issue Mar 11, 2025
…ors.

Until dlang/dmd#20970 is fixed, we need to be
warning against the use of copy constructors rather than warning
against the use of postblit constructors.

So, I fixed the warning and put both it and the description of what
happens when mixing postblit constructors and copy constructors together
on both the section for postblit constructors and the one for copy
constructors.

I suspect that move constructors merit a similar warning, but I'm not
sure what their current state is, so this just fixes the warning for
postblit constructors and copy constructors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant