From fa28c21364771712770656276542e0aa8f6c2a11 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Tue, 5 May 2026 11:27:31 +0200 Subject: [PATCH 1/7] Fix race between sending/clearing diagnostics via `didChange` and `didDeleteFiles` by keeping track of deletion status in file facts --- .../ParametricTextDocumentService.java | 10 ++--- .../parametric/model/ParametricFileFacts.java | 44 ++++++++++++++----- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java index 59593b749..dca47f3b0 100644 --- a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java +++ b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java @@ -339,12 +339,10 @@ public void didClose(DidCloseTextDocumentParams params) { @Override public void didDeleteFiles(DeleteFilesParams params) { - exec.submit(() -> { - // if a file is deleted, and we were tracking it, we remove our diagnostics - for (var f : params.getFiles()) { - availableClient().publishDiagnostics(new PublishDiagnosticsParams(f.getUri(), List.of())); - } - }); + for (var f : params.getFiles()) { + var loc = Locations.toLoc(f.getUri()); + facts(loc).remove(loc); + } } private void triggerAnalyzer(TextDocumentItem doc, Duration delay) { diff --git a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java index 88c147c75..db6e5dcec 100644 --- a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java +++ b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java @@ -163,10 +163,18 @@ public void close(ISourceLocation file) { getFile(file).close(); } + public void remove(ISourceLocation file) { + var fact = files.remove(file); + if (fact != null) { + fact.remove(); + } + } + private interface FileFact { void invalidateAnalyzer(boolean isClosing); void invalidateBuilder(boolean isClosing); void close(); + void remove(); void calculateAnalyzer(CompletableFuture> tree, int version, Duration delay); void calculateBuilder(CompletableFuture> tree); void reportParseErrors(Versioned> messages); @@ -177,6 +185,7 @@ private interface FileFact { @SuppressWarnings("java:S3077") // Reads/writes to fields of this class happen sequentially private class ActualFileFact implements FileFact { private final ISourceLocation file; + private volatile boolean removed = false; // To replace old diagnostics when new diagnostics become available in a // thread-safe way, we need to atomically: (1) check if the version of @@ -237,17 +246,15 @@ public void close() { if ((aMessages.isEmpty() && bMessages.isEmpty()) || !URIResolverRegistry.getInstance().exists(file)) { // If there are no messages for this file or the file has been deleted, can we remove it // else VS Code comes back and we've dropped the messages in our internal data - remove(file); + ParametricFileFacts.this.remove(file); } }); } - private @Nullable FileFact remove(ISourceLocation file) { - var removed = files.remove(file.top()); - if (removed != null) { - removed.clearDiagnostics(); - } - return removed; + @Override + public void remove() { + this.removed = true; + clearDiagnostics(); } /** @@ -363,6 +370,10 @@ public void clearDiagnostics() { } private void sendDiagnostics() { + if (removed) { + logger.debug("Will not send diagnostics since the file has been removed"); + return; + } if (client == null) { logger.debug("Cannot send diagnostics since the client hasn't been registered yet"); return; @@ -383,9 +394,17 @@ private void sendDiagnostics() { "Sending {} diagnostic(s) for {} (parser: v{}; analyzer: v{}; builder: v{})", diagnostics.size(), file, fromParser.version(), fromAnalyzer.version(), fromBuilder.version()); - client.publishDiagnostics(new PublishDiagnosticsParams( - Locations.toUri(file).toString(), - diagnostics)); + var uri = Locations.toUri(file).toString(); + client.publishDiagnostics(new PublishDiagnosticsParams(uri, diagnostics)); + + // The file for which diagnostics have just been sent, may have been + // deleted concurrently (i.e., after the `removed` check at the + // start of this method, but before the `publishDiagnostics` call). + // The resulting race may have caused the send to have accidentally + // overwritten the clear, so an additional clear might be needed. + if (removed) { + client.publishDiagnostics(new PublishDiagnosticsParams(uri, Collections.emptyList())); + } } /** @@ -458,6 +477,11 @@ public void close() { // NOP } + @Override + public void remove() { + // NOP + } + @Override public void calculateAnalyzer(CompletableFuture> tree, int version, Duration delay) { // NOP From ebc7ba5313a390ba29b55589252bf649a3c34636 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Tue, 5 May 2026 11:27:54 +0200 Subject: [PATCH 2/7] Add check in UI tests to make sure that all error diagnostics have been cleared after each test --- rascal-vscode-extension/src/test/vscode-suite/utils.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/rascal-vscode-extension/src/test/vscode-suite/utils.ts b/rascal-vscode-extension/src/test/vscode-suite/utils.ts index 9a4a15840..044f1dfb9 100644 --- a/rascal-vscode-extension/src/test/vscode-suite/utils.ts +++ b/rascal-vscode-extension/src/test/vscode-suite/utils.ts @@ -29,7 +29,7 @@ import { assert, expect } from "chai"; import { stat, unlink } from "fs/promises"; import * as os from 'os'; import { env } from "process"; -import { BottomBarPanel, By, ContentAssist, EditorView, Key, Locator, TerminalView, TextEditor, VSBrowser, WebDriver, WebElement, WebElementCondition, Workbench, until } from "vscode-extension-tester"; +import { BottomBarPanel, By, ContentAssist, EditorView, Key, Locator, MarkerType, TerminalView, TextEditor, VSBrowser, WebDriver, WebElement, WebElementCondition, Workbench, until } from "vscode-extension-tester"; import path = require("path"); export async function sleep(ms: number) { @@ -237,6 +237,12 @@ export class IDEOperations { const center = await ignoreFails(new Workbench().openNotificationsCenter()); await ignoreFails(center?.clearAllNotifications()); await ignoreFails(center?.close()); + + // There should be no more error diagnostics + const bottomBar = new Workbench().getBottomBar(); + const problemsView = await bottomBar.openProblemsView(); + const allVisibleMarkers = await problemsView.getAllVisibleMarkers(MarkerType.Error); + expect(allVisibleMarkers.length, "Not all error diagnostics have been cleared").to.equal(0); } assertLineBecomes(editor: TextEditor, lineNumber: number, lineContents: string, msg: string, wait = Delays.verySlow) : Promise { From c9d3a0be12725158e43c8120ad91b05f8827bcee Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Tue, 5 May 2026 12:43:47 +0200 Subject: [PATCH 3/7] Remove type error at the end of one of the IDE UI tests --- rascal-vscode-extension/src/test/vscode-suite/ide.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/rascal-vscode-extension/src/test/vscode-suite/ide.test.ts b/rascal-vscode-extension/src/test/vscode-suite/ide.test.ts index 40ba7972b..20340921f 100644 --- a/rascal-vscode-extension/src/test/vscode-suite/ide.test.ts +++ b/rascal-vscode-extension/src/test/vscode-suite/ide.test.ts @@ -251,11 +251,17 @@ describe('IDE', function () { const importerEditor = await ide.openModule(TestWorkspace.importerFile); const importeeEditor = await ide.openModule(TestWorkspace.importeeFile); + // Add type error await importeeEditor.typeTextAt(3, 1, "public str foo;"); await ide.openModule(TestWorkspace.importerFile); - await ide.triggerTypeChecker(importerEditor, {waitForFinish : true}); await ide.hasErrorSquiggly(importerEditor); + + // Remove type error (absence of error diagnostics is checked as part of the `cleanup` call in `afterEach`) + await ide.openModule(TestWorkspace.importeeFile); + await importeeEditor.setTextAtLine(3, "public int foo;"); + await ide.openModule(TestWorkspace.importerFile); + await ide.triggerTypeChecker(importerEditor, {waitForFinish : true}); }); it("errors in manifest detected", async() => { From e8456b1a87d492641da259c33bfcab7e917c0368 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Tue, 5 May 2026 13:28:45 +0200 Subject: [PATCH 4/7] Revert open changes at the end of a few DSL UI tests --- .../src/test/vscode-suite/dsl.test.ts | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/rascal-vscode-extension/src/test/vscode-suite/dsl.test.ts b/rascal-vscode-extension/src/test/vscode-suite/dsl.test.ts index c455803ea..70ca92279 100644 --- a/rascal-vscode-extension/src/test/vscode-suite/dsl.test.ts +++ b/rascal-vscode-extension/src/test/vscode-suite/dsl.test.ts @@ -308,11 +308,16 @@ end it("completion works", async function() { const editor = await ide.openModule(TestWorkspace.picoFile); - await editor.setTextAtLine(6, " aa : natural;"); + try { + await editor.setTextAtLine(6, " aa : natural;"); - await editor.moveCursor(9, 4); - await bench.executeCommand("editor.action.triggerSuggest"); // 'completion', typically triggered with Ctrl+Space - await expectCompletions(driver, editor, ["a", "aa"]); + await editor.moveCursor(9, 4); + await bench.executeCommand("editor.action.triggerSuggest"); // 'completion', typically triggered with Ctrl+Space + await expectCompletions(driver, editor, ["a", "aa"]); + } + finally { + await ide.revertOpenChanges(); + } }); it("completion by trigger character works", async function() { @@ -320,9 +325,14 @@ end if (!errorRecovery) { this.skip(); } const editor = await ide.openModule(TestWorkspace.picoFile); - await editor.moveCursor(10, 10); - await editor.typeText(" x :="); - await expectCompletions(driver, editor, ["a", "b", "n", "x"]); + try { + await editor.moveCursor(10, 10); + await editor.typeText(" x :="); + await expectCompletions(driver, editor, ["a", "b", "n", "x"]); + } + finally { + await ide.revertOpenChanges(); + } }); it("serializes Rascal values as expected", async function() { From cc06628a0055c46ab558db0527ddf66c316ff4a4 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Tue, 5 May 2026 13:30:12 +0200 Subject: [PATCH 5/7] Remove unused import --- .../vscode/lsp/parametric/ParametricTextDocumentService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java index dca47f3b0..8affd51e5 100644 --- a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java +++ b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java @@ -95,7 +95,6 @@ import org.eclipse.lsp4j.PrepareRenameDefaultBehavior; import org.eclipse.lsp4j.PrepareRenameParams; import org.eclipse.lsp4j.PrepareRenameResult; -import org.eclipse.lsp4j.PublishDiagnosticsParams; import org.eclipse.lsp4j.Range; import org.eclipse.lsp4j.ReferenceParams; import org.eclipse.lsp4j.RenameFilesParams; From 6215006d468277707ec470e8979670cb7f7c797b Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Tue, 5 May 2026 13:48:10 +0200 Subject: [PATCH 6/7] Make `didDeleteFiles` resilient to deletes of files that aren't tracked --- .../parametric/ParametricTextDocumentService.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java index 8affd51e5..e13671f96 100644 --- a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java +++ b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java @@ -338,9 +338,20 @@ public void didClose(DidCloseTextDocumentParams params) { @Override public void didDeleteFiles(DeleteFilesParams params) { + // This method might also be called when files are deleted that aren't + // tracked by this text document service. "Special care" is needed to + // silently ignore such files (without throwing exceptions). for (var f : params.getFiles()) { var loc = Locations.toLoc(f.getUri()); - facts(loc).remove(loc); + // Special care: Cannot use method `language`, because it throws when `safeLanguage` returns an empty optional. + var language = safeLanguage(loc); + if (language.isPresent()) { + // Special care: Cannot use method `facts`, because it throws when `get` returns `null`. + var factsOrNull = facts.get(language.get()); + if (factsOrNull != null) { + factsOrNull.remove(loc); + } + } } } From 7cbd7881380ad204ccda46e2702f5808133fa01e Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 6 May 2026 08:42:26 +0200 Subject: [PATCH 7/7] Rename variable and remove unnecessary `this.` --- .../lsp/parametric/ParametricTextDocumentService.java | 6 +++--- .../vscode/lsp/parametric/model/ParametricFileFacts.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java index e13671f96..15e806327 100644 --- a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java +++ b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java @@ -347,9 +347,9 @@ public void didDeleteFiles(DeleteFilesParams params) { var language = safeLanguage(loc); if (language.isPresent()) { // Special care: Cannot use method `facts`, because it throws when `get` returns `null`. - var factsOrNull = facts.get(language.get()); - if (factsOrNull != null) { - factsOrNull.remove(loc); + var facts = this.facts.get(language.get()); + if (facts != null) { + facts.remove(loc); } } } diff --git a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java index db6e5dcec..d9c024d83 100644 --- a/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java +++ b/rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/model/ParametricFileFacts.java @@ -253,7 +253,7 @@ public void close() { @Override public void remove() { - this.removed = true; + removed = true; clearDiagnostics(); }