Skip to content

Commit 4caa851

Browse files
sam-mccallsam-mccall
committedJun 7, 2018
[clangd] Code completion: drop explicit injected names/operators, ignore Sema priority
Summary: Now we have most of Sema's code completion signals incorporated in Quality, which will allow us to give consistent ranking to sema/index results. Therefore we can/should stop using Sema priority as an explicit signal. This fixes some issues like namespaces always having a terrible score. The most important missing signals are: - Really dumb/rarely useful completions like: SomeStruct().^SomeStruct SomeStruct().^operator= SomeStruct().~SomeStruct() We already filter out destructors, this patch adds injected names and operators to that list. - type matching the expression context. Ilya has a plan to add this in a way that's compatible with indexes (design doc should be shared real soon now!) Reviewers: ioeric Subscribers: ilya-biryukov, MaskRay, jkorous, cfe-commits Differential Revision: https://reviews.llvm.org/D47871 llvm-svn: 334192
·
llvmorg-21-initllvmorg-7.0.0-rc1
1 parent b557846 commit 4caa851

File tree

7 files changed

+61
-45
lines changed

7 files changed

+61
-45
lines changed
 

‎clang-tools-extra/clangd/CodeComplete.cpp

+22-3
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,25 @@ bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
467467
llvm_unreachable("unknown code completion context");
468468
}
469469

470+
// Some member calls are blacklisted because they're so rarely useful.
471+
static bool isBlacklistedMember(const NamedDecl &D) {
472+
// Destructor completion is rarely useful, and works inconsistently.
473+
// (s.^ completes ~string, but s.~st^ is an error).
474+
if (D.getKind() == Decl::CXXDestructor)
475+
return true;
476+
// Injected name may be useful for A::foo(), but who writes A::A::foo()?
477+
if (auto *R = dyn_cast_or_null<RecordDecl>(&D))
478+
if (R->isInjectedClassName())
479+
return true;
480+
// Explicit calls to operators are also rare.
481+
auto NameKind = D.getDeclName().getNameKind();
482+
if (NameKind == DeclarationName::CXXOperatorName ||
483+
NameKind == DeclarationName::CXXLiteralOperatorName ||
484+
NameKind == DeclarationName::CXXConversionFunctionName)
485+
return true;
486+
return false;
487+
}
488+
470489
// The CompletionRecorder captures Sema code-complete output, including context.
471490
// It filters out ignored results (but doesn't apply fuzzy-filtering yet).
472491
// It doesn't do scoring or conversion to CompletionItem yet, as we want to
@@ -520,9 +539,9 @@ struct CompletionRecorder : public CodeCompleteConsumer {
520539
(Result.Availability == CXAvailability_NotAvailable ||
521540
Result.Availability == CXAvailability_NotAccessible))
522541
continue;
523-
// Destructor completion is rarely useful, and works inconsistently.
524-
// (s.^ completes ~string, but s.~st^ is an error).
525-
if (dyn_cast_or_null<CXXDestructorDecl>(Result.Declaration))
542+
if (Result.Declaration &&
543+
!Context.getBaseType().isNull() // is this a member-access context?
544+
&& isBlacklistedMember(*Result.Declaration))
526545
continue;
527546
// We choose to never append '::' to completion results in clangd.
528547
Result.StartsNestedNameSpecifier = false;

‎clang-tools-extra/clangd/Quality.cpp

-8
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,6 @@ categorize(const index::SymbolInfo &D) {
9494
}
9595

9696
void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
97-
SemaCCPriority = SemaCCResult.Priority;
9897
if (SemaCCResult.Availability == CXAvailability_Deprecated)
9998
Deprecated = true;
10099

