Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add native lsp support #3406

Open
quolpr opened this issue Jun 27, 2024 · 7 comments
Open

Add native lsp support #3406

quolpr opened this issue Jun 27, 2024 · 7 comments

Comments

@quolpr
Copy link

quolpr commented Jun 27, 2024

Hello! I've been working on adding LSP support to nvim and encountered a few issues:

  1. The LSP is sending _onDiagnostics instead of the expected textDocument/publishDiagnostics.
  2. The LSP doesn't send "edits" with the codeAction. Below is an example of the edit JSON: edit json
  3. It's necessary to handle the _onWorkspaceConfigForDocumentRequest command to make cspell work correctly.
  4. Nvim sends the cursor position instead of the range of the selected word on codeAction. I had to patch the server with the following code to make it work:
diff --git a/packages/_server/src/codeActions.mts b/packages/_server/src/codeActions.mts
index 5023ca80..f8f3a603 100644
--- a/packages/_server/src/codeActions.mts
+++ b/packages/_server/src/codeActions.mts
@@ -93,7 +93,16 @@ class CodeActionHandler {
             textDocument: { uri },
         } = params;
         const { diagnostics } = context;
-        const spellCheckerDiags = diagnostics.filter((diag) => diag.source === Validator.diagSource);
+
+        const firstIntersection = diagnostics.find(
+            (diag) => diag.source === Validator.diagSource && range.intersect(diag.range, params.range),
+        );
+        if (firstIntersection) {
+            params.range = firstIntersection.range;
+        }
+        const spellCheckerDiags = diagnostics.filter(
+            (diag) => diag.source === Validator.diagSource && range.intersect(diag.range, params.range),
+        );
         const eslintSpellCheckerDiags = diagnostics.filter((diag) => diag.source === 'eslint' && diag.code == '@cspell/spellchecker');

         if (!spellCheckerDiags.length && !eslintSpellCheckerDiags.length) return [];

These issues make integrating cspell into nvim and other editors challenging. I'm considering adding a new setting, like cSpell.isNativeMode, which would make cspell function like a standard LSP. This could be a significant improvement for other editors!

Here is a workaround for nvim - nvim workaround
And here is one for sublime - sublime workaround

@Jason3S
Copy link
Collaborator

Jason3S commented Jun 27, 2024

@quolpr,

Thank you for the detailed explanation.

As you have seen, the LSP has been strongly tailored to work with the VS Code extension. I'm open to making changes so it is easier to integration with nvim and sublime.

A compatibility mode as you suggest with cSpell.isNativeMode is a viable suggestion. It might also be easier/better to create a pure LSP CSpell Server, since there are other uses for an LSP in addition to an editor.

Background Information

  1. The LSP is sending _onDiagnostics instead of the expected textDocument/publishDiagnostics.

One of the biggest requests / complaints was to show spelling issues independent from the normal coding issues. It was requested to hide them from the problems pane and to use custom decorations. To accomplish this, it was necessary to send Diagnostics as a custom notification instead of using the LSP Diagnostics Reporting. The format of the Diagnostics have not changed, so it should still be possible, it just needs to be a setting telling it to use publishDiagnostics. Related code:

function sendDiagnostics(result: ValidationResult) {
log(`Send Diagnostics v${result.version}`, result.uri);
const { uri, version, diagnostics } = result;
const diags: Required<PublishDiagnosticsParams> = { uri, version, diagnostics };
// This is to filter out the "Off" severity that is used to hide issues from the VS Code Problems panel.
const knownDiagnosticSeverityLevels = new Set<number | undefined>([
DiagnosticSeverity.Error,
DiagnosticSeverity.Warning,
DiagnosticSeverity.Information,
DiagnosticSeverity.Hint,
]);
function mapDiagnostic(diag: Diagnostic): Diagnostic {
return {
...diag,
severity: knownDiagnosticSeverityLevels.has(diag.severity) ? diag.severity : undefined,
};
}
const diagsForClient = { ...diags, diagnostics: diags.diagnostics.map(mapDiagnostic) };
catchPromise(clientServerApi.clientNotification.onDiagnostics(diagsForClient));
}

  1. The LSP doesn't send "edits" with the codeAction. Below is an example of the edit JSON:

