Allow asWorkspaceEdit to be overridden

[DanTup]

I implemented the Rust extension described in #724 / https://github.com/rust-lang/rust-analyzer/blob/master/docs/dev/lsp-extensions.md#snippet-textedit to support snippets in Code Actions. However, in the client I'm unable to access the new insertTextFormat field on TextEdit. Initially I implemented a hack that just looked for snippet placeholders in the edit body, but users have found if they have $0 (etc.) in their code, some refactors will not work correctly.

In microsoft/language-server-protocol#724 (comment), @dbaeumer said we can use middleware to use our own asWorkspaceEdit, however as far as I can tell, this can't be done in middleware because the middleware has to call next(), which sends the request to the server and deserialises and converts the response (so it appears to be too late to access the raw data before the insertTextFormat field is lost).

I think what we really want to do is replace protocol2CodeConverter.asCodeActionResult. We can already replace some functions on protocol2CodeConverter (eg. via clientOptions.uriConverters) but as far as I can tell, we can't replace this (because _p2c is private, and only has a public getter, and is constructed in the constructor only allowing URI converters to be changed).

@dbaeumer if there's not already a way to do this, would you accept a PR that allowed the whole of protocol2CodeConverter/code2ProtocolConverter to be provided to the constructor?

[dbaeumer]

In general, yes, but it depends a little bit on the impact of that export (e.g. what else we would need to export).

Another idea would be to change the middleware so that the result can be captured before conversion. I know that others have asked for this as well. But it would be quite some work.

[DanTup]

@dbaeumer are you thinking of just adding these functions to the middleware, something like being able to do:

const middleware = {
	asWorkspaceEdit: (
		item: ls.WorkspaceEdit | undefined | null,
		token: code.CancellationToken | undefined,
		original: (item: ls.WorkspaceEdit | undefined | null, token?: code.CancellationToken) => Promise<code.WorkspaceEdit | undefined>,
	): Promise<code.WorkspaceEdit | undefined> => {
		return original(item, token);
	}
}

Then delegating calls to p2c.asWorkspaceEdit to middleware.asWorkspaceEdit if it's set?

If it's not an easy thing to do, do you know whether it's possible to do something like this in the meantime (I understand if I update the client I may need to change this etc.):

const p2c = (client as any)._p2c;
const originalAsWorkspaceEdit = p2c.asWorkspaceEdit as Function;
function newAsWorkspaceEdit(...args: any[]) {
	console.log("running!"); // this is never called 😞
	return originalAsWorkspaceEdit.apply(p2c, args);
}

p2c.asWorkspaceEdit = newAsWorkspaceEdit;
console.log(p2c.asWorkspaceEdit === newAsWorkspaceEdit); // true

This prints true in the last line as if the function was replaced (and I can see originalAsWorkspaceEdit is the original function), but the new function is never called when workspace edits are being deserialised.

[dbaeumer]

Actually no. The problem with the middleware with each middleware function is that it can't intercept the result of the request before we convert it. It would be cool if we could do that in general. Might be at some point useful for other requests as well.

[DanTup]

I guess I was thinking my asWorkspaceEdit function would be applied to all involved types (including the result types). It would require a lot of new signatures adding to middleware, but I think that's unavoidable to allow every response to be intercepted anyway? (although they could perhaps be added progressively).

If it's not that simple, do you know why my attempt to replace asWorkspaceEdit wasn't working? It would be nice to get some fix for my issue, even if it needs redoing properly later.

[dbaeumer]

@DanTup the asWorkspaceEdit would work but it is a total new concept I don't want to introduce. If we start this there will be an asLocation, as.... which is the wrong solution for the problem. I would rather let you exchange the converter. However the converter has the problem that it is use API right now. But you would need an implement API which is bad as well since I would need to keep more things stable then.

[DanTup]

@dbaeumer understood. Do you know why my attempt to replace asWorkspaceEdit above doesn't work? I know it's not a good fix, but I'd like to find a way to handle my issue in the meantime as it's resulting incorrect code being inserted.

[dbaeumer]

@DanTup as said above it would work. However, adding this would make it API (otherwise you can't override it) and this introduces a concept I don't want to keep. As said, I would rather let you exchange the converter.

[DanTup]

@dbaeumer sorry, I wasn't very clear. I understand your position on an official API, but I was trying to implement a workaround inside my extension code in the meantime to fix the bug. I was trying to overwrite the method (unofficially) like this:

const p2c = (client as any)._p2c;
const originalAsWorkspaceEdit = p2c.asWorkspaceEdit as Function;
function newAsWorkspaceEdit(...args: any[]) {
	console.log("running!"); // this is never called 😞
	return originalAsWorkspaceEdit.apply(p2c, args);
}

p2c.asWorkspaceEdit = newAsWorkspaceEdit;
console.log(p2c.asWorkspaceEdit === newAsWorkspaceEdit); // true

It seems like p2c.asWorkspaceEdit was overwritten, however my function is never called when the response is received, the old function still is, but I don't understand why. (I appreciate this is likely more of a TS/JS question than VSC/LSP though!)

[DanTup]

nm, I figured this out... The original call is to p2c.asCodeActionResult, which then calls asCodeAction and asWorkspaceEdit, but those calls are internally to the local functions, so they're not affected by me changing p2c.asWorkspaceEdit.

So, it'll definitely be messier this way, but it might give me a fix for now if it's not clear what the official solution to this will be yet. Thanks!

[dbaeumer]

Great you figured it out. And sorry for misunderstanding your question at the beginning.

[DanTup]

np! The workaround is definitely quite hacky and could be fragile across LSP Client updates, but I've added tests for it and will change it if/when there's a more formal way to do it. Thanks!