Author: Sam McCall Date: 2021-01-29T14:59:16+01:00 New Revision: 7d1b499caef6ebde09a2697a97b43b89f7fa35c8
URL: https://github.com/llvm/llvm-project/commit/7d1b499caef6ebde09a2697a97b43b89f7fa35c8 DIFF: https://github.com/llvm/llvm-project/commit/7d1b499caef6ebde09a2697a97b43b89f7fa35c8.diff LOG: Revert "[clangd] Extract symbol-scope logic out of Quality, add tests. NFC" On second thought, this can't properly be reused for highlighting. Consider this example, which Quality wants to consider function-scope, but highlighting must consider class-scope: void foo() { class X { int ^y; }; } 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 f71d935c204c..aecaf7e6b8f7 100644 --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -478,29 +478,5 @@ 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 1baf8c585fd2..b603964189e8 100644 --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -162,15 +162,6 @@ 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 eb0df5fd4dd9..1d1027319dfb 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.ScopeKind = SymbolScope::FileScope; + Relevance.Scope = SymbolRelevanceSignals::FileScope; Origin |= SymbolOrigin::Identifier; } } diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp index 28b44bd0d4a1..b49392bc7d04 100644 --- a/clang-tools-extra/clangd/Quality.cpp +++ b/clang-tools-extra/clangd/Quality.cpp @@ -262,12 +262,37 @@ 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; - Scope = IndexResult.Scope; + SymbolScope = IndexResult.Scope; IsInstanceMember |= isInstanceMember(IndexResult.SymInfo); if (!(IndexResult.Flags & Symbol::VisibleOutsideFile)) { - ScopeKind = SymbolScope::FileScope; + Scope = AccessibleScope::FileScope; } if (MainFileSignals) { MainFileRefs = @@ -325,7 +350,7 @@ void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) { computeASTSignals(SemaCCResult); // Declarations are scoped, others (like macros) are assumed global. if (SemaCCResult.Declaration) - ScopeKind = std::min(ScopeKind, symbolScope(*SemaCCResult.Declaration)); + Scope = std::min(Scope, computeScope(SemaCCResult.Declaration)); NeedsFixIts = !SemaCCResult.FixIts.empty(); } @@ -368,7 +393,7 @@ SymbolRelevanceSignals::calculateDerivedSignals() const { if (ScopeProximityMatch) { // For global symbol, the distance is 0. Derived.ScopeProximityDistance = - Scope ? ScopeProximityMatch->distance(*Scope) : 0; + SymbolScope ? ScopeProximityMatch->distance(*SymbolScope) : 0; } return Derived; } @@ -404,26 +429,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 (ScopeKind) { - case SymbolScope::GlobalScope: + switch (Scope) { + case GlobalScope: break; - case SymbolScope::FileScope: + case FileScope: Score *= 1.5f; break; - case SymbolScope::ClassScope: + case ClassScope: Score *= 2; break; - case SymbolScope::FunctionScope: + case 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 (ScopeKind) { - case SymbolScope::GlobalScope: + switch (Scope) { + case GlobalScope: break; - case SymbolScope::FileScope: + case FileScope: Score *= 0.5f; break; default: @@ -482,11 +507,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.ScopeKind)); + OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.Scope)); OS << llvm::formatv("\tSymbol URI: {0}\n", S.SymbolURI); OS << llvm::formatv("\tSymbol scope: {0}\n", - S.Scope ? *S.Scope : "<None>"); + S.SymbolScope ? *S.SymbolScope : "<None>"); SymbolRelevanceSignals::DerivedSignals Derived = S.calculateDerivedSignals(); if (S.FileProximityMatch) { @@ -543,7 +568,7 @@ evaluateDecisionForest(const SymbolQualitySignals &Quality, E.setSemaFileProximityScore(Relevance.SemaFileProximityScore); E.setSymbolScopeDistanceCost(Derived.ScopeProximityDistance); E.setSemaSaysInScope(Relevance.SemaSaysInScope); - E.setScope(static_cast<uint32_t>(Relevance.ScopeKind)); + E.setScope(Relevance.Scope); 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 f4b925e5efbb..08a8981865de 100644 --- a/clang-tools-extra/clangd/Quality.h +++ b/clang-tools-extra/clangd/Quality.h @@ -108,11 +108,17 @@ struct SymbolRelevanceSignals { // Scope proximity is only considered (both index and sema) when this is set. ScopeDistance *ScopeProximityMatch = nullptr; - llvm::Optional<llvm::StringRef> Scope; + llvm::Optional<llvm::StringRef> SymbolScope; // A symbol from sema should be accessible from the current scope. bool SemaSaysInScope = false; - SymbolScope ScopeKind = SymbolScope::GlobalScope; + // An approximate measure of where we expect the symbol to be used. + enum AccessibleScope { + FunctionScope, + ClassScope, + FileScope, + GlobalScope, + } Scope = GlobalScope; enum QueryType { CodeComplete, diff --git a/clang-tools-extra/clangd/quality/CompletionModelCodegen.py b/clang-tools-extra/clangd/quality/CompletionModelCodegen.py index c93f76d23002..f27e8f6a28a3 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) (1u << unsigned(X)) +#define BIT(X) (1 << X) %s diff --git a/clang-tools-extra/clangd/quality/model/features.json b/clang-tools-extra/clangd/quality/model/features.json index 7831506d7f14..2f68848ce807 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::SymbolScope", - "header": "AST.h" + "type": "clang::clangd::SymbolRelevanceSignals::AccessibleScope", + "header": "Quality.h" } ] diff --git a/clang-tools-extra/clangd/unittests/ASTTests.cpp b/clang-tools-extra/clangd/unittests/ASTTests.cpp index df5af23abc7e..4c52c72d703c 100644 --- a/clang-tools-extra/clangd/unittests/ASTTests.cpp +++ b/clang-tools-extra/clangd/unittests/ASTTests.cpp @@ -10,7 +10,6 @@ #include "Annotations.h" #include "ParsedAST.h" -#include "SourceCode.h" #include "TestTU.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclBase.h" @@ -352,76 +351,6 @@ 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 bffbab49302d..f5fcb0e8d04e 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.ScopeKind, SymbolScope::GlobalScope); + EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::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.ScopeKind, SymbolScope::FileScope); + EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope); Relevance = {}; Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "y"), 42)); - EXPECT_EQ(Relevance.ScopeKind, SymbolScope::ClassScope); + EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::ClassScope); Relevance = {}; Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "z"), 42)); - EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FunctionScope); + 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)); - EXPECT_EQ(Relevance.ScopeKind, SymbolScope::GlobalScope); + EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope); Relevance = {}; EXPECT_FALSE(Relevance.InBaseClass); @@ -188,7 +188,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) { Matched = true; Relevance = {}; Relevance.merge(S); - EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope); + EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope); }); EXPECT_TRUE(Matched); } @@ -263,7 +263,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) { SymbolRelevanceSignals WithIndexScopeProximity; WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity; - WithIndexScopeProximity.Scope = "x::"; + WithIndexScopeProximity.SymbolScope = "x::"; EXPECT_GT(WithSemaScopeProximity.evaluateHeuristics(), Default.evaluateHeuristics()); @@ -282,7 +282,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) { EXPECT_GT(IndexDistant.evaluateHeuristics(), Default.evaluateHeuristics()); SymbolRelevanceSignals Scoped; - Scoped.ScopeKind = SymbolScope::FileScope; + Scoped.Scope = SymbolRelevanceSignals::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.Scope = "other::"; + Relevance.SymbolScope = "other::"; float NotMatched = Relevance.evaluateHeuristics(); - Relevance.Scope = ""; + Relevance.SymbolScope = ""; float Global = Relevance.evaluateHeuristics(); EXPECT_GT(Global, NotMatched); - Relevance.Scope = "llvm::"; + Relevance.SymbolScope = "llvm::"; float NonParent = Relevance.evaluateHeuristics(); EXPECT_GT(NonParent, Global); - Relevance.Scope = "x::"; + Relevance.SymbolScope = "x::"; float GrandParent = Relevance.evaluateHeuristics(); EXPECT_GT(GrandParent, Global); - Relevance.Scope = "x::y::"; + Relevance.SymbolScope = "x::y::"; float Parent = Relevance.evaluateHeuristics(); EXPECT_GT(Parent, GrandParent); - Relevance.Scope = "x::y::z::"; + Relevance.SymbolScope = "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.ScopeKind, SymbolScope::GlobalScope); - EXPECT_EQ(Ctor.ScopeKind, SymbolScope::GlobalScope); + EXPECT_EQ(Cls.Scope, SymbolRelevanceSignals::GlobalScope); + EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope); } TEST(QualityTests, IsInstanceMember) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
