sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov, klimek.
This signal is considered a relevance rather than a quality signal because it's
dependent on the query (the fact that it's completion, and implicitly the query
context).
This is part of the effort to reduce reliance on Sema priority, so we can have
consistent ranking between Index and Sema results.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47762
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===================================================================
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -53,6 +53,8 @@
const Symbol &findSymbol(const SymbolSlab &, llvm::StringRef QName);
// Look up an AST symbol by qualified name, which must be unique and top-level.
const NamedDecl &findDecl(ParsedAST &AST, llvm::StringRef QName);
+// Look up a main-file AST symbol by unqualified name, which must be unique.
+const NamedDecl &findAnyDecl(ParsedAST &AST, llvm::StringRef Name);
} // namespace clangd
} // namespace clang
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -10,6 +10,7 @@
#include "TestFS.h"
#include "index/FileIndex.h"
#include "index/MemIndex.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
#include "clang/Frontend/Utils.h"
@@ -49,7 +50,6 @@
return MemIndex::build(headerSymbols());
}
-// Look up a symbol by qualified name, which must be unique.
const Symbol &findSymbol(const SymbolSlab &Slab, llvm::StringRef QName) {
const Symbol *Result = nullptr;
for (const Symbol &S : Slab) {
@@ -92,5 +92,26 @@
return LookupDecl(*Scope, Components.back());
}
+const NamedDecl &findAnyDecl(ParsedAST &AST, llvm::StringRef Name) {
+ struct Visitor : RecursiveASTVisitor<Visitor> {
+ llvm::StringRef Name;
+ llvm::SmallVector<const NamedDecl *, 1> Decls;
+ bool VisitNamedDecl(const NamedDecl *ND) {
+ if (auto *ID = ND->getIdentifier())
+ if (ID->getName() == Name)
+ Decls.push_back(ND);
+ return true;
+ }
+ } Visitor;
+ Visitor.Name = Name;
+ for (Decl *D : AST.getLocalTopLevelDecls())
+ Visitor.TraverseDecl(D);
+ if (Visitor.Decls.size() != 1) {
+ ADD_FAILURE() << Visitor.Decls.size() << " symbols named " << Name;
+ assert(Visitor.Decls.size() == 1);
+ }
+ return *Visitor.Decls.front();
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/QualityTests.cpp
===================================================================
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -61,14 +61,27 @@
auto AST = TestTU::withHeaderCode(R"cpp(
[[deprecated]]
int f() { return 0; }
+
+ namespace { struct X { void y() { int z; } }; }
)cpp")
.build();
SymbolRelevanceSignals Relevance;
Relevance.merge(CodeCompletionResult(&findDecl(AST, "f"), /*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(&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);
}
// Do the signals move the scores in the direction we expect?
@@ -104,6 +117,12 @@
SymbolRelevanceSignals PoorNameMatch;
PoorNameMatch.NameMatch = 0.2f;
EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
+
+ 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) {
Index: clangd/Quality.h
===================================================================
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -64,8 +64,19 @@
// 0-1 fuzzy-match score for unqualified name. Must be explicitly assigned.
float NameMatch = 1;
bool Forbidden = false; // Unavailable (e.g const) or inaccessible (private).
+ enum AccessibleScope {
+ FunctionScope,
+ ClassScope,
+ FileScope,
+ GlobalScope,
+ } Scope = GlobalScope;
+ enum QueryType {
+ CodeComplete,
+ Generic,
+ } Query = Generic;
void merge(const CodeCompletionResult &SemaResult);
+ void merge(const Symbol &IndexResult);
// Condense these signals down to a single number, higher is better.
float evaluate() const;
Index: clangd/Quality.cpp
===================================================================
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -56,16 +56,62 @@
return OS;
}
+static SymbolRelevanceSignals::AccessibleScope
+ComputeScope(const NamedDecl &D) {
+ bool InClass;
+ for (const DeclContext *DC = D.getDeclContext(); DC != nullptr;
+ DC = DC->getParent()) {
+ if (DC->isFunctionOrMethod())
+ return SymbolRelevanceSignals::FunctionScope;
+ InClass = InClass || isa<RecordDecl>(DC);
+ }
+ if (InClass)
+ return SymbolRelevanceSignals::ClassScope;
+ if (D.getLinkageInternal() < ExternalLinkage)
+ return SymbolRelevanceSignals::FileScope;
+ return SymbolRelevanceSignals::GlobalScope;
+}
+
+void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
+ // FIXME: Index results always assumed to be at global scope.
+}
+
void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
if (SemaCCResult.Availability == CXAvailability_NotAvailable ||
SemaCCResult.Availability == CXAvailability_NotAccessible)
Forbidden = true;
+
+ // Declarations are scoped, others (like macros) are assumed global.
+ if (SemaCCResult.Kind == CodeCompletionResult::RK_Declaration)
+ Scope = std::min(Scope, ComputeScope(*SemaCCResult.Declaration));
}
float SymbolRelevanceSignals::evaluate() const {
+ float Score = 1;
+
if (Forbidden)
return 0;
- return NameMatch;
+
+ Score *= NameMatch;
+
+ // Symbols like local variables may only be referenced within their scope.
+ // Conversely if we're in that scope, it's likely we'll reference them.
+ if (Query == CodeComplete) {
+ // The narrower the scope where a symbol is visible, the more likely it is
+ // to be relevant when it is available.
+ switch (Scope) {
+ case GlobalScope:
+ break;
+ case FileScope:
+ Score *= 1.5;
+ case ClassScope:
+ Score *= 2;
+ case FunctionScope:
+ Score *= 4;
+ }
+ }
+
+ return Score;
}
raw_ostream &operator<<(raw_ostream &OS, const SymbolRelevanceSignals &S) {
OS << formatv("=== Symbol relevance: {0}\n", S.evaluate());
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -1005,12 +1005,15 @@
SymbolQualitySignals Quality;
SymbolRelevanceSignals Relevance;
+ Relevance.Query = SymbolRelevanceSignals::CodeComplete;
if (auto FuzzyScore = Filter->match(C.Name))
Relevance.NameMatch = *FuzzyScore;
else
return;
- if (IndexResult)
+ if (IndexResult) {
Quality.merge(*IndexResult);
+ Relevance.merge(*IndexResult);
+ }
if (SemaResult) {
Quality.merge(*SemaResult);
Relevance.merge(*SemaResult);
Index: clangd/ClangdUnit.h
===================================================================
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -91,7 +91,8 @@
/// This function returns top-level decls present in the main file of the AST.
/// The result does not include the decls that come from the preamble.
- ArrayRef<const Decl *> getLocalTopLevelDecls();
+ /// (These should be const, but RecursiveASTVisitor requires Decl*).
+ ArrayRef<Decl *> getLocalTopLevelDecls();
const std::vector<Diag> &getDiagnostics() const;
@@ -104,8 +105,8 @@
ParsedAST(std::shared_ptr<const PreambleData> Preamble,
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
- std::vector<const Decl *> LocalTopLevelDecls,
- std::vector<Diag> Diags, std::vector<Inclusion> Inclusions);
+ std::vector<Decl *> LocalTopLevelDecls, std::vector<Diag> Diags,
+ std::vector<Inclusion> Inclusions);
// In-memory preambles must outlive the AST, it is important that this member
// goes before Clang and Action.
@@ -122,7 +123,7 @@
std::vector<Diag> Diags;
// Top-level decls inside the current file. Not that this does not include
// top-level decls from the preamble.
- std::vector<const Decl *> LocalTopLevelDecls;
+ std::vector<Decl *> LocalTopLevelDecls;
std::vector<Inclusion> Inclusions;
};
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -51,11 +51,11 @@
class DeclTrackingASTConsumer : public ASTConsumer {
public:
- DeclTrackingASTConsumer(std::vector<const Decl *> &TopLevelDecls)
+ DeclTrackingASTConsumer(std::vector<Decl *> &TopLevelDecls)
: TopLevelDecls(TopLevelDecls) {}
bool HandleTopLevelDecl(DeclGroupRef DG) override {
- for (const Decl *D : DG) {
+ for (Decl *D : DG) {
// ObjCMethodDecl are not actually top-level decls.
if (isa<ObjCMethodDecl>(D))
continue;
@@ -66,23 +66,21 @@
}
private:
- std::vector<const Decl *> &TopLevelDecls;
+ std::vector<Decl *> &TopLevelDecls;
};
class ClangdFrontendAction : public SyntaxOnlyAction {
public:
- std::vector<const Decl *> takeTopLevelDecls() {
- return std::move(TopLevelDecls);
- }
+ std::vector<Decl *> takeTopLevelDecls() { return std::move(TopLevelDecls); }
protected:
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
StringRef InFile) override {
return llvm::make_unique<DeclTrackingASTConsumer>(/*ref*/ TopLevelDecls);
}
private:
- std::vector<const Decl *> TopLevelDecls;
+ std::vector<Decl *> TopLevelDecls;
};
class CppFilePreambleCallbacks : public PreambleCallbacks {
@@ -174,7 +172,7 @@
// CompilerInstance won't run this callback, do it directly.
ASTDiags.EndSourceFile();
- std::vector<const Decl *> ParsedDecls = Action->takeTopLevelDecls();
+ std::vector<Decl *> ParsedDecls = Action->takeTopLevelDecls();
std::vector<Diag> Diags = ASTDiags.take();
// Add diagnostics from the preamble, if any.
if (Preamble)
@@ -210,7 +208,7 @@
return Clang->getPreprocessor();
}
-ArrayRef<const Decl *> ParsedAST::getLocalTopLevelDecls() {
+ArrayRef<Decl *> ParsedAST::getLocalTopLevelDecls() {
return LocalTopLevelDecls;
}
@@ -261,7 +259,7 @@
ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
- std::vector<const Decl *> LocalTopLevelDecls,
+ std::vector<Decl *> LocalTopLevelDecls,
std::vector<Diag> Diags, std::vector<Inclusion> Inclusions)
: Preamble(std::move(Preamble)), Clang(std::move(Clang)),
Action(std::move(Action)), Diags(std::move(Diags)),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
