Skip to content

Use LSP-over-legacy for hover #278

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.

Sign up for GitHub

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

Draft
helin24 wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Use LSP-over-legacy for hover #278

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86c7fa4
Use LSP-over-legacy for hover
helin24 Mar 5, 2026
23afe83
Avoid pipe broken issue
helin24 Mar 5, 2026
70c0efe
Changes to use stream
helin24 Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions third_party/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ dependencies {
bundledModule("intellij.platform.coverage.agent")
bundledPlugin("org.jetbrains.plugins.yaml")
bundledPlugin("com.intellij.copyright")
plugin("com.redhat.devtools.lsp4ij:0.19.2")
}

implementation(fileTree("lib") { include("*.jar") })
Expand Down
9 changes: 9 additions & 0 deletions third_party/src/main/java/com/jetbrains/lang/dart/analyzer/DartAnalysisServerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2586,6 +2586,15 @@ public void sendRequest(String id, JsonObject request) {
}
}

/**
* Send an LSP JSON-RPC message directly to the server.
*/
public void sendLspMessage(JsonObject request) {
if (myServerSocket != null && myServerSocket.isOpen()) {
myServerSocket.getRequestSink().add(request);
}
}

/**
* Send the request and associate it with the passed {@link com.google.dart.server.Consumer}.
*/
Expand Down
245 changes: 245 additions & 0 deletions third_party/src/main/java/com/jetbrains/lang/dart/analyzer/DartLanguageServerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,245 @@
package com.jetbrains.lang.dart.analyzer;

import com.jetbrains.lang.dart.logging.PluginLogger;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.project.Project;
import com.redhat.devtools.lsp4ij.LanguageServerFactory;
import com.redhat.devtools.lsp4ij.server.StreamConnectionProvider;
import com.redhat.devtools.lsp4ij.client.features.LSPClientFeatures;
import com.redhat.devtools.lsp4ij.client.features.LSPHoverFeature;
import com.redhat.devtools.lsp4ij.client.features.LSPCompletionFeature;
import com.redhat.devtools.lsp4ij.client.features.LSPDiagnosticFeature;
import com.redhat.devtools.lsp4ij.client.features.LSPFormattingFeature;
import com.intellij.psi.PsiFile;
import org.jetbrains.annotations.NotNull;

import java.io.InputStream;
import java.io.OutputStream;
import java.io.PipedInputStream;
import java.io.PipedOutputStream;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import com.google.dart.server.ResponseListener;

