[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
dgoldman marked an inline comment as done.
Closed by commit rG2a030e680e08: [clangd][ObjC] Fix issue completing a method
decl by name (authored by dgoldman).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100798/new/
https://reviews.llvm.org/D100798
Files:
clang-tools-extra/clangd/CodeCompletionStrings.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2796,6 +2796,79 @@
EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
}
+TEST(CompletionTest, ObjectiveCSimpleMethodDeclaration) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (void)foo;
+ @end
+ @implementation Foo
+ fo^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("foo")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Qualifier("- (void)")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclaration) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c secondArgument:(id)object;
+ @end
+ @implementation Foo
+ valueFor^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Qualifier("- (int)")));
+ EXPECT_THAT(C, ElementsAre(Signature("(char)c secondArgument:(id)object")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationPrefixTyped) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c;
+ @end
+ @implementation Foo
+ - (int)valueFor^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Signature("(char)c")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationFromMiddle) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c secondArgument:(id)object;
+ @end
+ @implementation Foo
+ - (int)valueForCharacter:(char)c second^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("secondArgument:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Signature("(id)object")));
+}
+
TEST(CompletionTest, CursorInSnippets) {
clangd::CodeCompleteOptions Options;
Options.EnableSnippets = true;
Index: clang-tools-extra/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/clangd/CodeCompletionStrings.cpp
@@ -114,6 +114,7 @@
}
unsigned SnippetArg = 0;
bool HadObjCArguments = false;
+ bool HadInformativeChunks = false;
for (const auto : CCS) {
// Informative qualifier chunks only clutter completion results, skip
// them.
@@ -129,10 +130,14 @@
// reclassified as qualifiers.
//
// Objective-C:
- // Objective-C methods may have multiple typed-text chunks, so we must
- // treat them carefully. For Objective-C methods, all typed-text chunks
- // will end in ':' (unless there are no arguments, in which case we
- // can safely treat them as C++).
+ // Objective-C methods expressions may have multiple typed-text chunks,
+ // so we must treat them carefully. For Objective-C methods, all
+ // typed-text and informative chunks will end in ':' (unless there are
+ // no arguments, in which case we can safely treat them as C++).
+ //
+ // Completing a method declaration itself (not a method expression) is
+ // similar except that we use the `RequiredQualifiers` to store the
+ // text before the selector, e.g. `- (void)`.
if (!llvm::StringRef(Chunk.Text).endswith(":")) { // Treat as C++.
if (RequiredQualifiers)
*RequiredQualifiers = std::move(*Signature);
@@ -147,6 +152,28 @@
//
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
dgoldman marked an inline comment as done.
dgoldman added inline comments.
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:160
+ //
+ // e.g. to complete `- (void)doSomething:(id)argument`:
+ // - Completion name: `doSomething:`
sammccall wrote:
> Does completing the no-args declaration `- (void) foo` work as expected?
>
> We never get to this part of the code in this case because endswith(":") is
> never true. The comment above says "safe to treat as c++" but not sure this
> is true for declaration, just method calls. Maybe it works for some other
> reason though?
>
> In any case, it's OK if this case doesn't work (this patch still improves
> things a lot). We should probably have a test showing what the state is.
Yeah, it does. I added a test case for it, it does work, the current behavior
is fine, the issue for ObjC is specifically selectors with arguments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100798/new/
https://reviews.llvm.org/D100798
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
dgoldman updated this revision to Diff 349014.
dgoldman added a comment.
Add another test for a simple method decl (no arg selector)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100798/new/
https://reviews.llvm.org/D100798
Files:
clang-tools-extra/clangd/CodeCompletionStrings.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2783,6 +2783,79 @@
EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
}
+TEST(CompletionTest, ObjectiveCSimpleMethodDeclaration) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (void)foo;
+ @end
+ @implementation Foo
+ fo^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("foo")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Qualifier("- (void)")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclaration) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c secondArgument:(id)object;
+ @end
+ @implementation Foo
+ valueFor^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Qualifier("- (int)")));
+ EXPECT_THAT(C, ElementsAre(Signature("(char)c secondArgument:(id)object")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationPrefixTyped) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c;
+ @end
+ @implementation Foo
+ - (int)valueFor^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Signature("(char)c")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationFromMiddle) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c secondArgument:(id)object;
+ @end
+ @implementation Foo
+ - (int)valueForCharacter:(char)c second^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("secondArgument:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Signature("(id)object")));
+}
+
TEST(CompletionTest, CursorInSnippets) {
clangd::CodeCompleteOptions Options;
Options.EnableSnippets = true;
Index: clang-tools-extra/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/clangd/CodeCompletionStrings.cpp
@@ -114,6 +114,7 @@
}
unsigned SnippetArg = 0;
bool HadObjCArguments = false;
+ bool HadInformativeChunks = false;
for (const auto : CCS) {
// Informative qualifier chunks only clutter completion results, skip
// them.
@@ -129,10 +130,14 @@
// reclassified as qualifiers.
//
// Objective-C:
- // Objective-C methods may have multiple typed-text chunks, so we must
- // treat them carefully. For Objective-C methods, all typed-text chunks
- // will end in ':' (unless there are no arguments, in which case we
- // can safely treat them as C++).
+ // Objective-C methods expressions may have multiple typed-text chunks,
+ // so we must treat them carefully. For Objective-C methods, all
+ // typed-text and informative chunks will end in ':' (unless there are
+ // no arguments, in which case we can safely treat them as C++).
+ //
+ // Completing a method declaration itself (not a method expression) is
+ // similar except that we use the `RequiredQualifiers` to store the
+ // text before the selector, e.g. `- (void)`.
if (!llvm::StringRef(Chunk.Text).endswith(":")) { // Treat as C++.
if (RequiredQualifiers)
*RequiredQualifiers = std::move(*Signature);
@@ -147,6 +152,28 @@
// methods.
if (!HadObjCArguments) {
HadObjCArguments = true;
+ // If we have no previous informative chunks (informative selector
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks for the detailed comments, this makes a lot more sense to me now!
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:160
+ //
+ // e.g. to complete `- (void)doSomething:(id)argument`:
+ // - Completion name: `doSomething:`
Does completing the no-args declaration `- (void) foo` work as expected?
We never get to this part of the code in this case because endswith(":") is
never true. The comment above says "safe to treat as c++" but not sure this is
true for declaration, just method calls. Maybe it works for some other reason
though?
In any case, it's OK if this case doesn't work (this patch still improves
things a lot). We should probably have a test showing what the state is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100798/new/
https://reviews.llvm.org/D100798
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
dgoldman added a comment. Friendly ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100798/new/ https://reviews.llvm.org/D100798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
dgoldman added inline comments. Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:152 // // TODO: Make previous parameters part of the signature for Objective-C // methods. sammccall wrote: > is this TODO handled now? No, this does not change any behavior for parameters (we don't show the full ObjcMethodDecl signature including previous parameter types if you complete it from the middle) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100798/new/ https://reviews.llvm.org/D100798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
dgoldman updated this revision to Diff 340619.
dgoldman marked 4 inline comments as done.
dgoldman added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100798/new/
https://reviews.llvm.org/D100798
Files:
clang-tools-extra/clangd/CodeCompletionStrings.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2783,6 +2783,61 @@
EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
}
+TEST(CompletionTest, ObjectiveCMethodDeclaration) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c secondArgument:(id)object;
+ @end
+ @implementation Foo
+ valueFor^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Qualifier("- (int)")));
+ EXPECT_THAT(C, ElementsAre(Signature("(char)c secondArgument:(id)object")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationPrefixTyped) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c;
+ @end
+ @implementation Foo
+ - (int)valueFor^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Signature("(char)c")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationFromMiddle) {
+ auto Results = completions(R"objc(
+ @interface Foo
+ - (int)valueForCharacter:(char)c secondArgument:(id)object;
+ @end
+ @implementation Foo
+ - (int)valueForCharacter:(char)c second^
+ @end
+)objc",
+ /*IndexSymbols=*/{},
+ /*Opts=*/{}, "Foo.m");
+
+ auto C = Results.Completions;
+ EXPECT_THAT(C, ElementsAre(Named("secondArgument:")));
+ EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+ EXPECT_THAT(C, ElementsAre(Signature("(id)object")));
+}
+
TEST(CompletionTest, CursorInSnippets) {
clangd::CodeCompleteOptions Options;
Options.EnableSnippets = true;
Index: clang-tools-extra/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/clangd/CodeCompletionStrings.cpp
@@ -114,6 +114,7 @@
}
unsigned SnippetArg = 0;
bool HadObjCArguments = false;
+ bool HadInformativeChunks = false;
for (const auto : CCS) {
// Informative qualifier chunks only clutter completion results, skip
// them.
@@ -129,10 +130,14 @@
// reclassified as qualifiers.
//
// Objective-C:
- // Objective-C methods may have multiple typed-text chunks, so we must
- // treat them carefully. For Objective-C methods, all typed-text chunks
- // will end in ':' (unless there are no arguments, in which case we
- // can safely treat them as C++).
+ // Objective-C methods expressions may have multiple typed-text chunks,
+ // so we must treat them carefully. For Objective-C methods, all
+ // typed-text and informative chunks will end in ':' (unless there are
+ // no arguments, in which case we can safely treat them as C++).
+ //
+ // Completing a method declaration itself (not a method expression) is
+ // similar except that we use the `RequiredQualifiers` to store the
+ // text before the selector, e.g. `- (void)`.
if (!llvm::StringRef(Chunk.Text).endswith(":")) { // Treat as C++.
if (RequiredQualifiers)
*RequiredQualifiers = std::move(*Signature);
@@ -147,6 +152,28 @@
// methods.
if (!HadObjCArguments) {
HadObjCArguments = true;
+ // If we have no previous informative chunks (informative selector
+ // fragments in practice), we treat any previous chunks as
+ // `RequiredQualifiers` so they will be added as a prefix during the
+ // completion.
+ //
+ // e.g. to complete `- (void)doSomething:(id)argument`:
+ // - Completion name: `doSomething:`
+ // - RequiredQualifiers: `- (void)`
+ // - Snippet/Signature suffix: `(id)argument`
+ //
+ // This differs from the case when we're completing a method
+ //
[PATCH] D100798: [clangd][ObjC] Fix issue completing a method decl by name
sammccall added a comment.
Herald added a subscriber: cfe-commits.
The test results look good, but I have trouble following the code, I think more
comments might help :-)
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:140
+ // similar except that we use the `RequiredQualifiers` to store the
+ // prefix of our snippet (Objective-C doesn't have C++ style
+ // qualifiers).
can we say something more concrete than "the prefix of our snippet"? this is
the `-(void)` part right?
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:152
//
// TODO: Make previous parameters part of the signature for Objective-C
// methods.
is this TODO handled now?
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:156
HadObjCArguments = true;
+ if (!HadInformativeArguments) {
+if (RequiredQualifiers)
brief comment here should explain the significance of !HadInformativeArguments
(what it means if we know we're handling objc)
Maybe even worth an example here, not obvious to me what the data is that we're
shuffling between variables here.
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:187
case CodeCompletionString::CK_Informative:
+ HadInformativeArguments = true;
// For example, the word "const" for a const method, or the name of
informative chunks? Not sure how we'd know they're arguments here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100798/new/
https://reviews.llvm.org/D100798
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
