sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov.
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!)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47871
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
test/clangd/completion.test
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/QualityTests.cpp
Index: unittests/clangd/QualityTests.cpp
===================================================================
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -39,23 +39,20 @@
SymbolQualitySignals Quality;
Quality.merge(findSymbol(Symbols, "x"));
EXPECT_FALSE(Quality.Deprecated);
- EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
Symbol F = findSymbol(Symbols, "f");
F.References = 24; // TestTU doesn't count references, so fake it.
Quality = {};
Quality.merge(F);
EXPECT_FALSE(Quality.Deprecated); // FIXME: Include deprecated bit in index.
- EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
EXPECT_EQ(Quality.References, 24u);
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
Quality = {};
Quality.merge(CodeCompletionResult(&findDecl(AST, "f"), /*Priority=*/42));
EXPECT_TRUE(Quality.Deprecated);
- EXPECT_EQ(Quality.SemaCCPriority, 42u);
EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
}
@@ -121,12 +118,6 @@
EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
- SymbolQualitySignals LowPriority, HighPriority;
- LowPriority.SemaCCPriority = 60;
- HighPriority.SemaCCPriority = 20;
- EXPECT_GT(HighPriority.evaluate(), Default.evaluate());
- EXPECT_LT(LowPriority.evaluate(), Default.evaluate());
-
SymbolQualitySignals Variable, Macro;
Variable.Category = SymbolQualitySignals::Variable;
Macro.Category = SymbolQualitySignals::Macro;
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -207,9 +207,6 @@
EXPECT_THAT(completions(Body + "int main() { S().FR^ }").items,
AllOf(Has("FooBar"), Not(Has("FooBaz")), Not(Has("Qux"))));
- EXPECT_THAT(completions(Body + "int main() { S().opr^ }").items,
- Has("operator="));
-
EXPECT_THAT(completions(Body + "int main() { aaa^ }").items,
AllOf(Has("Abracadabra"), Has("Alakazam")));
Index: test/clangd/protocol.test
===================================================================
--- test/clangd/protocol.test
+++ test/clangd/protocol.test
@@ -34,11 +34,11 @@
# CHECK-NEXT: "result": {
# CHECK-NEXT: "isIncomplete": false,
# CHECK-NEXT: "items": [
-# CHECK: "filterText": "fake",
-# CHECK-NEXT: "insertText": "fake",
+# CHECK: "filterText": "a",
+# CHECK-NEXT: "insertText": "a",
# CHECK-NEXT: "insertTextFormat": 1,
-# CHECK-NEXT: "kind": 7,
-# CHECK-NEXT: "label": "fake",
+# CHECK-NEXT: "kind": 5,
+# CHECK-NEXT: "label": "a",
# CHECK-NEXT: "sortText": "{{.*}}"
# CHECK: ]
# CHECK-NEXT: }
@@ -63,11 +63,11 @@
# CHECK-NEXT: "result": {
# CHECK-NEXT: "isIncomplete": false,
# CHECK-NEXT: "items": [
-# CHECK: "filterText": "fake",
-# CHECK-NEXT: "insertText": "fake",
+# CHECK: "filterText": "a",
+# CHECK-NEXT: "insertText": "a",
# CHECK-NEXT: "insertTextFormat": 1,
-# CHECK-NEXT: "kind": 7,
-# CHECK-NEXT: "label": "fake",
+# CHECK-NEXT: "kind": 5,
+# CHECK-NEXT: "label": "a",
# CHECK-NEXT: "sortText": "{{.*}}"
# CHECK: ]
# CHECK-NEXT: }
@@ -92,11 +92,11 @@
# CHECK-NEXT: "result": {
# CHECK-NEXT: "isIncomplete": false,
# CHECK-NEXT: "items": [
-# CHECK: "filterText": "fake",
-# CHECK-NEXT: "insertText": "fake",
+# CHECK: "filterText": "a",
+# CHECK-NEXT: "insertText": "a",
# CHECK-NEXT: "insertTextFormat": 1,
-# CHECK-NEXT: "kind": 7,
-# CHECK-NEXT: "label": "fake",
+# CHECK-NEXT: "kind": 5,
+# CHECK-NEXT: "label": "a",
# CHECK-NEXT: "sortText": "{{.*}}"
# CHECK: ]
# CHECK-NEXT: }
Index: test/clangd/completion.test
===================================================================
--- test/clangd/completion.test
+++ test/clangd/completion.test
@@ -18,8 +18,8 @@
# CHECK-NEXT: "kind": 5,
# CHECK-NEXT: "label": "a",
# CHECK-NEXT: "sortText": "{{.*}}a"
-# CHECK-NEXT: },
-# CHECK: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
---
# Update the source file and check for completions again.
{"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 @@
# CHECK-NEXT: "kind": 5,
# CHECK-NEXT: "label": "b",
# CHECK-NEXT: "sortText": "{{.*}}b"
-# CHECK-NEXT: },
-# CHECK: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
---
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
Index: clangd/Quality.h
===================================================================
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -44,9 +44,6 @@
/// Attributes of a symbol that affect how much we like it.
struct SymbolQualitySignals {
- unsigned SemaCCPriority = 0; // 1-80, 1 is best. 0 means absent.
- // FIXME: this is actually a mix of symbol
- // quality and relevance. Untangle this.
bool Deprecated = false;
unsigned References = 0;
Index: clangd/Quality.cpp
===================================================================
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -94,7 +94,6 @@
}
void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
- SemaCCPriority = SemaCCResult.Priority;
if (SemaCCResult.Availability == CXAvailability_Deprecated)
Deprecated = true;
@@ -117,11 +116,6 @@
if (References >= 3)
Score *= std::log(References);
- if (SemaCCPriority)
- // Map onto a 0-2 interval, so we don't reward/penalize non-Sema results.
- // Priority 80 is a really bad score.
- Score *= 2 - std::min<float>(80, SemaCCPriority) / 40;
-
if (Deprecated)
Score *= 0.1f;
@@ -146,8 +140,6 @@
raw_ostream &operator<<(raw_ostream &OS, const SymbolQualitySignals &S) {
OS << formatv("=== Symbol quality: {0}\n", S.evaluate());
- if (S.SemaCCPriority)
- OS << formatv("\tSemaCCPriority: {0}\n", S.SemaCCPriority);
OS << formatv("\tReferences: {0}\n", S.References);
OS << formatv("\tDeprecated: {0}\n", S.Deprecated);
OS << formatv("\tCategory: {0}\n", static_cast<int>(S.Category));
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -467,6 +467,25 @@
llvm_unreachable("unknown code completion context");
}
+// Some member calls are blacklisted because they're so rarely useful.
+static bool isBlacklistedMember(const NamedDecl &D) {
+ // Destructor completion is rarely useful, and works inconsistently.
+ // (s.^ completes ~string, but s.~st^ is an error).
+ if (D.getKind() == Decl::CXXDestructor)
+ return true;
+ // Injected name may be useful for A::foo(), but who writes A::A::foo()?
+ if (auto *R = dyn_cast_or_null<RecordDecl>(&D))
+ if (R->isInjectedClassName())
+ return true;
+ // Explicit calls to operators are also rare.
+ auto NameKind = D.getDeclName().getNameKind();
+ if (NameKind == DeclarationName::CXXOperatorName ||
+ NameKind == DeclarationName::CXXLiteralOperatorName ||
+ NameKind == DeclarationName::CXXConversionFunctionName)
+ return true;
+ return false;
+}
+
// The CompletionRecorder captures Sema code-complete output, including context.
// It filters out ignored results (but doesn't apply fuzzy-filtering yet).
// It doesn't do scoring or conversion to CompletionItem yet, as we want to
@@ -520,9 +539,8 @@
(Result.Availability == CXAvailability_NotAvailable ||
Result.Availability == CXAvailability_NotAccessible))
continue;
- // Destructor completion is rarely useful, and works inconsistently.
- // (s.^ completes ~string, but s.~st^ is an error).
- if (dyn_cast_or_null<CXXDestructorDecl>(Result.Declaration))
+ if (Result.Declaration && !Context.getBaseType().isNull() &&
+ isBlacklistedMember(*Result.Declaration))
continue;
// We choose to never append '::' to completion results in clangd.
Result.StartsNestedNameSpecifier = false;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