public class DartLanguageServerFactory implements LanguageServerFactory {

private static final Logger LOG = PluginLogger.INSTANCE.createLogger(DartLanguageServerFactory.class);

public DartLanguageServerFactory() {
}

@Override
public @NotNull StreamConnectionProvider createConnectionProvider(@NotNull Project project) {
return new StreamConnectionProvider() {
private PipedInputStream lspInputStream; // lsp4ij reads from here
private PipedOutputStream dartToLspStream; // we write Dart server responses here

private PipedInputStream lspToDartStream; // we read lsp4ij requests from here
private PipedOutputStream lspOutputStream; // lsp4ij writes to here

private Thread lspToDartThread;
private ResponseListener dartResponseListener;
private DartAnalysisServerService dasService;

@Override
public void start() {
try {
// Setup streams
lspInputStream = new PipedInputStream();
dartToLspStream = new PipedOutputStream(lspInputStream);

lspToDartStream = new PipedInputStream();
lspOutputStream = new PipedOutputStream(lspToDartStream);
} catch (IOException e) {
LOG.error("Failed to initialize piped streams for lsp4ij", e);
return;
}

dasService = DartAnalysisServerService.getInstance(project);

// 1. Listen for Dart server responses and unwraps them for lsp4ij
dartResponseListener = new ResponseListener() {
@Override
public void onResponse(String jsonString) {
try {
JsonObject jsonObject = JsonParser.parseString(jsonString).getAsJsonObject();
JsonObject lspPayload = null;

// Check if it is a response to our lsp.handle request
if (jsonObject.has("result") && jsonObject.get("result").isJsonObject()) {
JsonObject result = jsonObject.getAsJsonObject("result");
if (result.has("lspResponse")) {
lspPayload = result.getAsJsonObject("lspResponse");
}
}

// Check if it is a notification or server-to-client request
if (lspPayload == null && jsonObject.has("params") && jsonObject.get("params").isJsonObject()) {
JsonObject params = jsonObject.getAsJsonObject("params");
if (params.has("lspMessage")) {
lspPayload = params.getAsJsonObject("lspMessage");
}
}

if (lspPayload != null) {
String lspMessageString = lspPayload.toString();
LOG.info("Dart server sent lsp payload: " + lspMessageString);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The application logs raw LSP payloads and messages at INFO and WARN levels. LSP messages frequently contain sensitive information, including source code snippets, file paths, and symbol names from the project being analyzed. Logging this data can lead to the exposure of intellectual property or other sensitive information if the IDE logs are shared for debugging or support purposes.

// Wrap in LSP standard Content-Length headers
String message = "Content-Length: " + lspMessageString.getBytes(StandardCharsets.UTF_8).length
+ "\r\n\r\n" + lspMessageString;
try {
dartToLspStream.write(message.getBytes(StandardCharsets.UTF_8));
dartToLspStream.flush();
} catch (IOException e) {
LOG.warn("Failed to write to dartToLspStream", e);
}
}
} catch (Exception e) {
LOG.warn("Failed to parse lspToDartStream", e);
// Ignore parse errors from non-json or unrelated traffic
}
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[CONCERN] Catching the generic Exception is overly broad and can hide unexpected runtime errors. It's better to catch more specific exceptions that you expect to handle, such as com.google.gson.JsonParseException, to make the error handling more explicit and robust. The log message is also a bit misleading, as it's parsing a response from the Dart Analysis Server, not reading from lspToDartStream.

Suggested change
} catch (Exception e) {
LOG.warn("Failed to parse lspToDartStream", e);
// Ignore parse errors from non-json or unrelated traffic
}
} catch (com.google.gson.JsonParseException e) {
LOG.warn("Failed to parse message from Dart Analysis Server", e);
// Ignore parse errors from non-json or unrelated traffic
}
References
  1. This change improves maintainability by making error handling more specific, which aligns with the goal of reducing "'clever' code that is hard to read" and improving overall code quality. (link)

}
};
dasService.addResponseListener(dartResponseListener);

// 2. Read lsp4ij requests and forward only hovers as `lsp.handle`
lspToDartThread = new Thread(() -> {
StringBuilder buffer = new StringBuilder();
try {
int b;
while ((b = lspToDartStream.read()) != -1) {
buffer.append((char) b);
// A very naive detection of the end of a JSON-RPC message.
// A robust implementation should parse Content-Length properly.
String content = buffer.toString();
if (content.endsWith("}")) {
int braceCount = 0;
int startIdx = content.indexOf("{");
if (startIdx != -1) {
for (int i = startIdx; i < content.length(); i++) {
if (content.charAt(i) == '{')
braceCount++;
else if (content.charAt(i) == '}')
braceCount--;
}
if (braceCount == 0 && content.substring(0, startIdx).contains("Content-Length")) {
String jsonPart = content.substring(startIdx);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[MUST-FIX] The current implementation for parsing LSP messages is not robust. As noted in the code comment, it's a naive implementation that relies on checking for a closing brace and counting braces. This approach is brittle and can easily fail if:

  • The JSON payload contains a } character within a string value.
  • Multiple messages arrive in a single read, or a single message arrives in multiple reads.

A correct implementation must adhere to the Language Server Protocol specification by:

  1. Reading the Content-Length: NNN\r\n\r\n header.
  2. Reading exactly NNN bytes for the JSON content.

While this is a proof-of-concept, this is a fundamental correctness issue that should be addressed before this approach is built upon further.

References
  1. This is a logical bug that can lead to incorrect behavior and instability, which falls under the [MUST-FIX] severity category. (link)

try {
JsonObject jsonObject = JsonParser.parseString(jsonPart).getAsJsonObject();
String method = jsonObject.has("method") ? jsonObject.get("method").getAsString() : null;

// Fake an initialize response so lsp4ij knows hover is supported
if ("initialize".equals(method)) {
String id = jsonObject.get("id").getAsString();
String fakeResponse = "{\"jsonrpc\":\"2.0\",\"id\":" + id
+ ",\"result\":{\"capabilities\":{\"hoverProvider\":true,\"completionProvider\":{\"resolveProvider\":false}}}}";
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The id field from an incoming JSON-RPC request is concatenated directly into a JSON response string without proper escaping or quoting, leading to a JSON injection vulnerability. An attacker could inject arbitrary keys into the JSON response, causing unexpected behavior. This issue is located within the start() method, which is also over 100 lines long, violating the repository style guide and making it difficult to read and maintain. Consider refactoring this method into smaller, focused private methods to improve readability and address the injection vulnerability.

String message = "Content-Length: " + fakeResponse.getBytes(StandardCharsets.UTF_8).length
+ "\r\n\r\n" + fakeResponse;
dartToLspStream.write(message.getBytes(StandardCharsets.UTF_8));
dartToLspStream.flush();
LOG.info("Sent fake initialize response to lsp4ij");
} else if ("textDocument/hover".equals(method)) {
LOG.info("Forwarding hover request to Dart server");
// Wrap the LSP request inside a legacy `lsp.handle` request
JsonObject legacyRequest = new JsonObject();
// Generate a unique ID for the legacy request wrapper
String legacyId = dasService.generateUniqueId();
legacyRequest.addProperty("id", legacyId);
legacyRequest.addProperty("method", "lsp.handle");

JsonObject params = new JsonObject();
params.add("lspMessage", jsonObject);
legacyRequest.add("params", params);

// Send down to the Dart server
dasService.sendLspMessage(legacyRequest);
} else {
LOG.info("Ignored lsp4ij request: " + method);
}
} catch (Exception e) {
LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The application logs raw LSP payloads and messages at INFO and WARN levels, which can expose sensitive information like source code or file paths, leading to intellectual property leakage if logs are shared. This logging occurs within a section where a raw new Thread() is created. It's a best practice in IntelliJ Platform development to use managed thread pools via ApplicationManager.getApplication().executeOnPooledThread() for proper thread lifecycle and resource management, especially when handling potentially sensitive data streams.

}
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[CONCERN] Similar to the other comment, catching the generic Exception is too broad. Please catch a more specific exception, like com.google.gson.JsonParseException, to make the error handling more precise and avoid masking other potential issues.

Suggested change
} catch (Exception e) {
LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e);
}
} catch (com.google.gson.JsonParseException e) {
LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e);
}
References
  1. This change improves maintainability by making error handling more specific, which aligns with the goal of reducing "'clever' code that is hard to read" and improving overall code quality. (link)

buffer.setLength(0); // clear buffer
}
}
}
}
} catch (IOException e) {
LOG.warn("Error reading from lspToDartStream", e);
}
}, "LspToDartForwarder");
lspToDartThread.setDaemon(true);
lspToDartThread.start();
}

