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

[cor3ntin]

Summary

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.

[cor3ntin]

Compiler explorer link demonstrating the issue this fixes https://godbolt.org/z/Tj3v5jbnq

[erichkeane]

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.

clang/test/Parser/cxx-template-template-recovery.cpp

28

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]

clang/test/Parser/cxx-template-template-recovery.cpp

28

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 :)

[aaron.ballman]

Want to add a release note for this as well?

clang/lib/Parse/Parser.cpp

1886–1889

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.

clang/test/Parser/cxx-template-template-recovery.cpp

6

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

[cor3ntin]

• Address Aaron's and Erich's comments

clang/lib/Parse/Parser.cpp

1886–1889

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]

clang/lib/Parse/Parser.cpp

1886–1889

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.

[cor3ntin]

This comment was removed by cor3ntin.

[cor3ntin]

clang/lib/Parse/Parser.cpp

1886–1889

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]

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

[erichkeane]

Yeah, LGTM too. Thanks!