[clang] Include type specifiers in typo correction when checking isCXXDeclarationSpecifiers.

[hokein]

Summary

• add more tests (the test added in https://github.com/llvm/llvm-project/commit/2f448467e4254ddc3191136c968e6054bc009b88 is weak);
• improve the MyTemplate<type_typo, int>(); case, with this patch, typo correction could suggest type decls, and no regressions found.

[hokein]

update

[sammccall]

clang/test/SemaCXX/typo-correction.cpp

614

are these one testcase or two?
If they're independent, please use different identifiers to avoid confusion

623

"resolve" and 5 lines below

[hokein]

rebase and address comments.

[sammccall]

clang/test/SemaCXX/typo-correction.cpp

614

I'm not sure this comment was addressed?
both testIncludeTypeInTemplateArgument and testNoCrashOnNullNNSTypoCorrection use the name "AddObservation", which seems like something very specific but in fact these tests are unrelated.

[aganea]

We're seeing the same issue mentionned in PR45498, but with the repro below. git bisect points to this patch. Could anyone please possibly confirm? Simply build clang with -DLLVM_ENABLE_ASSERTIONS=ON.

// compile with: clang-cl /c a.cpp
template < a > struct b;
template < bool a, class > using c = b< a >;
template < class > using d = void ;
template < class, class, class = void >
bool e template < class f, class g >
bool e< f, g, d< decltype(h(g())) > > template < class f, class g >
void i(f, g ) {
  e< f, g >
}
template < class _BidIt2, c< e< _BidIt, _BidIt2 >, int > = 0 >
void h(_BidIt2) short do_tolower__Last {
  i(do_tolower__First, do_tolower__Last)
}

[hokein]

In D83025#2389140, @aganea wrote:

We're seeing the same issue mentionned in PR45498, but with the repro below. git bisect points to this patch. Could anyone please possibly confirm? Simply build clang with -DLLVM_ENABLE_ASSERTIONS=ON.

// compile with: clang-cl /c a.cpp
template < a > struct b;
template < bool a, class > using c = b< a >;
template < class > using d = void ;
template < class, class, class = void >
bool e template < class f, class g >
bool e< f, g, d< decltype(h(g())) > > template < class f, class g >
void i(f, g ) {
  e< f, g >
}
template < class _BidIt2, c< e< _BidIt, _BidIt2 >, int > = 0 >
void h(_BidIt2) short do_tolower__Last {
  i(do_tolower__First, do_tolower__Last)
}

thanks for the testcase. I'm not sure the patch is the root cause -- from the Bugzilla thread, the issue was reported in April which is prior to this patch.

btw, the issue seems to be gone on the trunk, I don't reproduce them with clang building from the trunk, I guess it might be fixed by some patches after llvm-11.

[aganea]

In D83025#2389486, @hokein wrote:

In D83025#2389140, @aganea wrote:

We're seeing the same issue mentionned in PR45498, but with the repro below. git bisect points to this patch. Could anyone please possibly confirm? Simply build clang with -DLLVM_ENABLE_ASSERTIONS=ON.

// compile with: clang-cl /c a.cpp
template < a > struct b;
template < bool a, class > using c = b< a >;
template < class > using d = void ;
template < class, class, class = void >
bool e template < class f, class g >
bool e< f, g, d< decltype(h(g())) > > template < class f, class g >
void i(f, g ) {
  e< f, g >
}
template < class _BidIt2, c< e< _BidIt, _BidIt2 >, int > = 0 >
void h(_BidIt2) short do_tolower__Last {
  i(do_tolower__First, do_tolower__Last)
}

thanks for the testcase. I'm not sure the patch is the root cause -- from the Bugzilla thread, the issue was reported in April which is prior to this patch.

btw, the issue seems to be gone on the trunk, I don't reproduce them with clang building from the trunk, I guess it might be fixed by some patches after llvm-11.

Thanks for getting back! Just for the record, bisecting between the llvmorg-12-init git tag and rG637f19c36b3 leads to this fix: D87853.

commit cffb0dd54d41d8e249d2009467c4beb5b681ba26
Author: Bruno Cardoso Lopes <[e-mail]>
Date:   Mon Oct 12 15:58:52 2020 -0700

    [SemaTemplate] Stop passing insertion position around during VarTemplate instantiation

    They can get stale at use time because of updates from other recursive
    specializations. Instead, rely on the existence of previous declarations to add
    the specialization.

    Differential Revision: https://reviews.llvm.org/D87853

I was able to cherry-pick it cleanly on release/11.x and that solves our issue.

clang/lib/Parse/ParseTentative.cpp

return std::find(TentativelyDeclaredIdentifiers.begin(),
       != TentativelyDeclaredIdentifiers.end();
 }
 
 namespace {
 class TentativeParseCCC final : public CorrectionCandidateCallback {
 public:
   TentativeParseCCC(const Token &Next) {
     WantRemainingKeywords = false;
-    WantTypeSpecifiers = Next.isOneOf(tok::l_paren, tok::r_paren, tok::greater,
-                                      tok::l_brace, tok::identifier);
+    WantTypeSpecifiers =
+        Next.isOneOf(tok::l_paren, tok::r_paren, tok::greater, tok::l_brace,
+                     tok::identifier, tok::comma);
   }
 
   bool ValidateCandidate(const TypoCorrection &Candidate) override {
     // Reject any candidate that only resolves to instance members since they
     // aren't viable as standalone identifiers instead of member references.
     if (Candidate.isResolved() && !Candidate.isKeyword() &&
         llvm::all_of(Candidate,
                      [](NamedDecl *ND) { return ND->isCXXInstanceMember(); }))

clang/test/SemaCXX/typo-correction.cpp

namespace test {
   struct SomeSettings {};  // expected-note {{'test::SomeSettings' declared here}}
 }
 class Test {};
 int bar() {
   Test::SomeSettings some_settings; // expected-error {{no type named 'SomeSettings' in 'testCXXDeclarationSpecifierParsing::Test'; did you mean 'test::SomeSettings'?}}
 }
 }
 
+namespace testIncludeTypeInTemplateArgument {
+template <typename T, typename U>
+void foo(T t = {}, U = {}); // expected-note {{candidate template ignored}}
+
+class AddObservation {}; // expected-note {{declared here}}
+int bar1() {
+  // should resolve to a class.
+  foo<AddObservationFn, int>(); // expected-error {{unknown type name 'AddObservationFn'; did you mean 'AddObservation'?}}
+
+  // should not reslove to a class.
+  foo(AddObservationFn, 1);    // expected-error-re {{use of undeclared identifier 'AddObservationFn'{{$}}}}
+  int a = AddObservationFn, b; // expected-error-re {{use of undeclared identifier 'AddObservationFn'{{$}}}}
+
+  int AddObservation; // expected-note 3{{declared here}}
+  // should reslove to a local variable.
+  foo(AddObservationFn, 1);    // expected-error {{use of undeclared identifier 'AddObservationFn'; did you mean}}
+  int c = AddObservationFn, d; // expected-error {{use of undeclared identifier 'AddObservationFn'; did you mean}}
+
+  // FIXME: would be nice to not resolve to a variable.
+  foo<AddObservationFn, int>(); // expected-error {{use of undeclared identifier 'AddObservationFn'; did you mean}} \
+                                   expected-error {{no matching function for call}}
+}
+
+namespace NoCrash {
+int AddObservation();
+template <typename T, typename... Args>
+class UsingImpl {};
+class AddObservation { // expected-note {{declared here}}
+  using Using =
+      // should resolve to a class.
+      UsingImpl<AddObservationFn, const int>; // expected-error {{unknown type name 'AddObservationFn'; did you mean}}
+};
+} // namespace NoCrash
+} // namespace testIncludeTypeInTemplateArgument
+
 namespace testNonStaticMemberHandling {
 struct Foo {
   bool usesMetadata;  // expected-note {{'usesMetadata' declared here}}
 };
 int test(Foo f) {
   if (UsesMetadata)  // expected-error-re {{use of undeclared identifier 'UsesMetadata'{{$}}}}
     return 5;
   if (f.UsesMetadata)  // expected-error {{no member named 'UsesMetadata' in 'testNonStaticMemberHandling::Foo'; did you mean 'usesMetadata'?}}