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.

Sign up for GitHub

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

Fix Issue 13732 - non-member templates can use "template this" #14434

Merged
merged 7 commits into from
Oct 4, 2022
Merged

Fix Issue 13732 - non-member templates can use "template this" #14434

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c22f40b
Fix Issue 13732 - non-member templates can use "template this"
ntrel Sep 14, 2022
11c0eb1
Ignore template this parameter inside a template as it could be a mixin
ntrel Sep 14, 2022
88fdc35
Fix detecting aggregate template with TemplateThisParameter
ntrel Sep 14, 2022
f6878c9
Remove parse changes
ntrel Sep 18, 2022
a4ee2a4
Implement semantic checks for template this
ntrel Sep 18, 2022
0fb4622
Remove error for explicit template this parameter not class or struct
ntrel Sep 19, 2022
31f49e7
Merge error output, test mixin and remove static this for now
ntrel Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions compiler/src/dmd/templateparamsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,15 @@ private extern (C++) final class TemplateParameterSemanticVisitor : Visitor
result = !(ttp.specType && isError(ttp.specType));
}

override void visit(TemplateThisParameter ttp)
{
import dmd.errors;

if (!sc.getStructClassScope())
error(ttp.loc, "cannot use `this` outside an aggregate type");
visit(cast(TemplateTypeParameter)ttp);
}

override void visit(TemplateValueParameter tvp)
{
tvp.valType = tvp.valType.typeSemantic(tvp.loc, sc);
Expand Down
45 changes: 45 additions & 0 deletions compiler/test/fail_compilation/templatethis.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
TEST_OUTPUT:
---
fail_compilation/templatethis.d(11): Error: cannot use `this` outside an aggregate type
fail_compilation/templatethis.d(15): Error: cannot use `this` outside an aggregate type
fail_compilation/templatethis.d(19): Error: cannot use `this` outside an aggregate type
fail_compilation/templatethis.d(21): Error: cannot use `this` outside an aggregate type
---
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use #line for stable line numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then the error message line numbers don't match up.

Copy link
Member

@ibuclaw ibuclaw Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the point, adding/removing a line doesn't affect the entire output.

Copy link
Contributor

@RazvanN7 RazvanN7 Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

template t(this T)
{
}

struct S(this T)
{
}

enum e(this T) = 1;

void f(this T)()
{
}

/*
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
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(cannot be updated... yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: There are 85 files in fail_compilation which already do this according to a quick grep.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And they are annoying to deal with as one must manually work out where to update them when diagnostics change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that it's better to just group all the error messages into a single TEST_OUTPUT directive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibuclaw OK, I see. Fixed!

*/
mixin template t2()
{
int i(this T) = 1;
}

mixin t2;

class C
{
static void f(this T)() {}
}

alias a = C.f!int; // FIXME error
alias b = C.f!C; // OK