[clangd] Uprank delcarations when "using q::name" is present in the main file

[omtcyfz]

Summary

Having using qualified::name; for some symbol is an important signal for clangd code completion as the user is more likely to use such symbol. This patch helps to uprank the relevant symbols by saving UsingShadowDecl in the new field of CodeCompletionResult and checking whether the corresponding UsingShadowDecl is located in the main file later in ClangD code completion routine. While the relative importance of such signal is a subject to change in the future, this patch simply bumps DeclProximity score to the value of 1.0 which should be enough for now.

The patch was tested using

$ ninja check-clang check-clang-tools

No unexpected failures were noticed after running the relevant testsets.

[omtcyfz]

Remove comments from TestTU.cpp, both functions are properly documented in the corresponding header.

[omtcyfz]

Add const qualifier to UsingShadowDecl in unit tests.

[sammccall]

Nice! Mostly just a few nits.

clang-tools-extra/unittests/clangd/QualityTests.cpp

83

nit: can we have slightly more descriptive names here as this is a long test?
e.g. namespace hdr { class Used; }
(I think if you forward-declare the class, clang-format will let you have it on one line)

88

this bar::Foo proximity test doesn't seem to be substantially different from header(), could you drop it to reduce noise?(

116–135

definition is not relevant here, AFAICS, revert this comment?
(and in fact this is defined in the main file, not the header)

122

nit: spelling out the captures and types here adds a lot of noise to the test and doesn't seem to gain much in return. What about inlining this, capturing [&], and letting the return type be implicit?

125

doesn't this crash if there are no shadows?

127

findAnyDecl is designed to return the decl you find, idiomatically the filter shouldn't have side effects.

What about:

auto *Shadow = *findAnyDecl(AST, [&](const NamedDecl& ND) {
  if (... Using = ...)
    return Using->getQualifiedNameAsString() == "Bar" && Using->shadow_size();
  return false;
}).shadow_begin();
CodeCompletionResult Result(Shadow->getTargetDecl());
Result.setShadowDecl(Shadow);

140

nit: "current directory" isn't relevant here, rather "main file"?
nit: "uprank" isn't quite right here - ranking/scoring is a concern of extraction, rather evaluate(). Prefer just describing the situation ("Using declaration in main file")
nit: these are using declarations, not using directives

clang/include/clang/Sema/CodeCompleteConsumer.h

852

we don't need both the constructor and the setter/public field - I think the constructor param is actually unused. Drop?

910

the field is already public, no need to add a setter

[ioeric]

Nice!

Just one caveat regarding locations in addition to Sam's comments :)

clang-tools-extra/clangd/Quality.cpp

49

I think we might want to use getExpansionLoc here. Consider DEFINE_string which is implemented like:

#define DEFINE_string(XXX) \
namespace FLs { \
SomeType FLAGS_XXX; \
} \
using FLs::FLAGS_XXX;