@@ -117,11 +116,6 @@ float SymbolQualitySignals::evaluate() const {
117116
if (References >= 3)
118117
Score *= std::log(References);
119118

120-
if (SemaCCPriority)
121-
// Map onto a 0-2 interval, so we don't reward/penalize non-Sema results.
122-
// Priority 80 is a really bad score.
123-
Score *= 2 - std::min<float>(80, SemaCCPriority) / 40;
124-
125119
if (Deprecated)
126120
Score *= 0.1f;
127121

@@ -146,8 +140,6 @@ float SymbolQualitySignals::evaluate() const {
146140

147141
raw_ostream &operator<<(raw_ostream &OS, const SymbolQualitySignals &S) {
148142
OS << formatv("=== Symbol quality: {0}\n", S.evaluate());
149-
if (S.SemaCCPriority)
150-
OS << formatv("\tSemaCCPriority: {0}\n", S.SemaCCPriority);
151143
OS << formatv("\tReferences: {0}\n", S.References);
152144
OS << formatv("\tDeprecated: {0}\n", S.Deprecated);
153145
OS << formatv("\tCategory: {0}\n", static_cast<int>(S.Category));

‎clang-tools-extra/clangd/Quality.h

-3
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,6 @@ struct Symbol;
4444

4545
/// Attributes of a symbol that affect how much we like it.
4646
struct SymbolQualitySignals {
47-
unsigned SemaCCPriority = 0; // 1-80, 1 is best. 0 means absent.
48-
// FIXME: this is actually a mix of symbol
49-
// quality and relevance. Untangle this.
5047
bool Deprecated = false;
5148
unsigned References = 0;
5249

‎clang-tools-extra/test/clangd/completion.test

+4-4
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@
1818
# CHECK-NEXT: "kind": 5,
1919
# CHECK-NEXT: "label": "a",
2020
# CHECK-NEXT: "sortText": "{{.*}}a"
21-
# CHECK-NEXT: },
22-
# CHECK: ]
21+
# CHECK-NEXT: }
22+
# CHECK-NEXT: ]
2323
---
2424
# Update the source file and check for completions again.
2525
{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///main.cpp","version":2},"contentChanges":[{"text":"struct S { int b; };\nint main() {\nS().\n}"}]}}
@@ -38,7 +38,7 @@
3838
# CHECK-NEXT: "kind": 5,
3939
# CHECK-NEXT: "label": "b",
4040
# CHECK-NEXT: "sortText": "{{.*}}b"
41-
# CHECK-NEXT: },
42-
# CHECK: ]
41+
# CHECK-NEXT: }
42+
# CHECK-NEXT: ]
4343
---
4444
{"jsonrpc":"2.0","id":4,"method":"shutdown"}

‎clang-tools-extra/test/clangd/protocol.test

+12-12
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,11 @@ Content-Length: 146
3434
# CHECK-NEXT: "result": {
3535
# CHECK-NEXT: "isIncomplete": false,
3636
# CHECK-NEXT: "items": [
37-
# CHECK: "filterText": "fake",
38-
# CHECK-NEXT: "insertText": "fake",
37+
# CHECK: "filterText": "a",
38+
# CHECK-NEXT: "insertText": "a",
3939
# CHECK-NEXT: "insertTextFormat": 1,
40-
# CHECK-NEXT: "kind": 7,
41-
# CHECK-NEXT: "label": "fake",
40+
# CHECK-NEXT: "kind": 5,
41+
# CHECK-NEXT: "label": "a",
4242
# CHECK-NEXT: "sortText": "{{.*}}"
4343
# CHECK: ]
4444
# CHECK-NEXT: }
@@ -63,11 +63,11 @@ Content-Length: 146
6363
# CHECK-NEXT: "result": {
6464
# CHECK-NEXT: "isIncomplete": false,
6565
# CHECK-NEXT: "items": [
66-
# CHECK: "filterText": "fake",
67-
# CHECK-NEXT: "insertText": "fake",
66+
# CHECK: "filterText": "a",
67+
# CHECK-NEXT: "insertText": "a",
6868
# CHECK-NEXT: "insertTextFormat": 1,
69-
# CHECK-NEXT: "kind": 7,
70-
# CHECK-NEXT: "label": "fake",
69+
# CHECK-NEXT: "kind": 5,
70+
# CHECK-NEXT: "label": "a",
7171
# CHECK-NEXT: "sortText": "{{.*}}"
7272
# CHECK: ]
7373
# CHECK-NEXT: }
@@ -92,11 +92,11 @@ Content-Length: 146
9292
# CHECK-NEXT: "result": {
9393
# CHECK-NEXT: "isIncomplete": false,
9494
# CHECK-NEXT: "items": [
95-
# CHECK: "filterText": "fake",
96-
# CHECK-NEXT: "insertText": "fake",
95+
# CHECK: "filterText": "a",
96+
# CHECK-NEXT: "insertText": "a",
9797
# CHECK-NEXT: "insertTextFormat": 1,
98-
# CHECK-NEXT: "kind": 7,
99-
# CHECK-NEXT: "label": "fake",
98+
# CHECK-NEXT: "kind": 5,
99+
# CHECK-NEXT: "label": "a",
100100
# CHECK-NEXT: "sortText": "{{.*}}"
101101
# CHECK: ]
102102
# CHECK-NEXT: }

‎clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp

+23-6
Original file line numberDiff line numberDiff line change
@@ -207,9 +207,6 @@ TEST(CompletionTest, Filter) {
207207
EXPECT_THAT(completions(Body + "int main() { S().FR^ }").items,
208208
AllOf(Has("FooBar"), Not(Has("FooBaz")), Not(Has("Qux"))));
209209

210-
EXPECT_THAT(completions(Body + "int main() { S().opr^ }").items,
211-
Has("operator="));
212-
213210
EXPECT_THAT(completions(Body + "int main() { aaa^ }").items,
214211
AllOf(Has("Abracadabra"), Has("Alakazam")));
215212

@@ -250,9 +247,10 @@ void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
250247

251248
// Class members. The only items that must be present in after-dot
252249
// completion.
253-
EXPECT_THAT(
254-
Results.items,
255-
AllOf(Has(Opts.EnableSnippets ? "method()" : "method"), Has("field")));
250+
EXPECT_THAT(Results.items,
251+
AllOf(Has(Opts.EnableSnippets ? "method()" : "method"),
252+
Has("field"), Not(Has("ClassWithMembers")),
253+
Not(Has("operator=")), Not(Has("~ClassWithMembers"))));
256254
EXPECT_IFF(Opts.IncludeIneligibleResults, Results.items,
257255
Has("private_field"));
258256
// Global items.
@@ -379,6 +377,25 @@ TEST(CompletionTest, Qualifiers) {
379377
EXPECT_THAT(Results.items, Not(Contains(Labeled("foo() const")))); // private
380378
}
381379

380+
TEST(CompletionTest, InjectedTypename) {
381+
// These are suppressed when accessed as a member...
382+
EXPECT_THAT(completions("struct X{}; void foo(){ X().^ }").items,
383+
Not(Has("X")));
384+
EXPECT_THAT(completions("struct X{ void foo(){ this->^ } };").items,
385+
Not(Has("X")));
386+
// ...but accessible in other, more useful cases.
387+
EXPECT_THAT(completions("struct X{ void foo(){ ^ } };").items, Has("X"));
388+
EXPECT_THAT(completions("struct Y{}; struct X:Y{ void foo(){ ^ } };").items,
389+
Has("Y"));
390+
EXPECT_THAT(
391+
completions(
392+
"template<class> struct Y{}; struct X:Y<int>{ void foo(){ ^ } };")
393+
.items,
394+
Has("Y"));
395+
// This case is marginal (`using X::X` is useful), we allow it for now.
396+
EXPECT_THAT(completions("struct X{}; void foo(){ X::^ }").items, Has("X"));
397+
}
398+
382399
TEST(CompletionTest, Snippets) {
383400
clangd::CodeCompleteOptions Opts;
384401
Opts.EnableSnippets = true;

‎clang-tools-extra/unittests/clangd/QualityTests.cpp

-9
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ TEST(QualityTests, SymbolQualitySignalExtraction) {
3939
SymbolQualitySignals Quality;
4040
Quality.merge(findSymbol(Symbols, "x"));
4141
EXPECT_FALSE(Quality.Deprecated);
42-
EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
4342
EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
4443
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
4544

@@ -48,14 +47,12 @@ TEST(QualityTests, SymbolQualitySignalExtraction) {
4847
Quality = {};
4948
Quality.merge(F);
5049
EXPECT_FALSE(Quality.Deprecated); // FIXME: Include deprecated bit in index.
51-
EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
5250
EXPECT_EQ(Quality.References, 24u);
5351
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
5452

5553
Quality = {};
5654
Quality.merge(CodeCompletionResult(&findDecl(AST, "f"), /*Priority=*/42));
5755
EXPECT_TRUE(Quality.Deprecated);
58-
EXPECT_EQ(Quality.SemaCCPriority, 42u);
5956
EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
6057
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
6158
}
@@ -121,12 +118,6 @@ TEST(QualityTests, SymbolQualitySignalsSanity) {
121118
EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
122119
EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
123120

124-
SymbolQualitySignals LowPriority, HighPriority;
125-
LowPriority.SemaCCPriority = 60;
126-
HighPriority.SemaCCPriority = 20;
127-
EXPECT_GT(HighPriority.evaluate(), Default.evaluate());
128-
EXPECT_LT(LowPriority.evaluate(), Default.evaluate());
129-
130121
SymbolQualitySignals Variable, Macro;
131122
Variable.Category = SymbolQualitySignals::Variable;
132123
Macro.Category = SymbolQualitySignals::Macro;

0 commit comments

Comments
 (0)
Please sign in to comment.