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

(On Diff #154377)

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 ↗

(On Diff #154377)

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

122 ↗

(On Diff #154377)

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

128 ↗

(On Diff #154377)

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?

131 ↗

(On Diff #154377)

doesn't this crash if there are no shadows?

133 ↗

(On Diff #154377)

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

146 ↗

(On Diff #154377)

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 ↗

(On Diff #154377)

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

911 ↗

(On Diff #154377)

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

48 ↗

(On Diff #154379)

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

48 ↗

(On Diff #154379)

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 ↗

(On Diff #154383)

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

119 ↗

(On Diff #154383)

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.

clangd/Quality.cpp

static bool hasDeclInMainFile(const Decl &D) {
   for (auto *Redecl : D.redecls()) {
     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) {
+    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));
 }
 

unittests/clangd/QualityTests.cpp

TEST(QualityTests, SymbolQualitySignalExtraction) {
   Quality = {};
   Quality.merge(CodeCompletionResult("if"));
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Keyword);
 }
 
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
-    int header();
-    int header_main();
-    )cpp";
+
+  namespace hdr { class Bar {}; } // namespace hdr
+
+  #define DEFINE_FLAG(X) \
+  namespace flags { \
+  int FLAGS_##X; \
+  } \
+
+  DEFINE_FLAG(FOO)
   Test.Code = R"cpp(
-    int ::header_main() {}
-    int main();
+  using hdr::Bar;
+
+  using flags::FLAGS_FOO;
+
 
-    [[deprecated]]
-    int deprecated() { return 0; }
 
-    namespace { struct X { void y() { int z; } }; }
-    struct S{}
   )cpp";
   auto AST = Test.build();
 
   SymbolRelevanceSignals Relevance;
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "deprecated"),
                                        /*Priority=*/42, nullptr, false,
                                        /*Accessible=*/false));
   EXPECT_EQ(Relevance.NameMatch, SymbolRelevanceSignals().NameMatch);
   EXPECT_TRUE(Relevance.Forbidden);
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
 
   Relevance = {};
   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";
 
+  auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
+    auto *Shadow =
+        *dyn_cast<UsingDecl>(
+             &findAnyDecl(AST,
+                          [&](const NamedDecl &ND) {
+                            if (const UsingDecl *Using =
+                                    dyn_cast<UsingDecl>(&ND))
+                              if (Using->shadow_size() &&
+                                  Using->getQualifiedNameAsString() == DeclName)
+                                return true;
+                            return false;
+                          }))
+             ->shadow_begin();
+    CodeCompletionResult Result(Shadow->getTargetDecl(), 42);
+    Result.ShadowDecl = Shadow;
+    return Result;
+  };
+
+  Relevance = {};
+  Relevance.merge(constructShadowDeclCompletionResult("Bar"));
+  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+      << "Using declaration in main file";
+  Relevance.merge(constructShadowDeclCompletionResult("FLAGS_FOO"));
+  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));
TEST(QualityTests, SymbolRelevanceSignalsSanity) {
   SymbolRelevanceSignals Scoped;
   Scoped.Scope = SymbolRelevanceSignals::FileScope;
   EXPECT_EQ(Scoped.evaluate(), Default.evaluate());
   Scoped.Query = SymbolRelevanceSignals::CodeComplete;
   EXPECT_GT(Scoped.evaluate(), Default.evaluate());
 }
 
 TEST(QualityTests, SortText) {
-  EXPECT_LT(sortText(std::numeric_limits<float>::infinity()), sortText(1000.2f));
+  EXPECT_LT(sortText(std::numeric_limits<float>::infinity()),
+            sortText(1000.2f));
   EXPECT_LT(sortText(1000.2f), sortText(1));
   EXPECT_LT(sortText(1), sortText(0.3f));
   EXPECT_LT(sortText(0.3f), sortText(0));
   EXPECT_LT(sortText(0), sortText(-10));
   EXPECT_LT(sortText(-10), sortText(-std::numeric_limits<float>::infinity()));
 
   EXPECT_LT(sortText(1, "z"), sortText(0, "a"));
   EXPECT_LT(sortText(0, "a"), sortText(0, "z"));