Fix Issue 20549 - Initialization with a tuple of a module symbol refe… #10750
Conversation
…rencing itself could crash dmd
|
Thanks for your pull request and interest in making D better, @BorisCarvajal! 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 fetch digger
dub run digger -- build "stable + dmd#10750" |
|
I'm just disallowing it from that block, that piece of code seems to be there just to get some extra info about postblit/copy constructor in structs, the initialization works fine and the error is the expected output. |
|
Fair enough, I suppose if someone wants to use it in that way they will open a bug report about it. |
|
This seems to be a hackish workaround. I would prefer to have a look before this is merged... |
| if (ei) | ||
| // Bug 20549. Don't try this on modules or packages, syntaxCopy | ||
| // could crash (inf. recursion) on a mod/pkg referencing itself | ||
| if (ei && (ei.exp.op != TOK.scope_ ? true : !(cast(ScopeExp)ei.exp).sds.isPackage())) |
There was a problem hiding this comment.
|| is a thing too
ei.exp.op != TOK.scope_ || !(cast(ScopeExp)ei.exp).sds.isPackage())
There was a problem hiding this comment.
My initial version was a parenthesesless with the ternary, for some reason I put the parens before committing, but yeah with them that alternative is absolutely more clean.
Agreed. Yet another too eager merge :/ |
| @@ -0,0 +1,5 @@ | |||
| module test; | |||
There was a problem hiding this comment.
This should have a TEST_OUTPUT section checking for the correct error message.
There was a problem hiding this comment.
The error message is not the thing to be tested.
There was a problem hiding this comment.
All test that yield error messages should provide that section because its assumed to be silent otherwise. IIRC this is currently not enforced but will become the default once the remaining tests are updated.
There was a problem hiding this comment.
IIRC there were only a handful of tests left as they had a massive list of output, maybe they can be ignored manually?
There was a problem hiding this comment.
The point is not only to test the error message, but to make sure the assumptions made by the test hold.
Someone sometime later could change some code in DMD, and lead to the test case pass for the wrong reason
(here "passing" == "not compiling"). That could potentially mean the regression could sneak back because something unrelated in the test now fails.
And while it's unlikely with a 3 lines test, it's still a good habit to have. (On a side note, there also was some effort to have TEST_OUTPUT set as required IIRC).
There was a problem hiding this comment.
Sorry for not knowing all the test rules, there should be a test category for "not crash only".
Now thinking I should have written the code in a trait-compiles on the compilable category, much clever.
There was a problem hiding this comment.
fail_compilation is fine as it assigns a module to an enum.
Anyway, I assume the explanation concerning is in the tests README.md (or should be added there if it is missing)
|
Yeah its hacky and as I said before, "Maybe the correct fix is just to run the if only for structs.", that code looks like it was meant to be used only on a struct but I didn't know and I didn't want to keep investigating. Always someone can apply the proper fix in the future. |
|
That's fine, nobody forces you to continue your work on a certain task. Someone else can pick up this PR and maybe find at a better solution. |
…rencing itself could crash dmd