Determine end of definition location #35

srchase · 2022-04-15T20:54:52Z

Currently, the source location of a shape is used to set both the start and end positions of a location range when building the map of definition locations used by the definition provider.

This CR improves the definition locations by finding the appropriate end position of a shape. By working from the bottom of a file to the top, the real end of a shape definition can be found by ignoring the traits and comments of the shape beneath it in a file.

This also adds a getShapeIdFromLocation method which returns the shapeId of the shape defined at the given location within the model.

With improved definition locations, clients can render more accurate definitions.

In VS Code, the Peek Definition pane currently shows the following definition for a shape:

With the improved definition location, the entire shape is highlighted:

When using the "Go to Definition" context option, the cursor still moves to the beginning of the shape. With this CR, when selecting "Go to Definition" when the cursor is already within a shape, the cursor will no longer move. Currently, the cursor moves to the beginning of the shape definition due to the definition location being a zero-length location range that precedes the actual shape definition.

This CR also cleans up the unused reload method in SmithyProject.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java

kstich · 2022-04-15T21:37:32Z

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java

+        String shapeName = shape.getId().getName();
+        // Members get the same shapeName as their parent structure
+        // so we ignore them, to avoid producing a location per-member
+        // TODO: index members somehow as well?


This seems useful, since apply syntax allows for targeting a member externally. Maybe the location map should use the ShapeId as the key instead of the name?

When finding a token in a line, SmithyTextDocumentService only returns a potential shape name. We could use the shape name to try to find the appropriate ShapeId and then change over the location map to use the ShapeId as the key.

kstich · 2022-04-15T21:57:36Z

src/test/java/software/amazon/smithy/lsp/ext/SmithyProjectTest.java

