Completion of function calls sometimes fails to include snippet

[HighCommander4]

In the following code:

void waldo(int arg);

int main()
{
  int val;
  wal^

    // if (true)
  
}

Place the cursor at the location indicated by ^, invoke completion, and select the only result.

Expected result: waldo(int arg) is inserted.

Actual result: waldo is inserted.

The behaviour has something to do with the commented line.

[HighCommander4]

This branch is being incorrectly taken, because NextTokenKind is incorrectly calculated to be tok::l_paren.

[HighCommander4]

NextTokenKind is computed here.

Going into Lexer::findNextToken(), the input Loc is the code completion location, which in the source buffer looks like a null character was inserted in between the source character before the cursor and the source character after the cursor when completion was invoked.

Things go downhill on this line, though:

  Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts);

After this, Loc now points to the space character before the ( character in the comment, which explains why the next token from that point onwards is identified as an l_paren.

[HighCommander4]

Having a bit of a "how did this ever work" moment here.

Inside getLocForEndOfToken(), we call MeasureTokenLength(), which lexes the next non-whitespace token after the code completion location, which is the tok::comment token // if (true), and returns its length, which is 12...

... but then adds that 12 to the code completion location (which differs from the start of the comment token by the intervening whitespace that was skipped), resulting in a location in the middle of the comment token 🤯

[HighCommander4]

I guess the way the current code manages to successfully identify cases where the next token after the completion location really is a ( (which is what it's trying to do), is that in those cases, MeasureTokenLength() measures the length of that ( token, which is 1, and then adds that 1 to the code completion location (which points to the null character), resulting in the offset where we actually do want to start lexing for the next token.

It basically works by accident, but the mechanism is pretty badly broken.

[HighCommander4]

One more thing I noticed while stepping through the lexer code that may be relevant: the lexer's inner loop does have a check for the code completion location, but checking for this requires the lexer to have a Preprocessor, and the temporary lexer created inside Lexer::getRawToken() (called from MeasureTokenLength()) does not have one.

[HighCommander4]

My best idea for how to fix this is to have clangd not use Lexer::findNextToken, but instead use code similar to the implementation of findNextToken, but which works for the completion point's SourceLocation.

It would be nicer to get findNextToken itself to handle the completion point, but I don't see how it can, given that the completion point is stored on the Preprocessor, and findNextToken's arguments do not include anything from which you can recover the Preprocessor; we'd have to modify the interfaces of findNextToken, getLocForEndOfToken, and MeasureTokenLength to take a Preprocessor as an optional parameter, or something like that.

[HighCommander4]

Proposed fix: llvm/llvm-project#69153

[zyn0217]

Thanks for the fix. Looking around the changed lines, I remembered that once I encountered the case where the "don't add new parens & placeholders if arg list does exist." heuristic doesn't work when completing a setter function call generated by Protobuf. I'd like to have a try on this patch and see if this solves that problem. (I didn't get around to reducing the code snippet since I first ran into it. Looks like it is somehow particular to protobuf because I've never seen this issue for my hand-written functions.)