[clangd] Add heuristic for dropping snippet when completing member function pointer

[tom-anders]

Summary

This implements the 1st heuristic mentioned in https://github.com/clangd/clangd/issues/968#issuecomment-1002242704:

When completing a function that names a non-static member of a class, and we are not inside that class's scope, assume the reference will not be a call (and thus don't add the snippetSuffix)

[nridge]

Thanks for the patch!

The test looks good to me.

As for the fix, I wonder if it would make sense to implement it in the Sema layer rather than in clangd. For example, we could give clang::CodeCompletionResult a new flag called something like CanBeCall, which, for a completion result that refers to a function, is set to true (by the code that creates the CodeCompletionResult, in SemaCodeComplete.cpp) if the use of the function may be a call, and false if it cannot.

Then clangd can check this flag, and clear the snippet suffix (or even better, not build it in the first place) if the value is false.

I think this approach could have a couple of advantages:

1. By placing the logic into libSema, other consumers of the Sema completion layer, besides clangd, could benefit from it as well. (A couple of other consumers that I'm aware of are libclang and cling.)
2. Because the logic now resides inside Sema itself, if we make enhancements to the heuristics for computing CanBeCall that require additional state (besides CurContext), we can just use them in place rather than having to propagate them into places like clangd's CodeCompletionBuilder.

What do you think?

[tom-anders]

In D137040#3907497, @nridge wrote:

Thanks for the patch!

The test looks good to me.

As for the fix, I wonder if it would make sense to implement it in the Sema layer rather than in clangd. For example, we could give clang::CodeCompletionResult a new flag called something like CanBeCall, which, for a completion result that refers to a function, is set to true (by the code that creates the CodeCompletionResult, in SemaCodeComplete.cpp) if the use of the function may be a call, and false if it cannot.

Then clangd can check this flag, and clear the snippet suffix (or even better, not build it in the first place) if the value is false.

I think this approach could have a couple of advantages:

1. By placing the logic into libSema, other consumers of the Sema completion layer, besides clangd, could benefit from it as well. (A couple of other consumers that I'm aware of are libclang and cling.)
2. Because the logic now resides inside Sema itself, if we make enhancements to the heuristics for computing CanBeCall that require additional state (besides CurContext), we can just use them in place rather than having to propagate them into places like clangd's CodeCompletionBuilder.

What do you think?

That indeed sounds like a better solution. I haven't looked much into SemaCodeComplete.cpp up to now, but I'll take a look and try to implement the heuristic over there.

[tom-anders]

Move logic to SemaCodeComplete.cpp

[tom-anders]

The test in clangd/unittests/CodeCompleteTests.cpp still passes. We now probably also need a unit test for Sema that tests the new flag, right?

clang-tools-extra/clangd/CodeComplete.cpp

414

Here, the SnippetSuffix is still created and then immediately cleared again. But the logic inside getSignature is quite complex and it's used in multiple places, so I didn't really want to touch that function for now.

[nridge]

In D137040#3909906, @tom-anders wrote:

We now probably also need a unit test for Sema that tests the new flag, right?

Good point. My understanding is that most test coverage of libSema's code completion takes the form of lit tests which are suitable for checking the completion proposals themselves but not flags like this. However, we do have one unittest file which could be adapted for testing the flag. It may require some light refactoring to make this work (e.g. I don't think we can just stuff the CodeCompletionResults themselves into the returned CompletionContext because they point to Decls and such whose lifetime is tied to the AST which is destroyed by the time runCompletion() returns).

clang-tools-extra/clangd/CodeComplete.cpp

414

Fair enough, we can optimize this later if it turns out to be necessary

[tom-anders]

Add test to sema

[tom-anders]

Hmm I added the test for the flag to Sema, but now we kinda have the same test case duplicated in sema and clangd tests - I guess for clangd we now actually only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is true, but I don't see an easy way to somehow inject fake Sema results into CodeComplete.cpp

[tom-anders]

Fix test

[nridge]

Thanks!

In D137040#3918118, @tom-anders wrote:

now we kinda have the same test case duplicated in sema and clangd tests - I guess for clangd we now actually only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is true, but I don't see an easy way to somehow inject fake Sema results into CodeComplete.cpp

I don't mind the duplication too much; I like that the clangd test is an "end to end" test directly expressing the user-visible behaviour that "in these situations we should get these completions".

clang/unittests/Sema/CodeCompleteTest.cpp

125

Since the constructor has only one other caller (runCompletion()), I'd rather remove the default arg and put the new VisitedContextFinder(...) in that caller.

As a bonus, we would no longer need to pass the CompletionContext to the CodeCompleteAction constructor, and don't need to use a "dummy context" in CollectCompletedFunctions().

229

Consider using this style of matcher (example use).

[tom-anders]

Clean up test

[nridge]

Thanks!

[tom-anders]

In D137040#3924115, @nridge wrote:

Thanks!

Thanks for the review!