It sends a command instead of edits. The command gets turned into edits on the client side. I was necessary to do it this way to use the VSCode rename feature. It is not too difficult to send edits instead.

Related code:

sugs.map(({ word, isPreferred }) => ({ word: Text.isLowerCase(word) ? Text.matchCase(srcWord, word) : word, isPreferred }))
.filter(uniqueFilter())
.forEach((sug) => {
const sugWord = sug.word;
const title = suggestionToTitle(sug, issueType);
if (!title) return;
const cmd = createCommand(title, 'cSpell.editText', uri, textDocument.version, [replaceText(diag.range, sugWord)]);
const action = createAction(cmd, [diag], sug.isPreferred);
actions.push(action);
});
}

The codeAction also has options to add words to the user's dictionaries. But that requires significant client side support. Set cSpellhideAddToDictionaryCodeActions to true to hide those not implemented options.

Related code:

if (!docSetting.hideAddToDictionaryCodeActions) {
actions.push(...generateTargetActions(textDocument, spellCheckerDiags, word, targets));
}

  1. It's necessary to handle the _onWorkspaceConfigForDocumentRequest command to make cspell work correctly.

This configuration information is connected to adding words to dictionaries (the targets) in the client. If it is not necessary to support adding words to dictionaries, then It should be possible to ignore this.

  1. Nvim sends the cursor position instead of the range of the selected word on codeAction. I had to patch the server with the following code to make it work:

I can see how that might be an issue.

Related code:

const spellCheckerDiags = diagnostics.filter((diag) => diag.source === Validator.diagSource);
const eslintSpellCheckerDiags = diagnostics.filter((diag) => diag.source === 'eslint' && diag.code == '@cspell/spellchecker');
if (!spellCheckerDiags.length && !eslintSpellCheckerDiags.length) return [];
const textDocument = this.documents.get(uri);
if (!textDocument) return [];
const rangeIntersectDiags = [...spellCheckerDiags, ...eslintSpellCheckerDiags]
.map((diag) => diag.range)
.reduce((a: LangServerRange | undefined, b) => a && range.intersect(a, b), params.range);
// Only provide suggestions if the selection is contained in the diagnostics.
if (!rangeIntersectDiags || !(range.equal(params.range, rangeIntersectDiags) || isWordLikeSelection(textDocument, params.range))) {
return [];
}

@Jason3S
Copy link
Collaborator

Jason3S commented Jun 27, 2024

@quolpr,

One of the challenges here is that I don't have any insight into who is trying to use this as a LSP service, making it very easy to break things.

In the latest version, the server sends RPC requests to the client to load file if they use a virtual protocol. See: _server/src/vfs/CSpellFileSystemProvider.mts

@quolpr
Copy link
Author

quolpr commented Jun 28, 2024

@Jason3S thank you for all your comments! That will really help me. I hope I will start soon on working this 🙂

@BaptisteRoseau
Copy link

We would also be interested by a native LSP support to use it in the Zed editor, there is a dedicated issue on their end for this purpose: zed-industries/extensions#1414

@quolpr
Copy link
Author

quolpr commented Oct 24, 2024

Update: I've developed a working solution for Neovim LSP integration. You can find the implementation here:
https://gist.github.com/quolpr/2d9560c0ad5e77796a068061c8ea439c

The workaround functions as intended, so I decided to stop working on native mode. By the way, the codebase is well-structured, making it straightforward to implement this native mode if anyone wants to take it on 🙂

@Jason3S
Copy link
Collaborator

Jason3S commented Oct 25, 2024

@quolpr,
Thank you.
Would you make your gist public? I'm curious about your workaround.

@quolpr
Copy link
Author

quolpr commented Oct 25, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants