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) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
