[jwortmann]

When handling a workspace/applyEdit request from the server, the client must return a ApplyWorkspaceEditResult.

Currently, we unconditionally return "applied": true, even when the WorkspaceEdit cannot be applied correctly (e.g. due to failed to open view for URI, or view version doesn't match a given version in the WorkspaceEdit):

LSP/plugin/core/sessions.py

Line 1883 in ced62c9

).then(lambda _: {'applied': True})

Apart from that, we declare

LSP/plugin/core/sessions.py

Line 540 in ced62c9

"failureHandling": FailureHandlingKind.Abort,

for handling WorkspaceEdits, which means that (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#failureHandlingKind)

	/**
	 * Applying the workspace change is simply aborted if one of the changes
	 * provided fails. All operations executed before the failing operation
	 * stay executed.
	 */
	export const Abort: FailureHandlingKind = 'abort';

But in reality, we simply continue with further changes even if one of the change failed:

LSP/plugin/core/sessions.py

Lines 1674 to 1680 in ced62c9

	for uri, (edits, label, view_version) in changes.items():
	view_state_actions = self._get_view_state_actions(uri, auto_save)
	promises.append(
	self.open_uri_async(uri)
	.then(partial(handle_view, edits, label, view_version, uri, view_state_actions))
	)
	return Promise.all(promises) \

So the actual behavior isn't what we declared in the client capabilities as our failure handling strategy. Also I think this async implementation (via Promise.all) cannot work if support for resource operations (create / rename) should be implemented later at some point (#2706), because we cannot run a TextDocumentEdit before a file for the corresponding URI has been created or renamed. This means that if the WorkspaceEdit contains documentChanges, we have to apply them sequentially.

This PR should fix that, but only for WorkspaceEdits from the workspace/applyEdit request. Other WorkspaceEdits, for example from code actions or from a textDocument/rename request still use the previous (current) implementation. For those it would require some more refactorings, because they currently return a custom WorkspaceEditSummary which is used to display the number of changes and affected files in the UI (in case of rename).

---

Also this PR still ignores the needsConfirmation flag from the ChangeAnnotation of AnnotatedTextEdit:

	/**
	 * A flag which indicates that user confirmation is needed
	 * before applying the change.
	 */
	needsConfirmation?: boolean;

If we want to support that, for example with a sublime.ok_cancel_dialog, then probably more considerations will be required to make the code wait for the user choice.

[jwortmann]

I've read in some Pyright issue that (unlike TypeGuard) TypeIs is not supported for TypedDicts, because TypedDicts are "structural types" (not sure what that means exactly). So it will probably never be possible to use TypeIs for the LSP types, at least when using Pyright.

[jwortmann]

Note that this seems to cause the documentChanges of a WorkspaceEdit from a workspace/applyEdit request being shown as an empty array in the LSP log panel. But I would consider this as a bug in our logging functionality (I opened related issue #2897).

[rwols]

Yeah, we can log it from the transports level probably. Although, the problem with logging the plain JSON is that you can't easily hide large payloads while still printing which request/notification it is (and the request ID). Maybe there's a way to deal with that.

[jwortmann]

I've tested with different code actions from LSP-julia, which don't contain a WorkspaceEdit but trigger server commands via workspace/executeCommand, which in turn trigger the workspace/applyEdit requests from the server.

(Although those WorkspaceEdits only contain a single TextDocumentEdit, and also obviously they are all applied successfully. I don't know how we could test a WorkspaceEdit that does fail in practice - but perhaps we can write a test for that.)

[jwortmann]

I think it would be nice to keep this check here (because it is executed on the UI thread) to ensure that there can't be any race condition when applying this TextDocumentEdit while typing at the same time. But I don't think there is any good way to signalize from the command back to the calling method that the edit has failed, and we need that for stopping the execution and returning back the failureReason. Therefore I moved this check into the functions which call this command.

[rwols]

I have some ideas for this once the feat/asyncio branch is merged in main. But they're still a bit fuzzy.

[jwortmann]

I hope with asyncio we can also rewrite the recursive function as a simple for-loop which awaits each document change.

[rwols]

Indeed. But for now I kept the recursion in my branch.

[rwols]

These type of statements involving the := operator where the value is used before the assignment are getting into "too clever" territory :) but I'll allow it.

Something more plain and dumb is more clear to read, IMO:

metadata = params.get('metadata')
is_refactoring = metadata is not None and metadata.get('isRefactoring, False)

.

or even

is_refactoring = params.get('metadata', {}).get('isRefactoring', False)

[jwortmann]

I initially used the

is_refactoring = params.get('metadata', {}).get('isRefactoring', False)

when I added it recently, but the change I did here with the if-condition is a micro optimization that doesn't require to create an empty dict in most cases and then do the membership test (which involves calculating the hash value, I believe) for 'isRefactoring'. So the new code should be a little bit more efficient. In general I quite like the := operator and think it often results in more readable code. But okay, I'll try not to push it too far in the future :-)

[rwols]

I have some ideas for this once the feat/asyncio branch is merged in main. But they're still a bit fuzzy.

[rwols]

Yeah, we can log it from the transports level probably. Although, the problem with logging the plain JSON is that you can't easily hide large payloads while still printing which request/notification it is (and the request ID). Maybe there's a way to deal with that.

[jwortmann]

As a sidenote, I think we had an unspoken convention over the past time that for non-trivial pull requests, once it is approved, the PR author is the one who merges it. I don't think I had anything particular to add here, but in general the author probably knows best when it is fully ready, and sometimes you want to look over it again later, or still have something in your mind that you want to add. I think then it's a bit easier to handle and you don't always need to mark every PR as a draft PR until it gets merged eventually.

[rwols]

As a sidenote, I think we had an unspoken convention over the past time that for non-trivial pull requests, once it is approved, the PR author is the one who merges it.

OK, I'll keep this in mind. In this particular case, I was going to make changes related to the document changes, so I wanted to make sure this was merged before I got to that.