Support LSP based source code formatting on file save

[ghentschke]

As a user of a LSP based editor I want to format my source code on a file save action. (see this cdt-lsp issue)

Therefor I want to use the org.eclipse.lsp4e.operations.format.LSPFormatter class. The package org.eclipse.lsp4e.operations.format should be exported.

[rubenporras]

Hi @ghentschke , you can already implement format on save with current LSP4E. See #117. Would that work for you?

[ghentschke]

I'll ckeck that. Thanks for the hint!

[ghentschke]

Unfortunately is willSaveWaitUntil not suppported by the actual clangd 16.0.2 server:

TextDocumentSyncOptions [
  openClose = true
  change = Incremental
  willSave = null
  willSaveWaitUntil = null
  save = Either [
    left = true
    right = null
  ]
]

[ghentschke]

@rubenporras: Is there a way a user can disable the format-on-safe when using #117

[rubenporras]

If there is an standard way of doing that, I could not find it. For our implementation, we extended the client with an API

CompletableFuture<@NonNull Boolean> isFormatOnSaveEnabled(@NonNull URI uri);

and with a configuration page.

Then, the server ask the client if it should apply autoformat.

That works for us because we develop our own server and our own client, but since cdt-lsp is an independent project, that will not work. If you figure it out, I would be interested.

[ghentschke]

If you figure it out, I would be interested

One solution might be a OSGi service in LSP4E. This service can be provided by vendors who provides a server via the languageServer extension point as well. The provided interface of that service could look something like:

public interface IFormatOnSaveEnabler{
  Optional<Boolean> isFormatOnSaveEnabled(URI uri);
}

[mickaelistria]

@ghentschke I'm not much enthusiast in opening an internal package as API for a use-case that already has a LSP-standard approach. Couldn't clangd be changed to support standard LSP approach with willSaveUntil? Or maybe could clangd be changed to send an applyEdit after a save to fix some formatting?

[ghentschke]

I know that this approach is not the best solution. I'll open an issue in the clangd repo. This should be a temporary solution until clangd supports willSaveWaitUntil. I'll check the applyEdit as well.

Maybe make org.eclipse.lsp4e.operations.format x-friends with org.eclipse.cdt.lsp ?

[mickaelistria]

If this feature is very urgent or critical for CDT, we can add a x-friends. However, it's better if we can just do nothing in LSP4E exactly to ensure that the work is addressed where it's the most valuable (clangd)

[ghentschke]

I am little stuck in the middle now. I've to argue why willSaveWaitUntil should be supported by clangd.
The main argument against willSaveWaitUntil is the better control from client/editor side and a possible better UX:

The client can already request formatting at any point, including just before save. Putting formatting in willSaveWaitUntil puts the server in control of:

• whether formatting is performed before saving or not (this would need to be a config option in .clangd)
• which ranges should be formatted (full file, lines edited this session, lines edited vs source control baseline)

My impression is that having the client in control of these gives better UX. These are user preferences that you can connect to the checkboxes in your screenshot. In the case of edited lines, you can ensure that there's no disagreement on what's "edited" (e.g. between server vs client diff algorithms), the client may have relevant source control baseline info (clangd does not) etc.etc.

I can share this evaluation. I would propose to support both approaches in LSP4E: the formatting via willSaveWaitUntil, if the server supports it, or do a formatting/rangeFormatting as suggested here. My proposal for the latter could be something like this:

private void formatCode(IProgressMonitor monitor, ITextFileBuffer buffer) {
var document = buffer.getDocument();
if (document != null) {
	try {
		IRegion[] changedRegions = isLimitedFormatCode()
				? EditorUtility.calculateChangedLineRegions(buffer, monitor)
				: new IRegion[] { new Region(0, document.getLength()) };
		var textSelection = new MultiTextSelection(document, changedRegions);
		formatter.requestFormatting(document, textSelection).get(1000, TimeUnit.MILLISECONDS)
				.ifPresent(edits -> {
					try {
						edits.apply();
					} catch (final ConcurrentModificationException ex) {
						//								ServerMessageHandler.showMessage(Messages.LSPFormatHandler_DiscardedFormat,
						//										new MessageParams(MessageType.Error,
						//												Messages.LSPFormatHandler_DiscardedFormatResponse));
					} catch (BadLocationException e) {
						Platform.getLog(getClass()).error(e.getMessage(), e);
					}
				});
	} catch (BadLocationException | CoreException | InterruptedException | ExecutionException
			| TimeoutException e) {
		Platform.getLog(getClass()).error(e.getMessage(), e);
	}
}
}

