Skip to content

Commit 7d1b499

Browse files
sam-mccallsam-mccall
committed
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; }; }
1 parent d0817b5 commit 7d1b499

File tree

9 files changed

+68
-141
lines changed

9 files changed

+68
-141
lines changed

Diff for: ‎clang-tools-extra/clangd/AST.cpp

-24
Original file line numberDiff line numberDiff line change
@@ -478,29 +478,5 @@ bool hasUnstableLinkage(const Decl *D) {
478478
return VD && !VD->getType().isNull() && VD->getType()->isUndeducedType();
479479
}
480480

481-
SymbolScope symbolScope(const NamedDecl &D) {
482-
// Injected "Foo" within the class "Foo" has file scope, not class scope.
483-
const DeclContext *DC = D.getDeclContext();
484-
if (auto *R = dyn_cast<RecordDecl>(&D))
485-
if (R->isInjectedClassName())
486-
DC = DC->getParent();
487-
// Class constructor should have the same scope as the class.
488-
if (isa<CXXConstructorDecl>(D))
489-
DC = DC->getParent();
490-
bool InClass = false;
491-
for (; !DC->isFileContext(); DC = DC->getParent()) {
492-
if (DC->isFunctionOrMethod())
493-
return SymbolScope::FunctionScope;
494-
InClass = InClass || DC->isRecord();
495-
}
496-
if (InClass)
497-
return SymbolScope::ClassScope;
498-
// ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
499-
// Avoid caching linkage if it may change after enclosing code completion.
500-
if (hasUnstableLinkage(&D) || D.getLinkageInternal() < ExternalLinkage)
501-
return SymbolScope::FileScope;
502-
return SymbolScope::GlobalScope;
503-
}
504-
505481
} // namespace clangd
506482
} // namespace clang

Diff for: ‎clang-tools-extra/clangd/AST.h

-9
Original file line numberDiff line numberDiff line change
@@ -162,15 +162,6 @@ std::string getQualification(ASTContext &Context,
162162
/// the cached value is incorrect. (clang catches this with an assertion).
163163
bool hasUnstableLinkage(const Decl *D);
164164

165-
/// An approximate measure of where we expect a symbol to be used.
166-
enum class SymbolScope {
167-
FunctionScope,
168-
ClassScope,
169-
FileScope,
170-
GlobalScope,
171-
};
172-
SymbolScope symbolScope(const NamedDecl &D);
173-
174165
} // namespace clangd
175166
} // namespace clang
176167

Diff for: ‎clang-tools-extra/clangd/CodeComplete.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1726,7 +1726,7 @@ class CodeCompleteFlow {
17261726
}
17271727
if (Candidate.IdentifierResult) {
17281728
Quality.References = Candidate.IdentifierResult->References;
1729-
Relevance.ScopeKind = SymbolScope::FileScope;
1729+
Relevance.Scope = SymbolRelevanceSignals::FileScope;
17301730
Origin |= SymbolOrigin::Identifier;
17311731
}
17321732
}

Diff for: ‎clang-tools-extra/clangd/Quality.cpp

+40-15
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,37 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
262262
return OS;
263263
}
264264