When you have `DEFINE_string(X) in the main file, the spelling location will be in the file where the macro is defined, while the expansion location will be in the main file, and I think we would want later.

[omtcyfz]

Address the comments, add a couple of FIXMEs for the future.

[sammccall]

clang-tools-extra/clangd/Quality.cpp

49

If you're not motivated to add the test now, I doubt anyone ever will be, so just remove the FIXME.

clang-tools-extra/unittests/clangd/QualityTests.cpp

116

(this still says "definition", which doesn't seem true/relevant)

119

nit: inline this?
(or if you really want to name it, give a more specific name)

[omtcyfz]

Address few comments, add test for macro definition, slightly reformat QualityTests.cpp

[omtcyfz]

Slightly refactor code by making lambda only used once anonymous.

clang-tools-extra/clangd/Quality.cpp

static bool IsReserved(StringRef Name) {
   // FIXME: Should we exclude _Bool and others recognized by the standard?
   return Name.size() >= 2 && Name[0] == '_' &&
          (isUppercase(Name[1]) || Name[1] == '_');
 }
 
 static bool hasDeclInMainFile(const Decl &D) {
   auto &SourceMgr = D.getASTContext().getSourceManager();
   for (auto *Redecl : D.redecls()) {
+    // FIXME: Consider ExpansionLoc here and probably add a test for that.
     auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation());
     if (SourceMgr.isWrittenInMainFile(Loc))
       return true;
   }
   return false;
 }
 
+static bool hasUsingDeclInMainFile(const CodeCompletionResult &R) {
+  const auto &Context = R.Declaration->getASTContext();
+  const auto &SourceMgr = Context.getSourceManager();
+  if (R.ShadowDecl) {
+    // FIXME(kbobyrev): Add a test for ExpansionLoc as suggested by ioeric
+    // here: https://reviews.llvm.org/D49012#inline-429901
+    const auto Loc = SourceMgr.getExpansionLoc(R.ShadowDecl->getLocation());
+    if (SourceMgr.isWrittenInMainFile(Loc))
+      return true;
+  }
+  return false;
+}
+
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
   class Switch
       : public ConstDeclVisitor<Switch, SymbolQualitySignals::SymbolCategory> {
   public:
 #define MAP(DeclType, Category)                                                \
   SymbolQualitySignals::SymbolCategory Visit##DeclType(const DeclType *) {     \
     return SymbolQualitySignals::Category;                                     \
   }
void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
   if (SemaCCResult.Availability == CXAvailability_NotAvailable ||
       SemaCCResult.Availability == CXAvailability_NotAccessible)
     Forbidden = true;
 
   if (SemaCCResult.Declaration) {
     // We boost things that have decls in the main file. We give a fixed score
     // for all other declarations in sema as they are already included in the
     // translation unit.
-    float DeclProximity =
-        hasDeclInMainFile(*SemaCCResult.Declaration) ? 1.0 : 0.6;
+    float DeclProximity = (hasDeclInMainFile(*SemaCCResult.Declaration) ||
+                           hasUsingDeclInMainFile(SemaCCResult))
+                              ? 1.0
+                              : 0.6;
     SemaProximityScore = std::max(DeclProximity, SemaProximityScore);
   }
 
   // Declarations are scoped, others (like macros) are assumed global.
   if (SemaCCResult.Declaration)
     Scope = std::min(Scope, ComputeScope(SemaCCResult.Declaration));
 }
 

clang-tools-extra/unittests/clangd/QualityTests.cpp

TEST(QualityTests, SymbolQualitySignalExtraction) {
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Keyword);
 }
 
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
     int header();
     int header_main();
+
+    namespace hdr { class Bar {}; } // namespace hdr
     )cpp";
   Test.Code = R"cpp(
+    using hdr::Bar;
+
     int ::header_main() {}
     int main();
 
     [[deprecated]]
     int deprecated() { return 0; }
 
     namespace { struct X { void y() { int z; } }; }
     struct S{}
   )cpp";
TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
   EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) << "Decl in current file";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header"), 42));
   EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6f) << "Decl from header";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header_main"), 42));
   EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
-      << "Current file and header";
+      << "Current file and definition in header";
+
+  Relevance = {};
+  const auto SymbolName = "Bar";
+  const auto *Shadow =
+      *dyn_cast<UsingDecl>(
+           &findAnyDecl(AST,
+                        [&](const NamedDecl &ND) -> bool {
+                          if (const UsingDecl *Using = dyn_cast<UsingDecl>(&ND))
+                            if (Using->shadow_size() &&
+                                Using->getQualifiedNameAsString() == SymbolName)
+                              return true;
+                          return false;
+                        }))
+           ->shadow_begin();
+  CodeCompletionResult Result(Shadow->getTargetDecl(), 42);
+  Result.ShadowDecl = Shadow;
+  Relevance.merge(Result);
+  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+      << "Using declaration in main file";
 
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "X"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "y"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::ClassScope);
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "z"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FunctionScope);
   // The injected class name is treated as the outer class name.
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "S::S"), 42));

clang/include/clang/Sema/CodeCompleteConsumer.h

 class DeclContext;
 class FunctionDecl;
 class FunctionTemplateDecl;
 class IdentifierInfo;
 class LangOptions;
 class NamedDecl;
 class NestedNameSpecifier;
 class Preprocessor;
-class Sema;
 class RawComment;
+class Sema;
+class UsingShadowDecl;
 
 /// Default priority values for code-completion results based
 /// on their kind.
 enum {
   /// Priority for the next initialization in a constructor initializer
   /// list.
   CCP_NextInitializer = 7,
 
public:
   /// rather than a use of that entity.
   bool DeclaringEntity : 1;
 
   /// If the result should have a nested-name-specifier, this is it.
   /// When \c QualifierIsInformative, the nested-name-specifier is
   /// informative rather than required.
   NestedNameSpecifier *Qualifier = nullptr;
 
+  /// If this Decl was unshadowed by using declaration, this can store a
+  /// pointer to the UsingShadowDecl which was used in the unshadowing process.
+  /// This information can be used to uprank CodeCompletionResults / which have
+  /// corresponding `using decl::qualified::name;` nearby.
+  const UsingShadowDecl *ShadowDecl = nullptr;
+
   /// Build a result that refers to a declaration.
   CodeCompletionResult(const NamedDecl *Declaration, unsigned Priority,
                        NestedNameSpecifier *Qualifier = nullptr,
                        bool QualifierIsInformative = false,
                        bool Accessible = true,
                        std::vector<FixItHint> FixIts = std::vector<FixItHint>())
       : Declaration(Declaration), Priority(Priority), Kind(RK_Declaration),
         FixIts(std::move(FixIts)), Hidden(false),
         QualifierIsInformative(QualifierIsInformative),
         StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
         DeclaringEntity(false), Qualifier(Qualifier) {
-    //FIXME: Add assert to check FixIts range requirements.
+    // FIXME: Add assert to check FixIts range requirements.
     computeCursorKindAndAvailability(Accessible);
   }
 
   /// Build a result that refers to a keyword or symbol.
   CodeCompletionResult(const char *Keyword, unsigned Priority = CCP_Keyword)
       : Keyword(Keyword), Priority(Priority), Kind(RK_Keyword),
         CursorKind(CXCursor_NotImplemented), Hidden(false),
         QualifierIsInformative(false), StartsNestedNameSpecifier(false),
public:
   }
 
   /// Retrieve the keyword stored in this result.
   const char *getKeyword() const {
     assert(Kind == RK_Keyword && "Not a keyword result");
     return Keyword;
   }
 
   /// Create a new code-completion string that describes how to insert
   /// this result into a program.
   ///
   /// \param S The semantic analysis that created the result.
   ///
   /// \param Allocator The allocator that will be used to allocate the
   /// string itself.
   CodeCompletionString *CreateCodeCompletionString(Sema &S,
                                          const CodeCompletionContext &CCContext,

clang/lib/Sema/SemaCodeComplete.cpp

void ResultBuilder::MaybeAddResult(Result R, DeclContext *CurContext) {
 
   if (R.Kind != Result::RK_Declaration) {
     // For non-declaration results, just add the result.
     Results.push_back(R);
     return;
   }
 
   // Look through using declarations.
-  if (const UsingShadowDecl *Using =
-          dyn_cast<UsingShadowDecl>(R.Declaration)) {
-    MaybeAddResult(Result(Using->getTargetDecl(),
+  if (const UsingShadowDecl *Using = dyn_cast<UsingShadowDecl>(R.Declaration)) {
+    CodeCompletionResult Result(Using->getTargetDecl(),
                                 getBasePriority(Using->getTargetDecl()),
-                          R.Qualifier),
-                   CurContext);
+                                R.Qualifier);
+    Result.ShadowDecl = Using;
+    MaybeAddResult(Result, CurContext);
     return;
   }
 
   const Decl *CanonDecl = R.Declaration->getCanonicalDecl();
   unsigned IDNS = CanonDecl->getIdentifierNamespace();
 
   bool AsNestedNameSpecifier = false;
   if (!isInterestingDecl(R.Declaration, AsNestedNameSpecifier))
void ResultBuilder::AddResult(Result R, DeclContext *CurContext,
   if (R.Kind != Result::RK_Declaration) {
     // For non-declaration results, just add the result.
     Results.push_back(R);
     return;
   }
 
   // Look through using declarations.
   if (const UsingShadowDecl *Using = dyn_cast<UsingShadowDecl>(R.Declaration)) {
-    AddResult(Result(Using->getTargetDecl(),
+    CodeCompletionResult Result(Using->getTargetDecl(),
                                 getBasePriority(Using->getTargetDecl()),
-                     R.Qualifier),
-              CurContext, Hiding);
+                                R.Qualifier);
+    Result.ShadowDecl = Using;
+    AddResult(Result, CurContext, Hiding);
     return;
   }
 
   bool AsNestedNameSpecifier = false;
   if (!isInterestingDecl(R.Declaration, AsNestedNameSpecifier))
     return;
 
   // C++ constructors are never found by name lookup.
   if (isConstructor(R.Declaration))
     return;
 
   if (Hiding && CheckHiddenResult(R, CurContext, Hiding))
     return;
 
   // Make sure that any given declaration only shows up in the result set once.
   if (!AllDeclsFound.insert(R.Declaration->getCanonicalDecl()).second)
     return;
 
   // If the filter is for nested-name-specifiers, then this result starts a
   // nested-name-specifier.
   if (AsNestedNameSpecifier) {
     R.StartsNestedNameSpecifier = true;
     R.Priority = CCP_NestedNameSpecifier;
-  }
-  else if (Filter == &ResultBuilder::IsMember && !R.Qualifier && InBaseClass &&
-           isa<CXXRecordDecl>(R.Declaration->getDeclContext()
-                                                  ->getRedeclContext()))
+  } else if (Filter == &ResultBuilder::IsMember && !R.Qualifier &&
+             InBaseClass &&
+             isa<CXXRecordDecl>(
+                 R.Declaration->getDeclContext()->getRedeclContext()))
     R.QualifierIsInformative = true;
 
   // If this result is supposed to have an informative qualifier, add one.
   if (R.QualifierIsInformative && !R.Qualifier &&
       !R.StartsNestedNameSpecifier) {
     const DeclContext *Ctx = R.Declaration->getDeclContext();
     if (const NamespaceDecl *Namespace = dyn_cast<NamespaceDecl>(Ctx))
       R.Qualifier = NestedNameSpecifier::Create(SemaRef.Context, nullptr,