Page MenuHomePhabricator

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

Authored by tom-anders on Oct 30 2022, 11:01 AM.

Details

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)

Diff Detail

Event Timeline

tom-anders created this revision.Oct 30 2022, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2022, 11:01 AM
tom-anders requested review of this revision.Oct 30 2022, 11:01 AM
nridge added a comment.EditedNov 4 2022, 1:27 AM

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?

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 updated this revision to Diff 473408.Nov 5 2022, 2:25 AM

Move logic to SemaCodeComplete.cpp

Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2022, 2:25 AM

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.

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 updated this revision to Diff 474358.Nov 9 2022, 2:00 PM

Add test to sema

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

Thanks!

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).

Clean up test

tom-anders marked 2 inline comments as done.Nov 13 2022, 4:37 AM
nridge accepted this revision.Nov 13 2022, 11:43 PM

Thanks!

This revision is now accepted and ready to land.Nov 13 2022, 11:43 PM

Thanks!

Thanks for the review!

This revision was landed with ongoing or failed builds.Nov 16 2022, 2:38 PM
This revision was automatically updated to reflect the committed changes.