265+
static SymbolRelevanceSignals::AccessibleScope
266+
computeScope(const NamedDecl *D) {
267+
// Injected "Foo" within the class "Foo" has file scope, not class scope.
268+
const DeclContext *DC = D->getDeclContext();
269+
if (auto *R = dyn_cast_or_null<RecordDecl>(D))
270+
if (R->isInjectedClassName())
271+
DC = DC->getParent();
272+
// Class constructor should have the same scope as the class.
273+
if (isa<CXXConstructorDecl>(D))
274+
DC = DC->getParent();
275+
bool InClass = false;
276+
for (; !DC->isFileContext(); DC = DC->getParent()) {
277+
if (DC->isFunctionOrMethod())
278+
return SymbolRelevanceSignals::FunctionScope;
279+
InClass = InClass || DC->isRecord();
280+
}
281+
if (InClass)
282+
return SymbolRelevanceSignals::ClassScope;
283+
// ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
284+
// Avoid caching linkage if it may change after enclosing code completion.
285+
if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage)
286+
return SymbolRelevanceSignals::FileScope;
287+
return SymbolRelevanceSignals::GlobalScope;
288+
}
289+
265290
void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
266291
SymbolURI = IndexResult.CanonicalDeclaration.FileURI;
267-
Scope = IndexResult.Scope;
292+
SymbolScope = IndexResult.Scope;
268293
IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);
269294
if (!(IndexResult.Flags & Symbol::VisibleOutsideFile)) {
270-
ScopeKind = SymbolScope::FileScope;
295+
Scope = AccessibleScope::FileScope;
271296
}
272297
if (MainFileSignals) {
273298
MainFileRefs =
@@ -325,7 +350,7 @@ void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
325350
computeASTSignals(SemaCCResult);
326351
// Declarations are scoped, others (like macros) are assumed global.
327352
if (SemaCCResult.Declaration)
328-
ScopeKind = std::min(ScopeKind, symbolScope(*SemaCCResult.Declaration));
353+
Scope = std::min(Scope, computeScope(SemaCCResult.Declaration));
329354

330355
NeedsFixIts = !SemaCCResult.FixIts.empty();
331356
}
@@ -368,7 +393,7 @@ SymbolRelevanceSignals::calculateDerivedSignals() const {
368393
if (ScopeProximityMatch) {
369394
// For global symbol, the distance is 0.
370395
Derived.ScopeProximityDistance =
371-
Scope ? ScopeProximityMatch->distance(*Scope) : 0;
396+
SymbolScope ? ScopeProximityMatch->distance(*SymbolScope) : 0;
372397
}
373398
return Derived;
374399
}
@@ -404,26 +429,26 @@ float SymbolRelevanceSignals::evaluateHeuristics() const {
404429
if (Query == CodeComplete) {
405430
// The narrower the scope where a symbol is visible, the more likely it is
406431
// to be relevant when it is available.
407-
switch (ScopeKind) {
408-
case SymbolScope::GlobalScope:
432+
switch (Scope) {
433+
case GlobalScope:
409434
break;
410-
case SymbolScope::FileScope:
435+
case FileScope:
411436
Score *= 1.5f;
412437
break;
413-
case SymbolScope::ClassScope:
438+
case ClassScope:
414439
Score *= 2;
415440
break;
416-
case SymbolScope::FunctionScope:
441+
case FunctionScope:
417442
Score *= 4;
418443
break;
419444
}
420445
} else {
421446
// For non-completion queries, the wider the scope where a symbol is
422447
// visible, the more likely it is to be relevant.
423-
switch (ScopeKind) {
424-
case SymbolScope::GlobalScope:
448+
switch (Scope) {
449+
case GlobalScope:
425450
break;
426-
case SymbolScope::FileScope:
451+
case FileScope:
427452
Score *= 0.5f;
428453
break;
429454
default:
@@ -482,11 +507,11 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
482507
OS << llvm::formatv("\tInBaseClass: {0}\n", S.InBaseClass);
483508
OS << llvm::formatv("\tContext: {0}\n", getCompletionKindString(S.Context));
484509
OS << llvm::formatv("\tQuery type: {0}\n", static_cast<int>(S.Query));
485-
OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.ScopeKind));
510+
OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.Scope));
486511

487512
OS << llvm::formatv("\tSymbol URI: {0}\n", S.SymbolURI);
488513
OS << llvm::formatv("\tSymbol scope: {0}\n",
489-
S.Scope ? *S.Scope : "<None>");
514+
S.SymbolScope ? *S.SymbolScope : "<None>");
490515

