From 4a4e27fe3d1c9dc96b3383304675338691479285 Mon Sep 17 00:00:00 2001 From: Jakob Rathbauer Date: Mon, 3 Aug 2026 03:15:58 +0200 Subject: [PATCH] lsp: Refine jump to definition position --- .../main/vadl/lsp/AstFinderByPosition.java | 21 ---- .../vadl/lsp/VadlTextDocumentService.java | 95 +++++++++++++------ .../aliasRegister.snapshot | 52 +++++++++- .../isa.snapshot | 52 +++++++++- .../twoFiles.snapshot | 46 ++++++++- vadl-lsp/test/vadl/lsp/TestUtils.java | 15 ++- .../lsp/integration/GotoDefinitionTest.java | 15 ++- .../vadl/lsp/integration/IntegrationTest.java | 2 +- 8 files changed, 229 insertions(+), 69 deletions(-) diff --git a/vadl-lsp/main/vadl/lsp/AstFinderByPosition.java b/vadl-lsp/main/vadl/lsp/AstFinderByPosition.java index 3d7065be4..2ebbee273 100644 --- a/vadl-lsp/main/vadl/lsp/AstFinderByPosition.java +++ b/vadl-lsp/main/vadl/lsp/AstFinderByPosition.java @@ -53,27 +53,6 @@ public abstract class AstFinderByPosition extends RecursiveAstVi // - References to Model parameters (within the model body, i.e. Placeholders) do not have an // identifier nor a target - /** - * Finds an Identifier or IdentifierPath at the given source code position, and returns its - * target's location. - * - * @param path The source code file to search in - * @param position The position to search for (within the file identified by {@code path}) - * @return Null if no Identifier or IdentifierPath found at {@code position} or it has no target - */ - public static @Nullable SourceLocation findIdentifierTargetLocation( - Ast ast, Path path, SourceLocation.Position position) { - var identifier = findIdentifier(ast, path, position); - if (identifier == null) { - return null; - } - var target = identifier.target(); - if (target == null) { - return null; - } - return target.location(); - } - /** * Finds an Identifier or IdentifierPath at the given source code position. * diff --git a/vadl-lsp/main/vadl/lsp/VadlTextDocumentService.java b/vadl-lsp/main/vadl/lsp/VadlTextDocumentService.java index 9f41ac312..112773956 100644 --- a/vadl-lsp/main/vadl/lsp/VadlTextDocumentService.java +++ b/vadl-lsp/main/vadl/lsp/VadlTextDocumentService.java @@ -55,6 +55,8 @@ import vadl.ast.Ast; import vadl.ast.Frontend; import vadl.ast.VadlParser; +import vadl.ast.nodes.IdentifiableNode; +import vadl.ast.nodes.IsId; import vadl.error.Diagnostic.MsgType; import vadl.error.DiagnosticList; import vadl.utils.DiskVirtualFileSystem; @@ -137,35 +139,61 @@ public void didSave(DidSaveTextDocumentParams params) { } catch (DiagnosticList dl) { log.debug("UNABLE definition: Parser produced diagnostics instead of AST for {}", document.uri); - return definitionResult(null); + return emptyDefinitionResult(); } var position = document.calculateUtf8Position(params.getPosition(), false); - SourceLocation location = AstFinderByPosition.findIdentifierTargetLocation( + IsId identifier = AstFinderByPosition.findIdentifier( ast, toPath(document.uri), position ); - - if (location == null || location.path() == null) { - return definitionResult(null); + if (identifier == null) { + return emptyDefinitionResult(); + } + var target = identifier.target(); + if (target == null || target.location().path() == null) { + return emptyDefinitionResult(); } - var targetDocument = snapshots.getFileBasedDocument(toUri(location.path())); + var targetUri = toUri(Objects.requireNonNull(target.location().path())); + var targetDocument = snapshots.getFileBasedDocument(targetUri); if (targetDocument == null) { - log.debug("Unexpected: Definition target file {} does not exist", toUri(location.path())); - return definitionResult(null); + log.debug("Unexpected: Definition target file {} does not exist", targetUri); + return emptyDefinitionResult(); } - var lspLocation = new Location(targetDocument.uri, - targetDocument.calculateUtf16Range(location)); - return definitionResult(lspLocation); + // targetSelectionRange is the location to navigate to, whereas targetRange refers to the + // entire definition code (i.e. less important information) + var targetRange = targetDocument.calculateUtf16Range(target.location()); + Range targetSelectionRange = targetRange; // Fallback + if (target instanceof IdentifiableNode identifiableTarget) { + targetSelectionRange = targetDocument.calculateUtf16Range( + identifiableTarget.identifier().location()); + } + var originSelectionRange = document.calculateUtf16Range(identifier.location()); + + return definitionResult(targetDocument.uri, targetRange, targetSelectionRange, + originSelectionRange); }); } private Either, List> definitionResult( - @Nullable Location lspLocation) { - log.debug("<<- definition: {}", lspLocation); - return Either.forLeft(lspLocation != null ? List.of(lspLocation) : List.of()); + String targetUri, Range targetRange, Range targetSelectionRange, Range originSelectionRange) { + + if (!clientSupportsDefinitionLink()) { + var location = new Location(targetUri, targetSelectionRange); + log.debug("<<- definition: {}", location); + return Either.forLeft(List.of(location)); + } + + var locationLink = new LocationLink(targetUri, targetRange, targetSelectionRange, + originSelectionRange); + log.debug("<<- definition: {}", locationLink); + return Either.forRight(List.of(locationLink)); + } + + private Either, List> emptyDefinitionResult() { + return Either.forLeft(List.of()); } @Override @@ -220,15 +248,6 @@ public CompletableFuture hover(HoverParams params) { return result; } - private List getClientMarkupContent() { - var capabilities = server.params().getCapabilities().getTextDocument(); - if (capabilities == null || capabilities.getHover() == null - || capabilities.getHover().getContentFormat() == null) { - return List.of(); - } - return capabilities.getHover().getContentFormat(); - } - /** * Manages diagnostic publishing for a given document, incl. version checking, and * updating dependent documents. @@ -237,10 +256,7 @@ private List getClientMarkupContent() { * @param snapshots Must be fresh, i.e. not used in the VADL parser yet */ private void publishDiagnostics(Document document, LspSnapshotFileSystem snapshots) { - var capabilities = server.params().getCapabilities().getTextDocument(); - if (capabilities == null - || capabilities.getPublishDiagnostics() == null) { - // Don't push diagnostics if client doesn't support it + if (!clientSupportsPublishDiagnostics()) { return; } @@ -355,6 +371,31 @@ private Diagnostic buildLspDiagnostic(vadl.error.Diagnostic vadlDiagnostic, return lspDiagnostic; } + + private boolean clientSupportsDefinitionLink() { + var capabilities = server.params().getCapabilities().getTextDocument(); + if (capabilities == null || capabilities.getDefinition() == null + || capabilities.getDefinition().getLinkSupport() == null) { + return false; + } + return capabilities.getDefinition().getLinkSupport(); + } + + private List getClientMarkupContent() { + var capabilities = server.params().getCapabilities().getTextDocument(); + if (capabilities == null || capabilities.getHover() == null + || capabilities.getHover().getContentFormat() == null) { + return List.of(); + } + return capabilities.getHover().getContentFormat(); + } + + private boolean clientSupportsPublishDiagnostics() { + var capabilities = server.params().getCapabilities().getTextDocument(); + return capabilities != null && capabilities.getPublishDiagnostics() != null; + } + + private boolean documentVersionIsCurrent(Document document) { Document currentDocument = getDocument(document.uri); if (currentDocument == null) { diff --git a/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/aliasRegister.snapshot b/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/aliasRegister.snapshot index 9b1de5af6..839d669b6 100644 --- a/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/aliasRegister.snapshot +++ b/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/aliasRegister.snapshot @@ -19,9 +19,20 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: aliasR # % Requested data of input file input.vadl % - returned Goto Definition: Either [ - left = [Location [ - uri = ".../vadl.lsp.integration.GotoDefinitionTest/aliasRegister-input.vadl" - range = Range [ + left = null + right = [LocationLink [ + originSelectionRange = Range [ + start = Position [ + line = 7 + character = 19 + ] + end = Position [ + line = 7 + character = 24 + ] + ] + targetUri = ".../vadl.lsp.integration.GotoDefinitionTest/aliasRegister-input.vadl" + targetRange = Range [ start = Position [ line = 5 character = 2 @@ -31,8 +42,17 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: aliasR character = 29 ] ] + targetSelectionRange = Range [ + start = Position [ + line = 5 + character = 17 + ] + end = Position [ + line = 5 + character = 22 + ] + ] ]] - right = null ] - ... which looks like this: @@ -42,9 +62,31 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: aliasR register X : Bits<5> -> Bits<32> } application binary interface ABI for TEST = { - alias register hello = X(0) + alias register hello = X(0) + + return address = hello + } + ================ + ================ + // GOTO POSITION 8:23 - "hello" points to alias register definition + instruction set architecture TEST = { + register X : Bits<5> -> Bits<32> + } + application binary interface ABI for TEST = { + alias register hello = X(0) return address = hello } ================ + ================ + // GOTO POSITION 8:23 - "hello" points to alias register definition + instruction set architecture TEST = { + register X : Bits<5> -> Bits<32> + } + application binary interface ABI for TEST = { + alias register hello = X(0) + + return address = hello + } + ================ diff --git a/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/isa.snapshot b/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/isa.snapshot index c96f75636..76c898bab 100644 --- a/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/isa.snapshot +++ b/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/isa.snapshot @@ -19,9 +19,20 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: isa # % Requested data of input file input.vadl % - returned Goto Definition: Either [ - left = [Location [ - uri = ".../vadl.lsp.integration.GotoDefinitionTest/isa-input.vadl" - range = Range [ + left = null + right = [LocationLink [ + originSelectionRange = Range [ + start = Position [ + line = 4 + character = 37 + ] + end = Position [ + line = 4 + character = 41 + ] + ] + targetUri = ".../vadl.lsp.integration.GotoDefinitionTest/isa-input.vadl" + targetRange = Range [ start = Position [ line = 1 character = 0 @@ -31,14 +42,34 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: isa character = 1 ] ] + targetSelectionRange = Range [ + start = Position [ + line = 1 + character = 29 + ] + end = Position [ + line = 1 + character = 33 + ] + ] ]] - right = null ] - ... which looks like this: ================ // GOTO POSITION 5:40 - "TEST" points to isa definition - instruction set architecture TEST = { + instruction set architecture TEST = { + register X : Bits<5> -> Bits<32> + } + application binary interface ABI for TEST = { + alias register hello = X(0) + + return address = hello + } + ================ + ================ + // GOTO POSITION 5:40 - "TEST" points to isa definition + instruction set architecture TEST = { register X : Bits<5> -> Bits<32> } application binary interface ABI for TEST = { @@ -47,4 +78,15 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: isa return address = hello } ================ + ================ + // GOTO POSITION 5:40 - "TEST" points to isa definition + instruction set architecture TEST = { + register X : Bits<5> -> Bits<32> + } + application binary interface ABI for TEST = { + alias register hello = X(0) + + return address = hello + } + ================ diff --git a/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/twoFiles.snapshot b/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/twoFiles.snapshot index 0f0b2dc30..5ed75afd8 100644 --- a/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/twoFiles.snapshot +++ b/vadl-lsp/test/resources/snapshots/vadl.lsp.integration.GotoDefinitionTest/twoFiles.snapshot @@ -18,9 +18,20 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: twoFil # % Requested data of input file otherFile.vadl % - returned Goto Definition: Either [ - left = [Location [ - uri = ".../vadl.lsp.integration.GotoDefinitionTest/twoFiles-otherFile.vadl" - range = Range [ + left = null + right = [LocationLink [ + originSelectionRange = Range [ + start = Position [ + line = 4 + character = 27 + ] + end = Position [ + line = 4 + character = 30 + ] + ] + targetUri = ".../vadl.lsp.integration.GotoDefinitionTest/twoFiles-otherFile.vadl" + targetRange = Range [ start = Position [ line = 1 character = 2 @@ -30,15 +41,40 @@ Test method: vadl.lsp.integration.GotoDefinitionTest.mainTest, Test case: twoFil character = 19 ] ] + targetSelectionRange = Range [ + start = Position [ + line = 1 + character = 11 + ] + end = Position [ + line = 1 + character = 14 + ] + ] ]] - right = null ] - ... which looks like this: ================ instruction set architecture TEST = { - constant foo = 13 + constant foo = 13 + register X : Bits<5> -> Bits<32> + } + ================ + ================ + instruction set architecture TEST = { + constant foo = 13 register X : Bits<5> -> Bits<32> } ================ + ================ + // GOTO POSITION 5:29 - "foo" points to constant definition in other file + import "twoFiles-otherFile"::TEST + + application binary interface ABI for TEST = { + alias register hello = X(foo) + + return address = hello + } + ================ diff --git a/vadl-lsp/test/vadl/lsp/TestUtils.java b/vadl-lsp/test/vadl/lsp/TestUtils.java index 0344a10ad..ef14b2fc1 100644 --- a/vadl-lsp/test/vadl/lsp/TestUtils.java +++ b/vadl-lsp/test/vadl/lsp/TestUtils.java @@ -62,6 +62,19 @@ public static String showPositionInFile(Position position, String fileContent) { * @return given fileContent with range marked, formatted for use in a snapshot */ public static String showRangeInFile(Range range, String fileContent) { + return showRangeInFile(range, fileContent, "RANGE"); + } + + /** + * Displays a range marker within the given file content. + * + * @param range the lsp range (with start and end position) + * @param fileContent the entire content of the file which should contain range + * @param name the name of this range, used as part of the marker. Should be uppercase for + * aesthetic reasons. + * @return given fileContent with range marked, formatted for use in a snapshot + */ + public static String showRangeInFile(Range range, String fileContent, String name) { String[] lines = fileContent.split("\n", -1); boolean startOutOfBounds = true; boolean endOutOfBounds = true; @@ -88,7 +101,7 @@ public static String showRangeInFile(Range range, String fileContent) { startOutOfBounds = false; var line = lines[start.getLine()]; lines[start.getLine()] = line.substring(0, start.getCharacter()) - + "" + line.substring(start.getCharacter()); + + "<" + name + ">" + line.substring(start.getCharacter()); } return SEPARATOR_LINE + "\n " + String.join("\n ", lines) diff --git a/vadl-lsp/test/vadl/lsp/integration/GotoDefinitionTest.java b/vadl-lsp/test/vadl/lsp/integration/GotoDefinitionTest.java index f6c3b6798..cb7c71965 100644 --- a/vadl-lsp/test/vadl/lsp/integration/GotoDefinitionTest.java +++ b/vadl-lsp/test/vadl/lsp/integration/GotoDefinitionTest.java @@ -68,7 +68,7 @@ public void mainTest(String testCase) throws ExecutionException, InterruptedExce new TextDocumentIdentifier(inputUri), position )).get(); - var rangesInFiles = processResult(result, snapshot); + var rangesInFiles = processResult(result, input, snapshot); snapshot.add("returned Goto Definition", result); snapshot.add("... which looks like this", rangesInFiles); snapshot.verify(); @@ -76,7 +76,7 @@ public void mainTest(String testCase) throws ExecutionException, InterruptedExce private String processResult( Either, List> definitionResult, - TestSnapshot snapshot) { + String input, TestSnapshot snapshot) { List rangesInFiles = new ArrayList<>(); @@ -89,8 +89,15 @@ private String processResult( } else { for (var link : definitionResult.getRight()) { - rangesInFiles.add(TestUtils.showRangeInFile(link.getTargetRange(), - snapshot.getInputData(snapshot.getInputNameFromUri(link.getTargetUri())))); + var inputData = snapshot.getInputData(snapshot.getInputNameFromUri(link.getTargetUri())); + + rangesInFiles.add(TestUtils.showRangeInFile(link.getTargetSelectionRange(), inputData, + "TARGET SELECTION RANGE")); + rangesInFiles.add(TestUtils.showRangeInFile(link.getTargetRange(), inputData, + "TARGET RANGE")); + rangesInFiles.add(TestUtils.showRangeInFile(link.getOriginSelectionRange(), input, + "ORIGIN SELECTION RANGE")); + link.setTargetUri(TestUtils.normalizeUri(link.getTargetUri())); } } diff --git a/vadl-lsp/test/vadl/lsp/integration/IntegrationTest.java b/vadl-lsp/test/vadl/lsp/integration/IntegrationTest.java index d387ec2a1..45394e91d 100644 --- a/vadl-lsp/test/vadl/lsp/integration/IntegrationTest.java +++ b/vadl-lsp/test/vadl/lsp/integration/IntegrationTest.java @@ -59,7 +59,7 @@ public void createServer() throws ExecutionException, InterruptedException { // Client Capabilities - these may need to be adjusted if new features are added to the server var textDocumentCapabilities = new TextDocumentClientCapabilities(); - textDocumentCapabilities.setDefinition(new DefinitionCapabilities()); + textDocumentCapabilities.setDefinition(new DefinitionCapabilities(false, true)); textDocumentCapabilities.setHover(new HoverCapabilities( List.of(MarkupKind.MARKDOWN, MarkupKind.PLAINTEXT), false)); textDocumentCapabilities.setPublishDiagnostics(new PublishDiagnosticsCapabilities());