Fix Issue 21613 - DMD crash: copy ctor + templated rvalue ctor #13687
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + dmd#13687" |
src/dmd/dtemplate.d
Outdated
|
|
||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Copying this code isn't a good solution. Maybe this bug indicates that the checks done in dsymbolsem.d are not in the right location to begin with. But at the very least move the logic into a common function.
There was a problem hiding this comment.
The check in dsymbolsem is definitely in the right location. The problem is that function resolution does not fully instantiate a templated function (i.e. it does not perform any semantic analysis) to check whether it is a match or not. It just creates a dummy template instance to check the constraints. Bypassing semantic analysis is the problem here, that's why we need to redo the check.
test/fail_compilation/fail21613.d
Outdated
| void main(){ | ||
| const Test cb; | ||
| Test b = cb; | ||
| } |
There was a problem hiding this comment.
You could add this test to fail_compilation/test22593.d because it checks another manifestation of the same error.
src/dmd/dtemplate.d
Outdated
| auto ad = fd.isThis(); | ||
| auto sd = ad.isStructDeclaration(); | ||
| immutable dim = tf.parameterList.length; | ||
| if (sd && sd.hasCopyCtor && (dim == 1 || (dim > 1 && tf.parameterList[1].defaultArg)) ) |
There was a problem hiding this comment.
I see that the almost the same condition appears in two other places:
- https://github.com/dlang/dmd/blob/v2.099.0/src/dmd/dsymbolsem.d#L4048-L4052
- https://github.com/dlang/dmd/blob/v2.099.0/src/dmd/dsymbolsem.d#L4067-L4074
Can you refactor the code by adding a function that better clarifies the intent?
Edit: Nevermind, I didn't see that @MoonlightSentinel had posted a similar comment (I was reviewing with comments/annotations off).
There was a problem hiding this comment.
Note that the difference between the 2 occurrences you provided is that one checks for an rvalue constructor, whereas the other one checks for a copy constructor. I would conflate the two.
There was a problem hiding this comment.
What I can do is to put the rvalue constructor check in a function and look in a subsequent PR to create a function isSpecialConstructor which receives a TypeFunction and returns an enum value from enum CtorType { Normal, CopyConstructor, RvalueConstructor}
|
@RazvanN7 going to rebase and finish this? |
|
@9il it seems that this PR causes a breakage in mir/core. To me it looks like Algebraic ends up defining both a copy constructor and an rvalue constructor. Could you please update the code so that that does not happen anymore? Edit: Offending code: Variant!S a = S();
auto b = a;Variant (which is an alias to Algebraic) defines a copy constructor because I think we can get away by simply adding a constraint that excludes the constructor when the RHSTypes matches the instantiation types. |
|
@ibuclaw this should be ready to go once libmir/mir-core#81 is in. |
|
@ibuclaw this is ready to go. |
No description provided.