491516
SymbolRelevanceSignals::DerivedSignals Derived = S.calculateDerivedSignals();
492517
if (S.FileProximityMatch) {
@@ -543,7 +568,7 @@ evaluateDecisionForest(const SymbolQualitySignals &Quality,
543568
E.setSemaFileProximityScore(Relevance.SemaFileProximityScore);
544569
E.setSymbolScopeDistanceCost(Derived.ScopeProximityDistance);
545570
E.setSemaSaysInScope(Relevance.SemaSaysInScope);
546-
E.setScope(static_cast<uint32_t>(Relevance.ScopeKind));
571+
E.setScope(Relevance.Scope);
547572
E.setContextKind(Relevance.Context);
548573
E.setIsInstanceMember(Relevance.IsInstanceMember);
549574
E.setHadContextType(Relevance.HadContextType);

Diff for: ‎clang-tools-extra/clangd/Quality.h

+8-2
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,17 @@ struct SymbolRelevanceSignals {
108108

109109
// Scope proximity is only considered (both index and sema) when this is set.
110110
ScopeDistance *ScopeProximityMatch = nullptr;
111-
llvm::Optional<llvm::StringRef> Scope;
111+
llvm::Optional<llvm::StringRef> SymbolScope;
112112
// A symbol from sema should be accessible from the current scope.
113113
bool SemaSaysInScope = false;
114114

115-
SymbolScope ScopeKind = SymbolScope::GlobalScope;
115+
// An approximate measure of where we expect the symbol to be used.
116+
enum AccessibleScope {
117+
FunctionScope,
118+
ClassScope,
119+
FileScope,
120+
GlobalScope,
121+
} Scope = GlobalScope;
116122

117123
enum QueryType {
118124
CodeComplete,

Diff for: ‎clang-tools-extra/clangd/quality/CompletionModelCodegen.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ def gen_cpp_code(forest_json, features_json, filename, cpp_class):
245245
246246
%s
247247
248-
#define BIT(X) (1u << unsigned(X))
248+
#define BIT(X) (1 << X)
249249
250250
%s
251251

Diff for: ‎clang-tools-extra/clangd/quality/model/features.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@
7878
{
7979
"name": "Scope",
8080
"kind": "ENUM",
81-
"type": "clang::clangd::SymbolScope",
82-
"header": "AST.h"
81+
"type": "clang::clangd::SymbolRelevanceSignals::AccessibleScope",
82+
"header": "Quality.h"
8383
}
8484
]

Diff for: ‎clang-tools-extra/clangd/unittests/ASTTests.cpp

-71
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
#include "Annotations.h"
1212
#include "ParsedAST.h"
13-
#include "SourceCode.h"
1413
#include "TestTU.h"
1514
#include "clang/AST/Decl.h"
1615
#include "clang/AST/DeclBase.h"
@@ -352,76 +351,6 @@ TEST(ClangdAST, PrintType) {
352351
}
353352
}
354353
}
355-
356-
TEST(ASTTest, SymbolScope) {
357-
const struct {
358-
const char*Code;
359-
SymbolScope Expected;
360-
} Cases[] = {
361-
{
362-
"int ^x;",
363-
SymbolScope::GlobalScope,
364-
},
365-
{
366-
"static int ^x;",
367-
SymbolScope::FileScope,
368-
},
369-
{
370-
"namespace foo { int ^x; }",
371-
SymbolScope::GlobalScope,
372-
},
373-
{
374-
"namespace { int ^x; }",
375-
SymbolScope::FileScope,
376-
},
377-
{
378-
"class X{ void ^Y(); };",
379-
SymbolScope::ClassScope,
380-
},
381-
{
382-
"class X{ ^X(); };",
383-
SymbolScope::GlobalScope,
384-
},
385-
{
386-
"class X{ ^~X(); };",
387-
SymbolScope::ClassScope,
388-
},
389-
{
390-
"void x() { int ^y; }",
391-
SymbolScope::FunctionScope,
392-
},
393-
{
394-
"void x(int ^y) {}",
395-
SymbolScope::FunctionScope,
396-
},
397-
{
398-
"void x() { class ^Y{}; }",
399-
SymbolScope::FunctionScope,
400-
},
401-
{
402-
"void x() { class Y{ ^Y(); }; }",
403-
SymbolScope::FunctionScope,
404-
},
405-
{
406-
"void x() {auto y = [^z(0)]{};}",
407-
SymbolScope::FunctionScope,
408-
},
409-
};
410-
411-
for (const auto& Case : Cases) {
412-
SCOPED_TRACE(Case.Code);
413-
Annotations Test(Case.Code);
414-
auto AST = TestTU::withCode(Test.code()).build();
415-
416-
SourceLocation Loc = cantFail(
417-
sourceLocationInMainFile(AST.getSourceManager(), Test.point()));
418-
const NamedDecl &D = findDecl(
419-
AST, [&](const NamedDecl &D) { return D.getLocation() == Loc; });
420-
421-
EXPECT_EQ(symbolScope(D), Case.Expected);
422-
}
423-
}
424-
425354
} // namespace
426355
} // namespace clangd
427356
} // namespace clang

Diff for: ‎clang-tools-extra/clangd/unittests/QualityTests.cpp

+16-16
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
122122
/*Accessible=*/false));
123123
EXPECT_EQ(Relevance.NameMatch, SymbolRelevanceSignals().NameMatch);
124124
EXPECT_TRUE(Relevance.Forbidden);
125-
EXPECT_EQ(Relevance.ScopeKind, SymbolScope::GlobalScope);
125+
EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
126126