@Override
public InputStream getInputStream() {
return lspInputStream;
}

@Override
public OutputStream getOutputStream() {
return lspOutputStream;
}

@Override
public void stop() {
if (dasService != null && dartResponseListener != null) {
dasService.removeResponseListener(dartResponseListener);
}
if (lspToDartThread != null) {
lspToDartThread.interrupt();
}
try {
if (dartToLspStream != null)
dartToLspStream.close();
if (lspOutputStream != null)
lspOutputStream.close();
} catch (IOException e) {
LOG.warn("Error closing pipeds streams", e);
}
}
};
}

@Override
public @NotNull LSPClientFeatures createClientFeatures() {
LSPClientFeatures features = new LSPClientFeatures();

// Enable hover explicitly so we can test lsp4ij
features.setHoverFeature(new LSPHoverFeature() {
@Override
public boolean isEnabled(@NotNull PsiFile file) {
return true;
}
});

// Disable completion so it doesn't shadow DartServerCompletionContributor
features.setCompletionFeature(new LSPCompletionFeature() {
@Override
public boolean isEnabled(@NotNull PsiFile file) {
return false;
}
});

// Disable diagnostics so they don't double-report with legacy Annotators
features.setDiagnosticFeature(new LSPDiagnosticFeature() {
@Override
public boolean isEnabled(@NotNull PsiFile file) {
return false;
}
});

// Disable formatting as we use the legacy DartStyleAction
features.setFormattingFeature(new LSPFormattingFeature() {
@Override
public boolean isEnabled(@NotNull PsiFile file) {
return false;
}
});

return features;
}
}
8 changes: 7 additions & 1 deletion third_party/src/main/resources/META-INF/plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<vendor url="https://google.com">Google</vendor>
<category>Languages</category>
<depends>com.intellij.modules.xml</depends>
<depends>com.redhat.devtools.lsp4ij</depends>
<depends optional="true" config-file="dart-yaml.xml">org.jetbrains.plugins.yaml</depends>
<depends optional="true" config-file="dart-copyright.xml">com.intellij.copyright</depends>

Expand All @@ -30,6 +31,11 @@
<extensionPoint name="completionTimerExtension" interface="com.jetbrains.lang.dart.ide.completion.DartCompletionTimerExtension" dynamic="true"/>
</extensionPoints>

<extensions defaultExtensionNs="com.redhat.devtools.lsp4ij">
<server id="Dart" factoryClass="com.jetbrains.lang.dart.analyzer.DartLanguageServerFactory"/>
<languageMapping language="Dart" serverId="Dart" languageId="dart"/>
</extensions>

<extensions defaultExtensionNs="com.intellij">
<moveFileHandler implementation="com.jetbrains.lang.dart.ide.refactoring.moveFile.DartServerMoveDartFileHandler"/>
<fileType name="Dart" extensions="dart" language="Dart" implementationClass="com.jetbrains.lang.dart.DartFileType" fieldName="INSTANCE"/>
Expand Down Expand Up @@ -62,7 +68,7 @@
<nonProjectFileWritingAccessExtension implementation="com.jetbrains.lang.dart.ide.DartWritingAccessProvider"/>
<spellchecker.support language="Dart" implementationClass="com.jetbrains.lang.dart.ide.spelling.DartSpellcheckingStrategy"/>
<spellchecker.bundledDictionaryProvider implementation="com.jetbrains.lang.dart.ide.spelling.DartBundledDictionaryProvider"/>
<lang.documentationProvider language="Dart" implementationClass="com.jetbrains.lang.dart.ide.documentation.DartDocumentationProvider"/>
<!-- <lang.documentationProvider language="Dart" implementationClass="com.jetbrains.lang.dart.ide.documentation.DartDocumentationProvider"/> -->
<lang.implementationTextSelectioner language="Dart"
implementationClass="com.jetbrains.lang.dart.ide.DartImplementationTextSelectioner"/>
<lang.formatter language="Dart" implementationClass="com.jetbrains.lang.dart.ide.formatter.DartFormattingModelBuilder"/>
Expand Down
Loading