[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!

clang/docs/ReleaseNotes.rst

 - Fix false-positive diagnostic issued for consteval initializers of temporary
   objects.
   (`#60286 <https://github.com/llvm/llvm-project/issues/60286>`_)
 - Correct restriction of trailing requirements clauses on a templated function.
   Previously we only rejected non-'templated' things, but the restrictions ALSO need
   to limit non-defined/non-member functions as well. Additionally, we now diagnose
   requires on lambdas when not allowed, which we previously missed.
   (`#61748 <https://github.com/llvm/llvm-project/issues/61748>`_)
+- Fix confusing diagnostic for incorrect use of qualified concepts names.
+
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

 
 AMDGPU Support
 ^^^^^^^^^^^^^^
 
 - Linking for AMDGPU now uses ``--no-undefined`` by default. This causes
   undefined symbols in the created module to be a linker error. To prevent this,
   pass ``-Wl,--undefined`` if compiling directly, or ``-Xoffload-linker
   --undefined`` if using an offloading language.
-- The deprecated ``-mcode-object-v3`` and ``-mno-code-object-v3`` command-line 
+- The deprecated ``-mcode-object-v3`` and ``-mno-code-object-v3`` command-line
   options have been removed.
 
 X86 Support
 ^^^^^^^^^^^
 
 Arm and AArch64 Support
 ^^^^^^^^^^^^^^^^^^^^^^^
 

clang/lib/Parse/ParseDecl.cpp

case tok::annot_cxxscope: {
         DS.getTypeSpecScope() = SS;
         ConsumeAnnotationToken(); // The C++ scope.
         assert(Tok.is(tok::annot_template_id) &&
                "ParseOptionalCXXScopeSpecifier not working");
         AnnotateTemplateIdTokenAsType(SS, AllowImplicitTypename);
         continue;
       }
 
-      if (TemplateId && TemplateId->Kind == TNK_Concept_template &&
-          GetLookAheadToken(2).isOneOf(tok::kw_auto, tok::kw_decltype)) {
+      if (TemplateId && TemplateId->Kind == TNK_Concept_template) {
         DS.getTypeSpecScope() = SS;
-        // This is a qualified placeholder-specifier, e.g., ::C<int> auto ...
-        // Consume the scope annotation and continue to consume the template-id
-        // as a placeholder-specifier.
+        // This is probably a qualified placeholder-specifier, e.g., ::C<int>
+        // auto ... Consume the scope annotation and continue to consume the
+        // template-id as a placeholder-specifier. Let the next iteration
+        // diagnose a missing auto.
         ConsumeAnnotationToken();
         continue;
       }
 
       if (Next.is(tok::annot_typename)) {
         DS.getTypeSpecScope() = SS;
         ConsumeAnnotationToken(); // The C++ scope.
         TypeResult T = getTypeAnnotation(Tok);

clang/lib/Parse/Parser.cpp

Parser::TryAnnotateName(CorrectionCandidateCallback *CCC,
   case Sema::NC_TypeTemplate:
     if (Next.isNot(tok::less)) {
       // This may be a type template being used as a template template argument.
       if (SS.isNotEmpty())
         AnnotateScopeToken(SS, !WasScopeAnnotation);
       return ANK_TemplateName;
     }
     [[fallthrough]];
+  case Sema::NC_Concept:
   case Sema::NC_VarTemplate:
   case Sema::NC_FunctionTemplate:
   case Sema::NC_UndeclaredTemplate: {
-    // We have a type, variable or function template followed by '<'.
+    bool IsConceptName = Classification.getKind() == Sema::NC_Concept;
+    // We have a template name followed by '<'. Consume the identifier token so
+    // we reach the '<' and annotate it.
+    if (Next.is(tok::less))
       ConsumeToken();
     UnqualifiedId Id;
     Id.setIdentifier(Name, NameLoc);
     if (AnnotateTemplateIdToken(
             TemplateTy::make(Classification.getTemplateName()),
-            Classification.getTemplateNameKind(), SS, SourceLocation(), Id))
-      return ANK_Error;
-    return ANK_Success;
-  }
-  case Sema::NC_Concept: {
-    UnqualifiedId Id;
-    Id.setIdentifier(Name, NameLoc);
-    if (Next.is(tok::less))
-      // We have a concept name followed by '<'. Consume the identifier token so
-      // we reach the '<' and annotate it.
-      ConsumeToken();
-    if (AnnotateTemplateIdToken(
-            TemplateTy::make(Classification.getTemplateName()),
             Classification.getTemplateNameKind(), SS, SourceLocation(), Id,
-            /*AllowTypeAnnotation=*/false, /*TypeConstraint=*/true))
+            /*AllowTypeAnnotation=*/!IsConceptName,
+            /*TypeConstraint=*/IsConceptName))
       return ANK_Error;
+    if (SS.isNotEmpty())
+      AnnotateScopeToken(SS, !WasScopeAnnotation);
     return ANK_Success;
   }
   }
 
   // Unable to classify the name, but maybe we can annotate a scope specifier.
   if (SS.isNotEmpty())
     AnnotateScopeToken(SS, !WasScopeAnnotation);
   return ANK_Unresolved;

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

+// RUN: %clang_cc1 -std=c++20 -verify -fsyntax-only %s
+
+namespace a {
+  template <typename T>
+  concept C1 = true; // #C1
+
+  template <typename T>
+  auto V1 = true; // #V1
+
+  namespace b {
+    template <typename T>
+    concept C2 = true; // #C2
+    template <typename T>
+    auto V2 = true; // #V2
+  }
+}
+
+template <typename T>
+concept C3 = true; // #C3
+template <typename T>
+auto V3 = true; // #V3
+template <template <typename T> typename C>
+constexpr bool test = true;
+
+static_assert(test<a::C1>); // expected-error {{too few template arguments for concept 'C1'}} \
+                            // expected-note@#C1 {{here}}
+static_assert(test<a::b::C2>); // expected-error {{too few template arguments for concept 'C2'}} \
+                               // expected-note@#C2 {{here}}
+static_assert(test<C3>); // expected-error {{too few template arguments for concept 'C3'}} \
+                         // expected-note@#C3 {{here}}
+
+static_assert(test<a::V1>); // expected-error {{use of variable template 'V1' requires template arguments}} \
+                            // expected-note@#V1 {{here}}
+static_assert(test<a::b::V2>); // expected-error {{use of variable template 'V2' requires template arguments}} \
+                            // expected-note@#V2 {{here}}
+static_assert(test<V3>); // expected-error {{use of variable template 'V3' requires template arguments}} \
+                         // expected-note@#V3 {{here}}
+
+
+void f() {
+    C3 t1 = 0;  // expected-error {{expected 'auto' or 'decltype(auto)' after concept name}}
+    a::C1 t2 = 0; // expected-error {{expected 'auto' or 'decltype(auto)' after concept name}}
+    a::b::C2 t3 = 0; // expected-error {{expected 'auto' or 'decltype(auto)' after concept name}}
+}