Resolve 1: Support for resolve in overloaded-record-dot

[joyfulmantis]

In order to better understand how resolve should be implemented, I have started to implement codeAction resolve in overloaded-record-dot. This branch is based off of the new-lsp-release.

[michaelpj]

Hmmm, this made me realize that something funny may happen here. Suppose we have N code action handlers, each of which has a corresponding resolve handler. Now if one of the code action handlers produces a code action, then when the client asks to resolve it... it's going to get sent to every resolve handler. So we'll need to make sure that resolve handlers "know" if it's one of "their" code actions...

[michaelpj]

And indeed, I think we therefore don't want to combine the results of multiple resolve handlers firing. What would that even mean? Something has gone wrong if that has happened! We should get exactly one response.

Hard to do for now, but I do think that combineResponses should be able to throw an error in cases like this. For now I'd probably do the crappy "just take the first result" thing we do elsewhere.

[joyfulmantis]

So even if every plugin's resolve handler used the same type theoretically Data.Unique should allow types to know whether a resolve belongs to them (resolvable ones eventually anyways). To make sure no extra processing was needed I was thinking of having the plugin record their name or id in the type serialized to the data field, and then first match on that, to make sure no extra processing was needed.

Regarding combineResponses, I was actually originally just returning the unmodified codeAction if I was unable to resolve with the provided resolve data, which is why combineRespones would still be needed to find the modified codeAction from the list. I guess I could/should just throw an error, and that would be equivalent to returning Nothing? The plugin ultimately responsible for the resolve also needs to throw a ContentModified responseError if it can't resolve it. So not actually sure we can use pluginResponse in the end (or at least we need to modify it)

[michaelpj]

Right, if we use standard infrastructure and tag every code action with a Unique then we probably should be fairly okay. Although it requires some state on the plugin side somewhere to remember which uniques it produced. The advantage of the "just pass the CodeActionParams again" approach is that it's easier to be stateless if you want to. Not sure if it's possible to be stateless in general, though 🤔

I guess I could/should just throw an error, and that would be equivalent to returning Nothing?

I think that's fine. I think we run all the handlers, throw away any that failed and then combine the results of the rest. We could potentially also use the pluginResponsible method for this, I think? We use it in a similar way to restrict the scope of some handlers. So if we put the responsible plugin ID in the data then I think we could restrict to just that plugin in pluginResponsible.

[michaelpj]

WDYT about the idea of just reusing the CodeActionParams?

[joyfulmantis]

At least in this case, the CodeActionParams doesn't make sense. The problem with the codeActionParams is we are going to need to do processing anyways to know whether we can provide the codeAction, and right now it's a title too, so it makes sense to just process once instead of once to present and once to execute

[michaelpj]

Right. I think it definitely makes sense that we'll want a "stateful version" of resolve-based handlers. Maybe we'll also want a stateless one... I think there are some plugins that don't define any of their own rules so don't even have anywhere to put state. But perhaps easier to do a few and then refactor afterwards.

[michaelpj]

Is newUnique threadsafe? We run handlers in individual threads, so there could well be multiple instances of this running at once.

[joyfulmantis]

Yes https://stackoverflow.com/questions/70515566/when-is-data-unique-thread-safe

[michaelpj]

great

[michaelpj]

we might have to worry about contention on the ioref if we end up using this everywhere, but I expect that won't manifest until we're using it a lot. We should remember to try and check when we're nearly done

[michaelpj]

Nice, I like where this is going.

[michaelpj]

Just flagging that this seems like it's going to be a pretty much universal pattern for resolve handlers: the first thing they're going to do is decode their data from the data field. So we might want to make it part of the generic machinery.

[michaelpj]

Looking at the spec, I'm not sure this is the correct meaning of ContentModified. I don't think there's actually anything wrong with the client sending us a document change notification in between asking for code actions and resolving them, even if we can't handle that. Perhaps just RequestFailed?

[joyfulmantis]

It was mentioned in a GitHub issue that this was the way to do it. microsoft/language-server-protocol#1738

[michaelpj]

Okay, let's link to that when we do this!

[michaelpj]

Bonus points: make a PR upstream to clarify in the spec that this is what you should do!