Fix Issue 13732 - non-member templates can use "template this" #14434
Conversation
|
Thanks for your pull request and interest in making D better, @ntrel! 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 "master + dmd#14434" |
Note: Can't use goto to jump into scope(exit).
|
The grammar considers |
|
The grammar says this is just a token:
I'm not sure how to make it a semantic check. In templateparametersem.d there is |
You're right. What I really meant to say was: the grammar allows it outside an aggregate, so the parser should not reject it or the grammar needs to be updated as well.
I'll take a look later. |
I don't know what templateparametersem.d refers to, but I think you can do the check in |
| fail_compilation/templatethis.d(21): Error: cannot use `this` outside an aggregate type | ||
| --- | ||
| */ | ||
|
|
There was a problem hiding this comment.
Use #line for stable line numbers.
There was a problem hiding this comment.
Well, then the error message line numbers don't match up.
There was a problem hiding this comment.
That is the point, adding/removing a line doesn't affect the entire output.
There was a problem hiding this comment.
Using #line is also annoying since the error line that is reported does not match up the editor line and you can't just :linenumber
| TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/templatethis.d(34): Error: cannot use `this` outside an aggregate type | ||
| fail_compilation/templatethis.d(37): Error: mixin `templatethis.t2!()` error instantiating | ||
| --- |
There was a problem hiding this comment.
I'm not a fan of breaking up the TEST_OUTPUT directives, if the diagnostic were to change in the future, then it can't be updated in an automated way.
There was a problem hiding this comment.
Note: There are 85 files in fail_compilation which already do this according to a quick grep.
There was a problem hiding this comment.
And they are annoying to deal with as one must manually work out where to update them when diagnostics change.
There was a problem hiding this comment.
Yes, I agree that it's better to just group all the error messages into a single TEST_OUTPUT directive.
There was a problem hiding this comment.
@ibuclaw OK, I see. Fixed!
compiler/src/dmd/dsymbolsem.d
Outdated
| @@ -6055,6 +6055,9 @@ void templateInstanceSemantic(TemplateInstance tempinst, Scope* sc, Expressions* | |||
| continue; | |||
| Type t = isType((*tempinst.tiargs)[i]); | |||
| assert(t); | |||
| if (t.ty != TY.Tclass || t.ty != TY.Tstruct) | |||
| tempinst.error("argument for template this parameter must be a class or struct"); | |||
There was a problem hiding this comment.
Possibly missing syntax highlighting for class and struct.
| tempinst.error("argument for template this parameter must be a class or struct"); | |
| tempinst.error("argument for template this parameter must be a `class` or `struct`"); |
There was a problem hiding this comment.
I have removed this error as I don't know why it triggers with core.sync.condition.Condition.
There was a problem hiding this comment.
Probably should be a '&&' condition
-if (t.ty != TY.Tclass || t.ty != TY.Tstruct)
+if (t.ty != TY.Tclass && t.ty != TY.Tstruct)
There was a problem hiding this comment.
@apz28 Thanks, looks good. But I decided to leave that check out of this pull.
|
@ntrel Just group the error messages in the test case and we can get this in. |
Error if a TemplateThisParameter is declared when there's no parent aggregate type
or template (it could be for a mixin).