[clang-tools-extra] r334192 - [clangd] Code completion: drop explicit injected names/operators, ignore Sema priority
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 7 05:49:18 PDT 2018
Author: sammccall
Date: Thu Jun 7 05:49:17 2018
New Revision: 334192
URL: http://llvm.org/viewvc/llvm-project?rev=334192&view=rev
Log:
[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
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Quality.cpp
clang-tools-extra/trunk/clangd/Quality.h
clang-tools-extra/trunk/test/clangd/completion.test
clang-tools-extra/trunk/test/clangd/protocol.test
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Thu Jun 7 05:49:17 2018
@@ -467,6 +467,25 @@ bool contextAllowsIndex(enum CodeComplet
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,9 @@ struct CompletionRecorder : public CodeC
(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() // is this a member-access context?
+ && isBlacklistedMember(*Result.Declaration))
continue;
// We choose to never append '::' to completion results in clangd.
Result.StartsNestedNameSpecifier = false;
Modified: clang-tools-extra/trunk/clangd/Quality.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Quality.cpp?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Quality.cpp (original)
+++ clang-tools-extra/trunk/clangd/Quality.cpp Thu Jun 7 05:49:17 2018
@@ -94,7 +94,6 @@ categorize(const index::SymbolInfo &D) {
}
void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
- SemaCCPriority = SemaCCResult.Priority;
if (SemaCCResult.Availability == CXAvailability_Deprecated)
Deprecated = true;
@@ -117,11 +116,6 @@ float SymbolQualitySignals::evaluate() c
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 @@ float SymbolQualitySignals::evaluate() c
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));
Modified: clang-tools-extra/trunk/clangd/Quality.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Quality.h?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Quality.h (original)
+++ clang-tools-extra/trunk/clangd/Quality.h Thu Jun 7 05:49:17 2018
@@ -44,9 +44,6 @@ struct Symbol;
/// 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;
Modified: clang-tools-extra/trunk/test/clangd/completion.test
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/completion.test?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clangd/completion.test (original)
+++ clang-tools-extra/trunk/test/clangd/completion.test Thu Jun 7 05:49:17 2018
@@ -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"}
Modified: clang-tools-extra/trunk/test/clangd/protocol.test
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/protocol.test?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clangd/protocol.test (original)
+++ clang-tools-extra/trunk/test/clangd/protocol.test Thu Jun 7 05:49:17 2018
@@ -34,11 +34,11 @@ Content-Length: 146
# 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 @@ Content-Length: 146
# 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 @@ Content-Length: 146
# 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: }
Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Thu Jun 7 05:49:17 2018
@@ -207,9 +207,6 @@ TEST(CompletionTest, Filter) {
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")));
@@ -250,9 +247,10 @@ void TestAfterDotCompletion(clangd::Code
// Class members. The only items that must be present in after-dot
// completion.
- EXPECT_THAT(
- Results.items,
- AllOf(Has(Opts.EnableSnippets ? "method()" : "method"), Has("field")));
+ EXPECT_THAT(Results.items,
+ AllOf(Has(Opts.EnableSnippets ? "method()" : "method"),
+ Has("field"), Not(Has("ClassWithMembers")),
+ Not(Has("operator=")), Not(Has("~ClassWithMembers"))));
EXPECT_IFF(Opts.IncludeIneligibleResults, Results.items,
Has("private_field"));
// Global items.
@@ -379,6 +377,25 @@ TEST(CompletionTest, Qualifiers) {
EXPECT_THAT(Results.items, Not(Contains(Labeled("foo() const")))); // private
}
+TEST(CompletionTest, InjectedTypename) {
+ // These are suppressed when accessed as a member...
+ EXPECT_THAT(completions("struct X{}; void foo(){ X().^ }").items,
+ Not(Has("X")));
+ EXPECT_THAT(completions("struct X{ void foo(){ this->^ } };").items,
+ Not(Has("X")));
+ // ...but accessible in other, more useful cases.
+ EXPECT_THAT(completions("struct X{ void foo(){ ^ } };").items, Has("X"));
+ EXPECT_THAT(completions("struct Y{}; struct X:Y{ void foo(){ ^ } };").items,
+ Has("Y"));
+ EXPECT_THAT(
+ completions(
+ "template<class> struct Y{}; struct X:Y<int>{ void foo(){ ^ } };")
+ .items,
+ Has("Y"));
+ // This case is marginal (`using X::X` is useful), we allow it for now.
+ EXPECT_THAT(completions("struct X{}; void foo(){ X::^ }").items, Has("X"));
+}
+
TEST(CompletionTest, Snippets) {
clangd::CodeCompleteOptions Opts;
Opts.EnableSnippets = true;
Modified: clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp?rev=334192&r1=334191&r2=334192&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp Thu Jun 7 05:49:17 2018
@@ -39,7 +39,6 @@ TEST(QualityTests, SymbolQualitySignalEx
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);
@@ -48,14 +47,12 @@ TEST(QualityTests, SymbolQualitySignalEx
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 @@ TEST(QualityTests, SymbolQualitySignalsS
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;
More information about the cfe-commits
mailing list