[clangd] Drop injected class name when class scope is not explicitly specified.

[ioeric]

Summary

E.g. allow injected "A::A" in using A::A^ but not in "A^".

[kadircet]

LGTM, thanks!

unittests/clangd/CodeCompleteTests.cpp

419 ↗

(On Diff #175896)

Could you also add a test for the inheritance of injected class name? For ex:

struct X {};
struct T : private X {};
struct F : public T {
X^ <-- this is inavlid and we should not suggest X here, since it is inaccessible.
::X^ <-- this should be Ok.
};

[ioeric]

unittests/clangd/CodeCompleteTests.cpp

419 ↗

(On Diff #175896)

Happy to improve the test coverage. But I couldn't see how this tests changes in this patch. It seems something sema access should be testing and a bit out of scope? Could you elaborate on the intention? Thanks!

[kadircet]

unittests/clangd/CodeCompleteTests.cpp

419 ↗

(On Diff #175896)

Ah you are right, it should be rather handled within sema to mark the first one as not accessible.

clang-tools-extra/trunk/clangd/CodeComplete.cpp

bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
   case CodeCompletionContext::CCC_NaturalLanguage:
   case CodeCompletionContext::CCC_Recovery:
   case CodeCompletionContext::CCC_NewName:
     return false;
   }
   llvm_unreachable("unknown code completion context");
 }
 
+static bool isInjectedClass(const NamedDecl &D) {
+  if (auto *R = dyn_cast_or_null<RecordDecl>(&D))
+    if (R->isInjectedClassName())
+      return true;
+  return false;
+}
+
 // Some member calls are blacklisted because they're so rarely useful.
 static bool isBlacklistedMember(const NamedDecl &D) {
   // Destructor completion is rarely useful, and works inconsistently.
   // (s.^ completes ~string, but s.~st^ is an error).
   if (D.getKind() == Decl::CXXDestructor)
     return true;
   // Injected name may be useful for A::foo(), but who writes A::A::foo()?
-  if (auto *R = dyn_cast_or_null<RecordDecl>(&D))
-    if (R->isInjectedClassName())
-      return true;
+  if (isInjectedClass(D))
   // Explicit calls to operators are also rare.
   auto NameKind = D.getDeclName().getNameKind();
   if (NameKind == DeclarationName::CXXOperatorName ||
       NameKind == DeclarationName::CXXLiteralOperatorName ||
       NameKind == DeclarationName::CXXConversionFunctionName)
     return true;
   return false;
 }
for (unsigned I = 0; I < NumResults; ++I) {
       if (!Opts.IncludeIneligibleResults &&
           (Result.Availability == CXAvailability_NotAvailable ||
            Result.Availability == CXAvailability_NotAccessible))
         continue;
       if (Result.Declaration &&
           !Context.getBaseType().isNull() // is this a member-access context?
           && isBlacklistedMember(*Result.Declaration))
         continue;
+      // Skip injected class name when no class scope is not explicitly set.
+      // E.g. show injected A::A in `using A::A^` but not in "A^".
+      if (Result.Declaration && !Context.getCXXScopeSpecifier().hasValue() &&
+          isInjectedClass(*Result.Declaration))
+        continue;
       // We choose to never append '::' to completion results in clangd.
       Result.StartsNestedNameSpecifier = false;
       Results.push_back(Result);
     }
     ResultsCallback();
   }
 
   CodeCompletionAllocator &getAllocator() override { return *CCAllocator; }

clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

EXPECT_THAT(
           "template<class> struct Y{}; struct X:Y<int>{ void foo(){ ^ } };")
           .Completions,
       Has("Y"));
   // This case is marginal (`using X::X` is useful), we allow it for now.
   EXPECT_THAT(completions("struct X{}; void foo(){ X::^ }").Completions,
               Has("X"));
 }
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,
+              ElementsAre(Named("X"), Named("~X")));
+}
+
 TEST(CompletionTest, Snippets) {
   clangd::CodeCompleteOptions Opts;
   auto Results = completions(
       R"cpp(
       struct fake {
         int a;
         int f(int i, const float f) const;
       };