lsp: Refine jump to definition position

vadl-lsp/main/vadl/lsp/AstFinderByPosition.java

@@ -53,27 +53,6 @@ public abstract class AstFinderByPosition<N extends Node> 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.
    *

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<? extends Location>, List<? extends LocationLink>> 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<? extends Location>, List<? extends LocationLink>> emptyDefinitionResult() {
+    return Either.forLeft(List.of());
   }
 
   @Override
@@ -220,15 +248,6 @@ public CompletableFuture<Hover> hover(HoverParams params) {
     return result;
   }
 
-  private List<String> 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<String> 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<String> 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) {

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 = {
-      <RANGE>alias register hello = X(0)</>
+      alias register <TARGET SELECTION RANGE>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 = {
+      <TARGET RANGE>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 = <ORIGIN SELECTION RANGE>hello</>
+    }
+  ================
 

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
-    <RANGE>instruction set architecture TEST = {
+    instruction set architecture <TARGET SELECTION RANGE>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
+    <TARGET RANGE>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 <ORIGIN SELECTION RANGE>TEST</> = {
+      alias register hello = X(0)
+
+      return address = hello
+    }
+  ================
 

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 = {
-      <RANGE>constant foo = 13</>
+      constant <TARGET SELECTION RANGE>foo</> = 13
+      register X : Bits<5> -> Bits<32>
+    }
+  ================
+  ================
+    instruction set architecture TEST = {
+      <TARGET RANGE>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(<ORIGIN SELECTION RANGE>foo</>)
+
+      return address = hello
+    }
+  ================