fix: Trying to integrate diagnostics into bulk code inspection results (issue 476) #707

SCWells72 · 2024-12-16T19:21:51Z

This is not yet complete, but I'm opening a draft PR to discuss what needs to be done to complete it properly. I'll add questions in the PR itself.

…ded in bulk inspection runs. It doesn't totally work yet, primarily because diagnostics are received via notifications. We either need to pull them or wait for those publishDiagnostics notifications.

SCWells72 · 2024-12-16T19:23:13Z

src/main/java/com/redhat/devtools/lsp4ij/LanguageServerWrapper.java

@@ -635,8 +635,8 @@ public boolean canOperate(Project project) {
     *                 by using a blocking read action.
     * @return the completable future with the language server instance or null.
     */
-    CompletableFuture<@Nullable LanguageServer> connect(@NotNull VirtualFile file,
-                                                        @Nullable Document document) {
+    public CompletableFuture<@Nullable LanguageServer> connect(@NotNull VirtualFile file,


Had to make this public so that I could connect files that have not ever been opened in editor tabs to the respective language servers. See LSPDiagnosticAnnotator#connectIfNecessary() for more details.

If I understand correctly your code, you need to call connect for files which are not opened, right?

If it that we need to find an another way because connect call LSP didOpen notification which means "the file is opened in an editor".

Yeah, this approach is a dead end. I tried to add support for pull diagnostics instead as that would do exactly what we need, but unfortunately that capability is not very well supported, at least among the language servers that I tried. The TypeScript server doesn't support it, and the CSS server said it did but then threw an exception when it received the request.

I think this overall feature may need to wait until pull diagnostics are more broadly supported.

SCWells72 · 2024-12-16T19:24:17Z

...in/java/com/redhat/devtools/lsp4ij/features/codeAction/LSPLazyCodeActionIntentionAction.java

@@ -62,13 +62,13 @@ public void setLazyCodeActions(LSPLazyCodeActionProvider lazyCodeActions) {
    @Override
    public @IntentionName @NotNull String getText() {
        loadCodeActionIfNeeded();
-        return title;
+        return title != null ? title : "";


These are annotated as @NotNull but I was seeing runtime errors where they could be null. I've made them defensive, but perhaps these intentions should not ever be created if they don't have sufficient info to be used?

SCWells72 · 2024-12-16T19:24:54Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

    private static final Key<Boolean> APPLIED_KEY = Key.create("lsp.diagnostic.annotator.applied");

+    private final boolean forBulkInspection;


This allows the LSPDiagnosticAnnotatorInspection to create an instance of the annotator that knows that it's running as part of a bulk/batch inspection run.

SCWells72 · 2024-12-16T19:25:38Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

    }

    @Nullable
    @Override
-    public Boolean collectInformation(@NotNull PsiFile file, @NotNull Editor editor, boolean hasErrors) {
+    public Boolean collectInformation(@NotNull PsiFile file) {


This missing signature of collectInformation() needs to be present as it's called during bulk/batch inspection runs. The original just delegates to this one with the same implementation as before.

SCWells72 · 2024-12-16T19:26:09Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

@@ -66,28 +91,86 @@ public void doApply(@NotNull PsiFile file, Boolean applyAnnotator, @NotNull Anno
            return;
        }
        URI fileUri = LSPIJUtils.toUri(file);
-        Document document = LSPIJUtils.getDocument(file.getVirtualFile());
+        VirtualFile virtualFile = file.getVirtualFile();


Needed the VirtualFile as well as the Document, and added defensive null checks for both before proceeding.

SCWells72 · 2024-12-16T19:27:55Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

-                for (Diagnostic diagnostic : ds.getDiagnostics()) {
-                    ProgressManager.checkCanceled();
-                    createAnnotation(diagnostic, document, file, ds, holder);
+            // TODO: Is there not a way to check whether the server is valid for this file here?


I noticed that LSPDiagnosticFeature#isSupported() always returns true, so this block is being executed for language servers that don't support the file. Is there some way to check whether the language server supports the file, e.g., based on its mapping configuration?

SCWells72 · 2024-12-16T19:28:39Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

-                    createAnnotation(diagnostic, document, file, ds, holder);
+            // TODO: Is there not a way to check whether the server is valid for this file here?
+            // Force a connection for batch inspection
+            boolean needsDisconnect = forBulkInspection && connectIfNecessary(ls, file);


In a bulk inspection run, connect the file to the language server if not already connected and note that we did so so that we can disconnect the file when we're finished with it.

SCWells72 · 2024-12-16T19:31:10Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

+                        // The file is mapped with the current language server
+                        var ds = data.getDiagnosticsForServer();
+                        // Loop for LSP diagnostics to transform it to Intellij annotation.
+                        // TODO: When running for bulk inspection, we need to wait for diagnostics here; not sure how to


This is the main remaining issue...diagnostics are not guaranteed to be populated by the time getDiagnosticsForServer() returns. Is there a way to pull diagnostics explcitly? If not, is there a (good) way to wait for the diagnostics to be published? We really need them sychronously during bulk inspection runs.

It looks like we just need to implement a client for TextDocumentService#diagnostic() to compute/retrieve diagnostics synchronously in this situation.

SCWells72 · 2024-12-16T19:32:03Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

    }

    private static void createAnnotation(@NotNull Diagnostic diagnostic,
                                         @NotNull Document document,
-                                         @Nullable PsiFile file,
+                                         @NotNull PsiFile file,


All existing callers of this had already guaranteed a non-null file, so I changed the parameter nullability annotation accordingly since the code is using it as if it were non-null anyway.

SCWells72 · 2024-12-16T19:32:58Z

...n/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotatorInspection.java

+    public ProblemDescriptor @Nullable [] checkFile(@NotNull PsiFile file,
+                                                    @NotNull InspectionManager manager,
+                                                    boolean isOnTheFly) {
+        if (!isOnTheFly) {


Only call the annotator when invoked for a bulk inspection run.

SCWells72 · 2024-12-16T19:33:55Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticHandler.java

@@ -93,7 +94,10 @@ private void updateDiagnostics(@NotNull PublishDiagnosticsParams params, @NotNul
            // Trigger Intellij validation to execute
            // {@link LSPDiagnosticAnnotator}.
            // which translates LSP Diagnostics into Intellij Annotation
-            DaemonCodeAnalyzer.getInstance(project).restart(psiFile);
+            Editor editor = LSPIJUtils.editorForElement(psiFile);


No need to restart the analyzer if the file isn't in an editor tab, and that will be the case for many files during a bulk inspection run if we stick with reacting to published diagnostic notifications.

SCWells72 · 2024-12-16T19:34:18Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticsForServer.java

@@ -77,7 +77,9 @@ private Map<Diagnostic, LSPLazyCodeActions> toMap(List<Diagnostic> diagnostics,
        List<Diagnostic> sortedDiagnostics = diagnostics
                .stream()
                .sorted((d1, d2) -> {
-                    if (Ranges.containsRange(d1.getRange(), d2.getRange())) {
+                    if (Objects.equals(d1.getRange(), d2.getRange())) {


This comparator didn't have any logic for equivalent ranges, so I added it.

SCWells72 · 2024-12-16T19:34:40Z

src/main/resources/META-INF/plugin.xml

@@ -249,6 +249,17 @@ L
                id="LSPDiagnosticAnnotator"
                language=""
                implementationClass="com.redhat.devtools.lsp4ij.features.diagnostics.LSPDiagnosticAnnotator"/>
+        <localInspection


This is the inspection-based adapter for the external annotator.

SCWells72 · 2024-12-16T19:44:24Z

src/main/java/com/redhat/devtools/lsp4ij/features/diagnostics/LSPDiagnosticAnnotator.java

+                }
+            } finally {
+                if (needsDisconnect) {
+                    ls.disconnect(fileUri, false);


I don't think we can/should disconnect until we know that we have diagnostics for the file, but assuming we add some kind of synchronous wait for them above, this should still be what want to release resources that were only needed for the batch run.

SCWells72 · 2024-12-16T21:10:05Z

Closing because it's not really possible -- at least in any efficient manner -- without pull diagnostics, and pull diagnostics aren't supported well enough by existing language servers yet.

Issue 476 - This is an initial attempt to get diagnostics to be inclu…

864cd41

…ded in bulk inspection runs. It doesn't totally work yet, primarily because diagnostics are received via notifications. We either need to pull them or wait for those publishDiagnostics notifications.

SCWells72 marked this pull request as draft December 16, 2024 19:21

openshift-ci bot added the do-not-merge/work-in-progress label Dec 16, 2024

SCWells72 commented Dec 16, 2024

View reviewed changes

SCWells72 closed this Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Trying to integrate diagnostics into bulk code inspection results (issue 476) #707

fix: Trying to integrate diagnostics into bulk code inspection results (issue 476) #707

SCWells72 commented Dec 16, 2024

SCWells72 Dec 16, 2024

angelozerr Dec 16, 2024 •

edited

Loading

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 Dec 16, 2024

SCWells72 commented Dec 16, 2024

		private static final Key<Boolean> APPLIED_KEY = Key.create("lsp.diagnostic.annotator.applied");

		private final boolean forBulkInspection;

fix: Trying to integrate diagnostics into bulk code inspection results (issue 476) #707

fix: Trying to integrate diagnostics into bulk code inspection results (issue 476) #707

Conversation

SCWells72 commented Dec 16, 2024

Choose a reason for hiding this comment

angelozerr Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 commented Dec 16, 2024

angelozerr Dec 16, 2024 •

edited

Loading