+        try (Harness hs = Harness.create(SmithyBuildExtensions.builder().build(), modelFiles)) {
+            Map<String, List<Location>> locations = hs.getProject().getLocations();
+
+            correctLocation(locations, "SingleLine", 4, 5);


Seems a little strange that the starts for this are the line before, worth seeing if that's necessary to work properly.

Positions for LSP are zero-based, so this is currently setting the end position to the start of the next line.

I'll add logic to properly determine the line length so we can determine the actual end of a shape on a given line.

src/test/resources/software/amazon/smithy/lsp/ext/models/main.smithy

Baccata · 2022-04-23T10:25:20Z

When using the "Go to Definition" context option, the cursor still moves to the beginning of the shape. With this CR, when selecting "Go to Definition" when the cursor is already within a shape, the cursor will no longer move. Currently, the cursor moves to the beginning of the shape definition due to the definition location being a zero-length location range that precedes the actual shape definition.

I've not read the code so bear with me, but could use these more precise locations to actually determine the actual shape the cursor is hovering over ? Go to Definition suffers from not having this information, as it does a naive extraction of the underlying string, then gets you the first shape the name of which matches that string. If we had a Location => Shape method, when the cursor is on a member shape, we could jump to the actual targeted shape. There's also a number of additional LSP endpoints that could be implemented by virtue of having this information.

srchase · 2022-04-25T21:18:01Z

When using the "Go to Definition" context option, the cursor still moves to the beginning of the shape. With this CR, when selecting "Go to Definition" when the cursor is already within a shape, the cursor will no longer move. Currently, the cursor moves to the beginning of the shape definition due to the definition location being a zero-length location range that precedes the actual shape definition.

I've not read the code so bear with me, but could use these more precise locations to actually determine the actual shape the cursor is hovering over ? Go to Definition suffers from not having this information, as it does a naive extraction of the underlying string, then gets you the first shape the name of which matches that string. If we had a Location => Shape method, when the cursor is on a member shape, we could jump to the actual targeted shape. There's also a number of additional LSP endpoints that could be implemented by virtue of having this information.

Yes, the member's location could be used to determine its actual definition which could then allow for jumping to its target. One nice side-effect of the current naive extraction is that tokens can be pulled out of line docs and you can still jump to the definition.
Which other LSP endpoints did you have in mind for being able to use a Location => Shape method?

Baccata · 2022-04-28T07:49:36Z

Which other LSP endpoints did you have in mind for being able to use a Location => Shape method?

I was thinking things along the lines of :

find references
rename symbol

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java

kstich · 2022-04-28T21:02:51Z

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java

+    private static String getUri(String fileName) {
+        return Utils.isJarFile(fileName)
+                ? Utils.toSmithyJarFile(fileName)
+                : !fileName.startsWith("file:") ? "file:" + fileName


Consider breaking this nested ternary out for readability.

kstich · 2022-04-28T21:14:37Z

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java

+        return Optional.ofNullable(matchingShapes.get(0).getKey());
+    }
+
+    private Predicate<Map.Entry<ShapeId, Location>> filterByLocation(Position position) {


This seems like it should be a Predicate<Location> instead, since it doesn't use the key at all.

I agree this looks a bit awkward, but the key (the shapeId) is what needs to be carried forward. If I mapped to a location first, I'd lose the reference to the shapeId. I did simplify this a bit by mapping to a shapeId in the stream after filtering.

kstich · 2022-04-28T21:32:27Z

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java


+                // Find the end of a member's location by first trimming trailing commas, empty lines and closing
+                // structure braces.
+                if (shape.getType() == ShapeType.MEMBER) {


Is it reasonable to break these branches up in to their own methods for readability?

I broke out the non-member branch since it only updates the end marker, but the non-member branch is responsible for updating two separate markers and the end position of a member shape. I think it made readability worse for separate method to do all of that.

kstich · 2022-04-28T21:39:06Z

src/main/java/software/amazon/smithy/lsp/SmithyTextDocumentService.java

+                // Use location on target, or else default to initial shape.
+                locations = Collections.singletonList(project.getLocations().get(target.orElse(initialShapeId.get())));
+            } else {
+                // If cursor location doesn't correspond to definition, return locations of all shapes that match token.


Not sure what this comment means.

kstich · 2022-04-28T21:39:18Z

src/main/java/software/amazon/smithy/lsp/SmithyTextDocumentService.java

@@ -254,8 +262,30 @@ private String findToken(String path, Position p) throws IOException {
    public CompletableFuture<Either<List<? extends Location>, List<? extends LocationLink>>> definition(
            DefinitionParams params) {
        try {
+            List<Location> locations;


It would be nice to have a description of what this is doing behaviorally.

Added with an example.

1.22.0 not yet released.

Determine end of definition location

e0579d0

srchase requested a review from a team as a code owner April 15, 2022 20:54

kstich requested changes Apr 15, 2022

View reviewed changes

srchase mentioned this pull request Apr 15, 2022

Implement comparable, add static compare method smithy-lang/smithy#1192

Merged

Add TODOs for using Comparator, update jar loading

40993b8

srchase force-pushed the improve-definition-locations branch from be2e5d7 to c288641 Compare April 19, 2022 15:28

Fix jar file loading on Windows

16893c0

srchase force-pushed the improve-definition-locations branch from c288641 to 16893c0 Compare April 19, 2022 16:05

Set end position to same line, expand test files

9444923

srchase force-pushed the improve-definition-locations branch from 7aa59d0 to 9444923 Compare April 22, 2022 20:03

Chase Coalwell added 2 commits April 25, 2022 15:48

Fix loading from jars

c8d1ed6

Use ShapeId for Locations map

aefd638

JordonPhillips previously requested changes Apr 28, 2022

View reviewed changes

src/main/java/software/amazon/smithy/lsp/ext/SmithyProject.java Show resolved Hide resolved

srchase force-pushed the improve-definition-locations branch 2 times, most recently from 0b29f1c to 29b4070 Compare April 28, 2022 14:40

Fix Windows uri comparison

a8405e4

srchase force-pushed the improve-definition-locations branch from 29b4070 to a8405e4 Compare April 28, 2022 17:45

kstich reviewed Apr 28, 2022

View reviewed changes

Chase Coalwell added 2 commits April 29, 2022 09:32

Address PR comments

98f382b

Simplify position filtering helper

b0657d4

kstich approved these changes Apr 29, 2022

View reviewed changes

srchase merged commit 6cd1f26 into smithy-lang:main Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Determine end of definition location #35

Determine end of definition location #35

srchase commented Apr 15, 2022 •

edited

kstich Apr 15, 2022

srchase Apr 18, 2022

kstich Apr 15, 2022

srchase Apr 19, 2022

Baccata commented Apr 23, 2022

srchase commented Apr 25, 2022

Baccata commented Apr 28, 2022

kstich Apr 28, 2022

kstich Apr 28, 2022

srchase Apr 29, 2022

kstich Apr 28, 2022

srchase Apr 29, 2022

kstich Apr 28, 2022

kstich Apr 28, 2022

srchase Apr 29, 2022

Determine end of definition location #35

Determine end of definition location #35

Conversation

srchase commented Apr 15, 2022 • edited

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

Baccata commented Apr 23, 2022

srchase commented Apr 25, 2022

Baccata commented Apr 28, 2022

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

srchase commented Apr 15, 2022 •

edited