127127
Relevance = {};
128128
Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
@@ -160,17 +160,17 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
160160

161161
Relevance = {};
162162
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "X"), 42));
163-
EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope);
163+
EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
164164
Relevance = {};
165165
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "y"), 42));
166-
EXPECT_EQ(Relevance.ScopeKind, SymbolScope::ClassScope);
166+
EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::ClassScope);
167167
Relevance = {};
168168
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "z"), 42));
169-
EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FunctionScope);
169+
EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FunctionScope);
170170
// The injected class name is treated as the outer class name.
171171
Relevance = {};
172172
Relevance.merge(CodeCompletionResult(&findDecl(AST, "S::S"), 42));
173-
EXPECT_EQ(Relevance.ScopeKind, SymbolScope::GlobalScope);
173+
EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
174174

175175
Relevance = {};
176176
EXPECT_FALSE(Relevance.InBaseClass);
@@ -188,7 +188,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
188188
Matched = true;
189189
Relevance = {};
190190
Relevance.merge(S);
191-
EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope);
191+
EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
192192
});
193193
EXPECT_TRUE(Matched);
194194
}
@@ -263,7 +263,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) {
263263

264264
SymbolRelevanceSignals WithIndexScopeProximity;
265265
WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
266-
WithIndexScopeProximity.Scope = "x::";
266+
WithIndexScopeProximity.SymbolScope = "x::";
267267
EXPECT_GT(WithSemaScopeProximity.evaluateHeuristics(),
268268
Default.evaluateHeuristics());
269269

@@ -282,7 +282,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) {
282282
EXPECT_GT(IndexDistant.evaluateHeuristics(), Default.evaluateHeuristics());
283283

284284
SymbolRelevanceSignals Scoped;
285-
Scoped.ScopeKind = SymbolScope::FileScope;
285+
Scoped.Scope = SymbolRelevanceSignals::FileScope;
286286
EXPECT_LT(Scoped.evaluateHeuristics(), Default.evaluateHeuristics());
287287
Scoped.Query = SymbolRelevanceSignals::CodeComplete;
288288
EXPECT_GT(Scoped.evaluateHeuristics(), Default.evaluateHeuristics());
@@ -317,26 +317,26 @@ TEST(QualityTests, ScopeProximity) {
317317
ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
318318
Relevance.ScopeProximityMatch = &ScopeProximity;
319319

320-
Relevance.Scope = "other::";
320+
Relevance.SymbolScope = "other::";
321321
float NotMatched = Relevance.evaluateHeuristics();
322322

323-
Relevance.Scope = "";
323+
Relevance.SymbolScope = "";
324324
float Global = Relevance.evaluateHeuristics();
325325
EXPECT_GT(Global, NotMatched);
326326

327-
Relevance.Scope = "llvm::";
327+
Relevance.SymbolScope = "llvm::";
328328
float NonParent = Relevance.evaluateHeuristics();
329329
EXPECT_GT(NonParent, Global);
330330

331-
Relevance.Scope = "x::";
331+
Relevance.SymbolScope = "x::";
332332
float GrandParent = Relevance.evaluateHeuristics();
333333
EXPECT_GT(GrandParent, Global);
334334

335-
Relevance.Scope = "x::y::";
335+
Relevance.SymbolScope = "x::y::";
336336
float Parent = Relevance.evaluateHeuristics();
337337
EXPECT_GT(Parent, GrandParent);
338338

339-
Relevance.Scope = "x::y::z::";
339+
Relevance.SymbolScope = "x::y::z::";
340340
float Enclosing = Relevance.evaluateHeuristics();
341341
EXPECT_GT(Enclosing, Parent);
342342
}
@@ -375,8 +375,8 @@ TEST(QualityTests, NoBoostForClassConstructor) {
375375
SymbolRelevanceSignals Ctor;
376376
Ctor.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
377377

378-
EXPECT_EQ(Cls.ScopeKind, SymbolScope::GlobalScope);
379-
EXPECT_EQ(Ctor.ScopeKind, SymbolScope::GlobalScope);
378+
EXPECT_EQ(Cls.Scope, SymbolRelevanceSignals::GlobalScope);
379+
EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope);
380380
}
381381

382382
TEST(QualityTests, IsInstanceMember) {

0 commit comments

Comments
 (0)