Page MenuHomePhabricator

[Clang] Improve diagnostics when using a concept as template argument

Authored by cor3ntin on Mar 23 2023, 6:34 AM.



When using the name of a template variable or concept in places
where an expression was expected, Clang would drop the cxxscope token
preceeding it, if any.

This leads to subpar diagnostics - complaining about the
identifier being undeclared as clang would not know to look into a
non-global scope.

We make sure the scope is preserved.

When encountering ns::Concept foo x;, Clang would also fail
to provide the same quality as it does at global scope.

Diff Detail

Event Timeline

cor3ntin created this revision.Mar 23 2023, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 6:34 AM
cor3ntin requested review of this revision.Mar 23 2023, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 6:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Compiler explorer link demonstrating the issue this fixes

This generally looks good to me, but I get lost in the parser pretty quick, so hoping one of the other reviewers can take a look.


So it appears that lines 27 and 28 are the only behavioral changes here, right? The line 31 and 37 sections are the same, right?

1 Nit here: I REALLY prefer when we use bookmarks, and include the notes next to their diagnostic. Here it isn't hugely necessary, but still a preference if you're needing to update this again.

cor3ntin added inline comments.Mar 23 2023, 6:58 AM

Yes - but 38/39 would produce different diagnostics without the changes in ParseDecl.cpp So i thought it best to test everything.
Gotcha on the bookmarks - never used them before though :)

Want to add a release note for this as well?


Would this change make sense, to validate that we're still consuming the token in these three cases? I think it was an assumption we made previously, but it's not clear to me if the code used to consume something other than < as well.


Totally minor nit, but the same should be changed throughout the file.

cor3ntin updated this revision to Diff 508299.Mar 25 2023, 5:07 AM
  • Address Aaron's and Erich's comments

I'm not sure. You'll notice that using a var template without template argument does not trigger the assert. That's because ClassifyName will return NC_NonType instead of NC_VarTemplate in that case. I don't think that makes sense either, but that require more investigation. I do think however all template names not followed by arguments should produce a TemplateIdAnnotation so the change is not not correct :)

If you insist, i can put the assert back, but I'm not sure the assert was testing the right thing.

aaron.ballman added inline comments.Mar 25 2023, 5:38 AM

I don't insist on adding the assert, it was more a matter of trying to figure out whether the changes to the non-concept cases have changed parsing behavior in some subtle way. My thinking is that an assert that fails (signaling we are no longer consuming a token when we previously would have) would be easier for us to debug than a mysterious parsing failure. But if that assert would be wrong to add, then it makes me wonder what the test case is for it because then we've changed parsing behavior.

This comment was removed by cor3ntin.

Funnily, the cases

case Sema::NC_VarTemplate:
case Sema::NC_FunctionTemplate:
case Sema::NC_UndeclaredTemplate:

Are never traversed, We can remove them without breaking tests. the assert doesn't do anything then. arguably ClassifyName needs further fix but that might be more complicated surgery

aaron.ballman accepted this revision.Mar 29 2023, 9:58 AM

LGTM, but please give @erichkeane a chance to give it a second set of eyes given he's been knee-deep in concepts recently.

This revision is now accepted and ready to land.Mar 29 2023, 9:58 AM
erichkeane accepted this revision.Mar 29 2023, 9:59 AM

Yeah, LGTM too. Thanks!

This revision was landed with ongoing or failed builds.Mar 30 2023, 7:30 AM
This revision was automatically updated to reflect the committed changes.