RFC: Constructor/destructor code completion

Hi,

As a follow-up to discussion about destructor return type in code completion [1] I investigated status quo of constructor and destructor code completion in clangd.

I was using this trivial lit-test case:

{“jsonrpc”:“2.0”,“id”:0,“method”:“initialize”,“params”:{“processId”:123,“rootPath”:“clangd”,“capabilities”:{},“trace”:“off”}}

If we ignore the implementation details for a moment and think of what the results should be in those cases, my thoughts would be:

For (2) neither of the results make sense, the completion results should be empty.We are completing on top-level, so we try to complete either (a) a type of the declaration or (b) constructor/destructor name.
There are no types inside ‘foo’, so (a) should not produce any results either.

There are no user-defined ctors and dtors, so (b) should not produce any results.

Note that (b) describes a form of completion that we do not have, i.e. the completion for out-of-line definitions of methods inside a class.
It is different from normal member completions after dot (e.g. foo().^):

  • Should only include methods that do not have a definition.
  • Should not have any argument placeholders in snippets.
  • Should include the return type.
  • Should probably complete the braces for method body {} too.

For (1), we sometimes (very rarely) want destructor, but it’s so rare that typing ‘~’ and repeating completion seems to be fine in those cases.
For (3), the struct and destructor names are almost indistinguishable, the only possible visible difference is the presence of function argument snippet (i.e. foo() vs foo).
This does not seem to be a major UX concern, either seems fine. Reiterating (2): without user-defined dtor we should not show dtor in those completion results.

Don’t have a strong opinion, but if we do choose to have destructors in code completion, not showing void as return type for destructors (and no return type for constructors constructors) seems totally reasonable.

Thanks for looking into this!

  1. We currently skip constructors but include destructors, assignment operators and struct/class name after double colon.

struct foo {}; foo::^
// produces 4 results - struct foo itself, copy assignment, move assignment, destructor

I think there’s a fair case to be made we shouldn’t include any of these. Explicitly referring to constructor, destructor, operators, injected class name are all rare enough that these may add more noise than value. I’m personally not opposed to removing some completions that are technically correct but rarely useful.

Possible exceptions:
struct bar : foo { using foo::^foo }
(probably looks the same to the parser)
This seems rare enough though.

Bar& Bar::^operator=(const Bar&) { }
(completion of out-of-line implementation)
I’m not sure how these completions currently work, but I’m sure we can make sure these only trigger where an out-of-line definition is actually plausible.

  • It seems to me that by including struct/class name we effectively include constructor code completion which makes sense in this situation but we shouldn’t put it’s kind as struct/class.

I’m not sure what you’re suggesting we complete here.
If it’s literally “foo::foo” then that’s the injected class name, struct or class seems correct.
If it’s a whole constructor “foo::foo(someargs)”, with one for each overload, that seems rarely correct/useful.

  • What seems odd to me in this context is that we use void as return type of destructor (and had we not skipped constructor, it’s return type might be unexpected too). We previously discussed destructor return type with Sam in [1] and I agreed that it’s fine under false impression that we aren’t displaying destructors anyway (off-by-one error in my test case).

I think we talked past each other then :slight_smile:
I’m not sure what you can do with a destructor other than declare it or call it, you can’t take its address. So presumably the expression we’re completing here is a destructor call: ~Foo(). The type of that is void: https://godbolt.org/z/L4US-Q

We can omit return type but I’m a bit uncomfortable with the approach of omitting it in the CodeCompletionResult for aesthetic reasons, because return type matching the context is a completion quality signal. Maybe in this particular case we get away with it, but…

I suggest that we don’t include struct/class name but include both constructor (without any return type == no “detail” in LSP) and destructor (without any return type == no “detail” in LSP) when code completion context is “struct/class name followed by double colon”.

Are there common patterns when these are actually useful completions?
My intuition is that these are rarely useful, and so attempting to handle them correctly may come out worse overall by adding complexity or false-positive results and getting little true-positives in return. Just a guess though.

  1. We currently skip both constructors and destructors but include struct/class name after double colon and tilde but also include lot of other results.

struct foo {}; foo::~^
// produces 66 results - including keywords and probably built-in macros and struct/class name but not constructor or destructor

This seems wrong to me. I suggest we skip unrelated keywords and struct/class name but we should include destructor (with proper insertText to not repeat the leading tilde).

I think this is just a parser bug that nobody’s bothered to fix. The parser is designed to parse valid C++, code completion is bolted on. It probably doesn’t recover from the syntax error that is foo::~f in a useful way. Please fix it if you can!

I totally missed that in my examples all those special methods were declared only implicitly. Thanks Ilya for pointing that out!

For the record - it seems like we are currently providing code completion for out-of-class method definitions.
struct foo { void foomethod(); } foo::^
// returns among others also foomethod

If you think that providing struct name, ctor, dtor and assignment completion for this case without degrading quality of results or adding noise somewhere else is too big hassle right now I don’t mind removing these.

I think the best way forward is to agree on some testcases which would specify our requirements against which I could write my patches. (Real TDD first time in my life, yay!)

Could you please take a look at these tests to check we are on the same page?
https://reviews.llvm.org/D52554

I will look into the issue with “foo::~^" separately. I guess Sam might be right about a parser bug as I remember seeing some funny-looking callstack in clang under lldb.

Cheers,

Jan

I agree with the thrust of your tests - that we should rarely be providing these completions.

(As noted on the patch, the tests themselves could probably be combined with the existing related tests, rather than adding lots of new ones).

Cheers, Sam