Handle `completionItem/resolve` #1384

gabritto · 2025-07-10T19:29:15Z

This PR implements LSP's completionItem/resolve method in the language server.
This roughly corresponds to Strada's completionEntryDetails.

completionItem/resolve method sends a completion item to the server, and expects the same completion item with lazily-computed properties filled in. Right now vscode supports lazy computation of documentation, detail, and additionalTextEdits.
This PR implements computing detail (Strada's text) and documentation (Strada's documentation). additionalTextEdits will be used once we have auto-imports.
Note that for now, if a client doesn't support lazy computation of detail and documentation, we simply won't fill in those properties.

The completion code is the same as it was in Strada, with one exception. To be able to compute the properties for a completion item resolve call, we effectively need to do much of the same work we do for the original completion request, e.g. we need to collect symbols available at the requested file and location. To reconstruct the original completion request, need to know the file and position of the original request, among other things. That information is now passed in the completion item data property in the original completion response, which is always preserved by the client. In Strada, this information was passed by the client in the completionEntryDetails request.

Another note is that many of the tests ported in this PR fail because quickinfo produces a different string. Hopefully that will be fixed later when we have a more complete implementation of quickinfo, but in any case I'll start fixing the failing tests after this PR.

jakebailey · 2025-07-10T19:31:22Z

internal/ls/completions.go

+}
+
+func GetCompletionItemData(item *lsproto.CompletionItem) (*itemData, error) {
+	bytes, err := json.Marshal(item.Data)


To avoid this, we should probably special case some of these incoming anys as json.RawMessage, maybe?

Yes, that would be nice. I can try and add this to the lsp generator in a separate PR.

The annoying bit is that it would probably break things where we are passing the objects as-is, I think

Right, because then the type of CompletionItem.Data would be bytes and we'd have to marshall that as we construct the object.

jakebailey · 2025-07-10T19:32:36Z

internal/ls/completions.go

+		return nil, errors.New("completion item data is nil")
+	}
+
+	program, file := l.tryGetProgramAndFile(data.FileName)


In the near future, I sort of wonder how resolve will work in the snapshot model where things can change. Should we include some sort of snapshot number that indicates where a completion came from and reject it if the snapshot has been dropped?

Or, I guess in this code, is it possible for this code to fail in an unhand able way just because the user is typing while this is happening?

It's possible we can't resolve an item because something in the program changed, yes. But it was already possible in Strada, I think. If this fails though, the worse that can happen is we try to request completions in a file/position that doesn't exist or that produces different symbols, and then we simply won't fill in the lazy properties.
Also, I don't know if it's as simple as "this file changed, we can no longer resolve the completion item", because I can imagine the common scenario being a user starts typing, the client request completions and gets back a list, and then the user continues typing the same identifier, let's say. The file has changed, but if you request completions at the old position you'd get the same list, and so we'll still be able to resolve the items. And in this scenario I think the client won't re-request completions with every new character, but will instead re-use the old list for a bit and filter it down based on the new characters. So I don't know, maybe it's fine as it is, or maybe we can do better and re-use the old snapshot once we have that?

jakebailey · 2025-07-10T19:33:24Z

internal/parser/jsdoc.go

@@ -285,6 +286,7 @@ loop:
 	}

 	if len(comments) > 0 {
+		comments[len(comments)-1] = strings.TrimRightFunc(comments[len(comments)-1], unicode.IsSpace)


Ugh, so there was a place where this mattered? can we instead post-process those away rather than trimming just one side of the comments or something?

This matters for quickinfo and everything that depends on it, and possibly other places that handle jsdoc, we just didn't have tests for any of this before. Do you mean post-processing in quickinfo itself? I added this code here in the parser because that's exactly what Strada does, and I don't know if it'll be enough to just do it on quickinfo.

I don't know if what's added here is exactly like Strada; see also #1280 where I basically undid a bunch of this in hopes it wouldn't affect anything, with the assumption that any caller who cared would instead do that processing themself or something.

But, maybe it's .Text() which needs to do that.

Well, it's the closest equivalent to Strada's, since that part of the code changed a bit in Corsa. But I'm open to ideas here, I'm just not very familiar with jsdoc handling.
The thing I find tricky with changing .Text() is that we want to trim the last comment text on a list of comments, right? since the intermediary ones should keep their trailing spaces.

Eh, what you've done is fine, if it really is just the last one that must be trimmed.

jakebailey · 2025-07-10T19:45:54Z

internal/ls/completions.go

@@ -4193,7 +4202,7 @@ func (l *LanguageService) createLSPCompletionItem(
 		InsertTextFormat: insertTextFormat,
 		TextEdit:         textEdit,
 		CommitCharacters: commitCharacters,
-		Data:             nil, // !!! auto-imports
+		Data:             &data,


This will eventually come to bite us, but one thing we are not doing at all in the LS at the moment is respecting the client's capabilities. All of these things should actually be things where we check the client's caps and say "do they support resolve?", and if not, include all of this information like docs up front, or similar.

Not really critical for this PR, but something to think about.

Yep, that's what I meant to note in the description:

Note that for now, if a client doesn't support lazy computation of detail and documentation, we simply won't fill in those properties.

Especially for auto-imports, this could be detrimental to performance.

Ah, missed it, great.

jakebailey · 2025-07-10T19:48:06Z

yesss

gabritto added 13 commits July 4, 2025 16:31

WIP: completionItem/resolve

2a34abb

initial completionItemResolve handling

e7c0eb9

add handleCompletionItemResolve to server

20d418e

fix completions call to quickinfo

5250898

fix tests

a550000

format

631d5c0

add detail and documentation comparison to fourslash

2e39398

convert details and documentation in completion item

9047f1a

Merge branch 'main' into gabritto/completionresolve

35641bd

update failing tests

f8bc9b6

add string completion detail

e5ac850

fix lint and test errors

f595881

Merge branch 'main' into gabritto/completionresolve

40940b1

jakebailey reviewed Jul 10, 2025

View reviewed changes

gabritto marked this pull request as ready for review July 10, 2025 20:22

gabritto requested review from andrewbranch, jakebailey and iisaduan July 10, 2025 20:36

jakebailey approved these changes Jul 10, 2025

View reviewed changes

gabritto added this pull request to the merge queue Jul 10, 2025

Merged via the queue into main with commit 48799f0 Jul 10, 2025
20 of 21 checks passed

gabritto deleted the gabritto/completionresolve branch July 10, 2025 20:59

Handle completionItem/resolve #1384

Handle completionItem/resolve #1384

Uh oh!

Conversation

gabritto commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabritto Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Jul 10, 2025

Uh oh!

Uh oh!

Uh oh!

Handle `completionItem/resolve` #1384

Handle `completionItem/resolve` #1384

gabritto commented Jul 10, 2025 •

edited

Loading

gabritto Jul 10, 2025 •

edited

Loading