formatCode can be called in org.eclipse.lsp4e.DocumentContentSynchronizer.documentAboutToBeSaved() as well. It is very similar to willSaveWaitUntil does, except that the editor/client has more control of the formatting.

The settings could be fetched from the editor as proposed in this comment.

[rubenporras]

I now how it feels to be stuck between two projects with two opinions myself :)

Regarding on why the server should support willSaveWaitUntil, to me it has the advantage than it works in all the clients, which is kind of the point of LSP. The documentation on this is sparse, but for example, Neovim announces it:

https://neovim.io/doc/user/news-0.9.html

Added support for the willSave and willSaveWaitUntil capabilities to the LSP client. willSaveWaitUntil allows a server to modify a document before it gets saved. Example use-cases by language servers include removing unused imports, or formatting the file.

I agree that the user (and thus the client) should control when to apply the edits and which ones, and that looks like it is not an standard in the protocol. Yet, from the examples, one can imagine that the intention of the client capabilities is to enable/disable it, as taken from https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#clientCapabilities

{
	"method": "client/registerCapability",
	"params": {
		"registrations": [
			{
				"id": "79eee87c-c409-4664-8102-e03263673f6f",
				"method": "textDocument/willSaveWaitUntil",
				"registerOptions": {
					"documentSelector": [
						{ "language": "javascript" }
					]
				}
			}
		]
	}
}

Which such a document selector, the client can tell the server for which languages to apply willSaveWaitUntil and for which not. The registerOptions are defined as LSPAny so, literally, that can be anything. Another option would be that if the user does not want format on save, the client could announce it does not support the capability. I have also read that VSCode supports different save options (microsoft/vscode#177787), but I do not know if those are passed to the LS or not as registrationOptions.

Maybe you could open a ticket for the LSP project and ask for clarification?

[ghentschke]

Regarding on why the server should support willSaveWaitUntil, to me it has the advantage than it works in all the clients, which is kind of the point of LSP

Agree. But that could be a long road (clangd has to implement it. See my answer in my clangd issue)

Could we implement my proposal above as a fallback in case the server does not support willSaveWaitUntil (yet)?

[rubenporras]

Following on this, I have found this morning https://learn.microsoft.com/en-us/visualstudio/extensibility/adding-an-lsp-extension?view=vs-2022, it says:

Support for custom language-server-specific settings is available, but it is still in the process of being improved. Settings are specific to what the language server supports and usually control how the language server emits data. For example, a language server might have a setting for the maximum number of errors reported. Extension authors would define a default value, which can be changed by users for specific projects.

The text gives then the example of the setting foo.maxNumberOfProblems: -1. That is the same format used in the example https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration with the option cpp.formatterOptions.

So it looks to me that the usual way of support it would be that the server can autoformat and that the behaviour can be controlled by such options. You could just return the value compatible by clangd by overriding the class that this PR https://github.com/eclipse/lsp4e/pull/792/files fixes (I have realized all this by reviewing this change).

I think that this technically much easier than #783 and its all part of the spec. Could you maybe still convince clangd developers or make a PR for them? It looks to me that we are creating mid-size custom code with #783, so maybe it is worth checking again that it is indeed needed?

[ghentschke]

Could you maybe still convince clangd developers or make a PR for them

I could but since there are ~567 open issues and the last commit was on April 14 I guess it will take a long time until this feature is implemented. Unfortunately I have not the time to implement it by myself in clangd.