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 1c313e553..bd41e26ae 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; @@ -339,12 +338,21 @@ 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())); + // 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()); + // 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 facts = this.facts.get(language.get()); + if (facts != null) { + facts.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..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 @@ -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() { + 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 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() { 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() => { 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 {