[clang-tools-extra] d0817b5 - [clangd] Extract symbol-scope logic out of Quality, add tests. NFC
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 05:44:37 PST 2021
Author: Sam McCall
Date: 2021-01-29T14:44:28+01:00
New Revision: d0817b5f18c7bb435012d214f293d4a7839e492e
URL: https://github.com/llvm/llvm-project/commit/d0817b5f18c7bb435012d214f293d4a7839e492e
DIFF: https://github.com/llvm/llvm-project/commit/d0817b5f18c7bb435012d214f293d4a7839e492e.diff
LOG: [clangd] Extract symbol-scope logic out of Quality, add tests. NFC
This prepares for reuse from the semantic highlighting code.
There's a bit of yak-shaving here:
- when the enum is moved into the clangd namespace, promote it to a
scoped enum. This means teaching the decision forest infrastructure
to deal with scoped enums.
- AccessibleScope isn't quite the right name: e.g. public class members
are treated as accessible, but still have class scope. So rename to
SymbolScope.
- Rename some QualitySignals members to avoid name conflicts.
(the string) SymbolScope -> Scope
(the enum) Scope -> ScopeKind
Added:
Modified:
clang-tools-extra/clangd/AST.cpp
clang-tools-extra/clangd/AST.h
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Quality.cpp
clang-tools-extra/clangd/Quality.h
clang-tools-extra/clangd/quality/CompletionModelCodegen.py
clang-tools-extra/clangd/quality/model/features.json
clang-tools-extra/clangd/unittests/ASTTests.cpp
clang-tools-extra/clangd/unittests/QualityTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index aecaf7e6b8f7..f71d935c204c 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -478,5 +478,29 @@ bool hasUnstableLinkage(const Decl *D) {
return VD && !VD->getType().isNull() && VD->getType()->isUndeducedType();
}
+SymbolScope symbolScope(const NamedDecl &D) {
+ // Injected "Foo" within the class "Foo" has file scope, not class scope.
+ const DeclContext *DC = D.getDeclContext();
+ if (auto *R = dyn_cast<RecordDecl>(&D))
+ if (R->isInjectedClassName())
+ DC = DC->getParent();
+ // Class constructor should have the same scope as the class.
+ if (isa<CXXConstructorDecl>(D))
+ DC = DC->getParent();
+ bool InClass = false;
+ for (; !DC->isFileContext(); DC = DC->getParent()) {
+ if (DC->isFunctionOrMethod())
+ return SymbolScope::FunctionScope;
+ InClass = InClass || DC->isRecord();
+ }
+ if (InClass)
+ return SymbolScope::ClassScope;
+ // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
+ // Avoid caching linkage if it may change after enclosing code completion.
+ if (hasUnstableLinkage(&D) || D.getLinkageInternal() < ExternalLinkage)
+ return SymbolScope::FileScope;
+ return SymbolScope::GlobalScope;
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h
index b603964189e8..1baf8c585fd2 100644
--- a/clang-tools-extra/clangd/AST.h
+++ b/clang-tools-extra/clangd/AST.h
@@ -162,6 +162,15 @@ std::string getQualification(ASTContext &Context,
/// the cached value is incorrect. (clang catches this with an assertion).
bool hasUnstableLinkage(const Decl *D);
+/// An approximate measure of where we expect a symbol to be used.
+enum class SymbolScope {
+ FunctionScope,
+ ClassScope,
+ FileScope,
+ GlobalScope,
+};
+SymbolScope symbolScope(const NamedDecl &D);
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 1d1027319dfb..eb0df5fd4dd9 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -1726,7 +1726,7 @@ class CodeCompleteFlow {
}
if (Candidate.IdentifierResult) {
Quality.References = Candidate.IdentifierResult->References;
- Relevance.Scope = SymbolRelevanceSignals::FileScope;
+ Relevance.ScopeKind = SymbolScope::FileScope;
Origin |= SymbolOrigin::Identifier;
}
}
diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp
index b49392bc7d04..28b44bd0d4a1 100644
--- a/clang-tools-extra/clangd/Quality.cpp
+++ b/clang-tools-extra/clangd/Quality.cpp
@@ -262,37 +262,12 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
return OS;
}
-static SymbolRelevanceSignals::AccessibleScope
-computeScope(const NamedDecl *D) {
- // Injected "Foo" within the class "Foo" has file scope, not class scope.
- const DeclContext *DC = D->getDeclContext();
- if (auto *R = dyn_cast_or_null<RecordDecl>(D))
- if (R->isInjectedClassName())
- DC = DC->getParent();
- // Class constructor should have the same scope as the class.
- if (isa<CXXConstructorDecl>(D))
- DC = DC->getParent();
- bool InClass = false;
- for (; !DC->isFileContext(); DC = DC->getParent()) {
- if (DC->isFunctionOrMethod())
- return SymbolRelevanceSignals::FunctionScope;
- InClass = InClass || DC->isRecord();
- }
- if (InClass)
- return SymbolRelevanceSignals::ClassScope;
- // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
- // Avoid caching linkage if it may change after enclosing code completion.
- if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage)
- return SymbolRelevanceSignals::FileScope;
- return SymbolRelevanceSignals::GlobalScope;
-}
-
void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
SymbolURI = IndexResult.CanonicalDeclaration.FileURI;
- SymbolScope = IndexResult.Scope;
+ Scope = IndexResult.Scope;
IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);
if (!(IndexResult.Flags & Symbol::VisibleOutsideFile)) {
- Scope = AccessibleScope::FileScope;
+ ScopeKind = SymbolScope::FileScope;
}
if (MainFileSignals) {
MainFileRefs =
@@ -350,7 +325,7 @@ void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
computeASTSignals(SemaCCResult);
// Declarations are scoped, others (like macros) are assumed global.
if (SemaCCResult.Declaration)
- Scope = std::min(Scope, computeScope(SemaCCResult.Declaration));
+ ScopeKind = std::min(ScopeKind, symbolScope(*SemaCCResult.Declaration));
NeedsFixIts = !SemaCCResult.FixIts.empty();
}
@@ -393,7 +368,7 @@ SymbolRelevanceSignals::calculateDerivedSignals() const {
if (ScopeProximityMatch) {
// For global symbol, the distance is 0.
Derived.ScopeProximityDistance =
- SymbolScope ? ScopeProximityMatch->distance(*SymbolScope) : 0;
+ Scope ? ScopeProximityMatch->distance(*Scope) : 0;
}
return Derived;
}
@@ -429,26 +404,26 @@ float SymbolRelevanceSignals::evaluateHeuristics() const {
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:
+ switch (ScopeKind) {
+ case SymbolScope::GlobalScope:
break;
- case FileScope:
+ case SymbolScope::FileScope:
Score *= 1.5f;
break;
- case ClassScope:
+ case SymbolScope::ClassScope:
Score *= 2;
break;
- case FunctionScope:
+ case SymbolScope::FunctionScope:
Score *= 4;
break;
}
} else {
// For non-completion queries, the wider the scope where a symbol is
// visible, the more likely it is to be relevant.
- switch (Scope) {
- case GlobalScope:
+ switch (ScopeKind) {
+ case SymbolScope::GlobalScope:
break;
- case FileScope:
+ case SymbolScope::FileScope:
Score *= 0.5f;
break;
default:
@@ -507,11 +482,11 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
OS << llvm::formatv("\tInBaseClass: {0}\n", S.InBaseClass);
OS << llvm::formatv("\tContext: {0}\n", getCompletionKindString(S.Context));
OS << llvm::formatv("\tQuery type: {0}\n", static_cast<int>(S.Query));
- OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.Scope));
+ OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.ScopeKind));
OS << llvm::formatv("\tSymbol URI: {0}\n", S.SymbolURI);
OS << llvm::formatv("\tSymbol scope: {0}\n",
- S.SymbolScope ? *S.SymbolScope : "<None>");
+ S.Scope ? *S.Scope : "<None>");
SymbolRelevanceSignals::DerivedSignals Derived = S.calculateDerivedSignals();
if (S.FileProximityMatch) {
@@ -568,7 +543,7 @@ evaluateDecisionForest(const SymbolQualitySignals &Quality,
E.setSemaFileProximityScore(Relevance.SemaFileProximityScore);
E.setSymbolScopeDistanceCost(Derived.ScopeProximityDistance);
E.setSemaSaysInScope(Relevance.SemaSaysInScope);
- E.setScope(Relevance.Scope);
+ E.setScope(static_cast<uint32_t>(Relevance.ScopeKind));
E.setContextKind(Relevance.Context);
E.setIsInstanceMember(Relevance.IsInstanceMember);
E.setHadContextType(Relevance.HadContextType);
diff --git a/clang-tools-extra/clangd/Quality.h b/clang-tools-extra/clangd/Quality.h
index 08a8981865de..f4b925e5efbb 100644
--- a/clang-tools-extra/clangd/Quality.h
+++ b/clang-tools-extra/clangd/Quality.h
@@ -108,17 +108,11 @@ struct SymbolRelevanceSignals {
// Scope proximity is only considered (both index and sema) when this is set.
ScopeDistance *ScopeProximityMatch = nullptr;
- llvm::Optional<llvm::StringRef> SymbolScope;
+ llvm::Optional<llvm::StringRef> Scope;
// A symbol from sema should be accessible from the current scope.
bool SemaSaysInScope = false;
- // An approximate measure of where we expect the symbol to be used.
- enum AccessibleScope {
- FunctionScope,
- ClassScope,
- FileScope,
- GlobalScope,
- } Scope = GlobalScope;
+ SymbolScope ScopeKind = SymbolScope::GlobalScope;
enum QueryType {
CodeComplete,
diff --git a/clang-tools-extra/clangd/quality/CompletionModelCodegen.py b/clang-tools-extra/clangd/quality/CompletionModelCodegen.py
index f27e8f6a28a3..c93f76d23002 100644
--- a/clang-tools-extra/clangd/quality/CompletionModelCodegen.py
+++ b/clang-tools-extra/clangd/quality/CompletionModelCodegen.py
@@ -245,7 +245,7 @@ def gen_cpp_code(forest_json, features_json, filename, cpp_class):
%s
-#define BIT(X) (1 << X)
+#define BIT(X) (1u << unsigned(X))
%s
diff --git a/clang-tools-extra/clangd/quality/model/features.json b/clang-tools-extra/clangd/quality/model/features.json
index 2f68848ce807..7831506d7f14 100644
--- a/clang-tools-extra/clangd/quality/model/features.json
+++ b/clang-tools-extra/clangd/quality/model/features.json
@@ -78,7 +78,7 @@
{
"name": "Scope",
"kind": "ENUM",
- "type": "clang::clangd::SymbolRelevanceSignals::AccessibleScope",
- "header": "Quality.h"
+ "type": "clang::clangd::SymbolScope",
+ "header": "AST.h"
}
]
diff --git a/clang-tools-extra/clangd/unittests/ASTTests.cpp b/clang-tools-extra/clangd/unittests/ASTTests.cpp
index 4c52c72d703c..df5af23abc7e 100644
--- a/clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -10,6 +10,7 @@
#include "Annotations.h"
#include "ParsedAST.h"
+#include "SourceCode.h"
#include "TestTU.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
@@ -351,6 +352,76 @@ TEST(ClangdAST, PrintType) {
}
}
}
+
+TEST(ASTTest, SymbolScope) {
+ const struct {
+ const char*Code;
+ SymbolScope Expected;
+ } Cases[] = {
+ {
+ "int ^x;",
+ SymbolScope::GlobalScope,
+ },
+ {
+ "static int ^x;",
+ SymbolScope::FileScope,
+ },
+ {
+ "namespace foo { int ^x; }",
+ SymbolScope::GlobalScope,
+ },
+ {
+ "namespace { int ^x; }",
+ SymbolScope::FileScope,
+ },
+ {
+ "class X{ void ^Y(); };",
+ SymbolScope::ClassScope,
+ },
+ {
+ "class X{ ^X(); };",
+ SymbolScope::GlobalScope,
+ },
+ {
+ "class X{ ^~X(); };",
+ SymbolScope::ClassScope,
+ },
+ {
+ "void x() { int ^y; }",
+ SymbolScope::FunctionScope,
+ },
+ {
+ "void x(int ^y) {}",
+ SymbolScope::FunctionScope,
+ },
+ {
+ "void x() { class ^Y{}; }",
+ SymbolScope::FunctionScope,
+ },
+ {
+ "void x() { class Y{ ^Y(); }; }",
+ SymbolScope::FunctionScope,
+ },
+ {
+ "void x() {auto y = [^z(0)]{};}",
+ SymbolScope::FunctionScope,
+ },
+ };
+
+ for (const auto& Case : Cases) {
+ SCOPED_TRACE(Case.Code);
+ Annotations Test(Case.Code);
+ auto AST = TestTU::withCode(Test.code()).build();
+
+ SourceLocation Loc = cantFail(
+ sourceLocationInMainFile(AST.getSourceManager(), Test.point()));
+ const NamedDecl &D = findDecl(
+ AST, [&](const NamedDecl &D) { return D.getLocation() == Loc; });
+
+ EXPECT_EQ(symbolScope(D), Case.Expected);
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/QualityTests.cpp b/clang-tools-extra/clangd/unittests/QualityTests.cpp
index f5fcb0e8d04e..bffbab49302d 100644
--- a/clang-tools-extra/clangd/unittests/QualityTests.cpp
+++ b/clang-tools-extra/clangd/unittests/QualityTests.cpp
@@ -122,7 +122,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
/*Accessible=*/false));
EXPECT_EQ(Relevance.NameMatch, SymbolRelevanceSignals().NameMatch);
EXPECT_TRUE(Relevance.Forbidden);
- EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
+ EXPECT_EQ(Relevance.ScopeKind, SymbolScope::GlobalScope);
Relevance = {};
Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
@@ -160,17 +160,17 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
Relevance = {};
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "X"), 42));
- EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
+ EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope);
Relevance = {};
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "y"), 42));
- EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::ClassScope);
+ EXPECT_EQ(Relevance.ScopeKind, SymbolScope::ClassScope);
Relevance = {};
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "z"), 42));
- EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FunctionScope);
+ EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FunctionScope);
// The injected class name is treated as the outer class name.
Relevance = {};
Relevance.merge(CodeCompletionResult(&findDecl(AST, "S::S"), 42));
- EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
+ EXPECT_EQ(Relevance.ScopeKind, SymbolScope::GlobalScope);
Relevance = {};
EXPECT_FALSE(Relevance.InBaseClass);
@@ -188,7 +188,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
Matched = true;
Relevance = {};
Relevance.merge(S);
- EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
+ EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope);
});
EXPECT_TRUE(Matched);
}
@@ -263,7 +263,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) {
SymbolRelevanceSignals WithIndexScopeProximity;
WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
- WithIndexScopeProximity.SymbolScope = "x::";
+ WithIndexScopeProximity.Scope = "x::";
EXPECT_GT(WithSemaScopeProximity.evaluateHeuristics(),
Default.evaluateHeuristics());
@@ -282,7 +282,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) {
EXPECT_GT(IndexDistant.evaluateHeuristics(), Default.evaluateHeuristics());
SymbolRelevanceSignals Scoped;
- Scoped.Scope = SymbolRelevanceSignals::FileScope;
+ Scoped.ScopeKind = SymbolScope::FileScope;
EXPECT_LT(Scoped.evaluateHeuristics(), Default.evaluateHeuristics());
Scoped.Query = SymbolRelevanceSignals::CodeComplete;
EXPECT_GT(Scoped.evaluateHeuristics(), Default.evaluateHeuristics());
@@ -317,26 +317,26 @@ TEST(QualityTests, ScopeProximity) {
ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
Relevance.ScopeProximityMatch = &ScopeProximity;
- Relevance.SymbolScope = "other::";
+ Relevance.Scope = "other::";
float NotMatched = Relevance.evaluateHeuristics();
- Relevance.SymbolScope = "";
+ Relevance.Scope = "";
float Global = Relevance.evaluateHeuristics();
EXPECT_GT(Global, NotMatched);
- Relevance.SymbolScope = "llvm::";
+ Relevance.Scope = "llvm::";
float NonParent = Relevance.evaluateHeuristics();
EXPECT_GT(NonParent, Global);
- Relevance.SymbolScope = "x::";
+ Relevance.Scope = "x::";
float GrandParent = Relevance.evaluateHeuristics();
EXPECT_GT(GrandParent, Global);
- Relevance.SymbolScope = "x::y::";
+ Relevance.Scope = "x::y::";
float Parent = Relevance.evaluateHeuristics();
EXPECT_GT(Parent, GrandParent);
- Relevance.SymbolScope = "x::y::z::";
+ Relevance.Scope = "x::y::z::";
float Enclosing = Relevance.evaluateHeuristics();
EXPECT_GT(Enclosing, Parent);
}
@@ -375,8 +375,8 @@ TEST(QualityTests, NoBoostForClassConstructor) {
SymbolRelevanceSignals Ctor;
Ctor.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
- EXPECT_EQ(Cls.Scope, SymbolRelevanceSignals::GlobalScope);
- EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope);
+ EXPECT_EQ(Cls.ScopeKind, SymbolScope::GlobalScope);
+ EXPECT_EQ(Ctor.ScopeKind, SymbolScope::GlobalScope);
}
TEST(QualityTests, IsInstanceMember) {
More information about the cfe-commits
mailing list