Fix location for elided members (smithy-lang#98)

* Fix source location for elided member * Update src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java Co-authored-by: Steven Yuan <s.yuan.all@gmail.com> --------- Co-authored-by: Steven Yuan <s.yuan.all@gmail.com>
disneystreaming · Apr 18, 2023 · e467548 · e467548
1 parent ce330ec
commit e467548
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 16 deletions.
diff --git a/build.gradle b/build.gradle
@@ -157,7 +157,7 @@ publishing {
 
 dependencies {
     implementation "org.eclipse.lsp4j:org.eclipse.lsp4j:0.14.0"
-    implementation "software.amazon.smithy:smithy-model:[1.27.0, 2.0["
+    implementation "software.amazon.smithy:smithy-model:[1.30.0, 2.0["
     implementation 'io.get-coursier:interface:1.0.4'
     implementation 'com.disneystreaming.smithy:smithytranslate-formatter-jvm-java-api:0.3.3'
 

diff --git a/src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java b/src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java
@@ -51,6 +51,7 @@ final class FileCachingCollector implements ShapeLocationCollector {
     private Map<String, ModelFile> fileCache;
     private Map<OperationShape, List<Shape>> operationsWithInlineInputOutputMap;
     private Map<ShapeId, List<MemberShape>> containerMembersMap;
+    private Map<ShapeId, ShapeId> membersToUpdateMap;
 
     @Override
     public Map<ShapeId, Location> collectDefinitionLocations(Model model) {
@@ -59,13 +60,16 @@ public Map<ShapeId, Location> collectDefinitionLocations(Model model) {
         this.fileCache = createModelFileCache(model);
         this.operationsWithInlineInputOutputMap = new HashMap<>();
         this.containerMembersMap = new HashMap<>();
+        this.membersToUpdateMap = new HashMap<>();
 
         for (ModelFile modelFile : this.fileCache.values()) {
             collectContainerShapeLocationsInModelFile(modelFile);
         }
 
         operationsWithInlineInputOutputMap.forEach((this::collectInlineInputOutputLocations));
         containerMembersMap.forEach(this::collectMemberLocations);
+        // Make final pass to set locations for mixed-in member locations that weren't available on first pass.
+        membersToUpdateMap.forEach(this::updateElidedMemberLocation);
         return this.locations;
     }
 
@@ -146,11 +150,23 @@ private void collectMemberLocations(ShapeId containerId, List<MemberShape> membe
             int memberShapeSourceLocationLine = memberShape.getSourceLocation().getLine();
 
             boolean isContainerInAnotherFile = !containerLocation.getUri().equals(getUri(modelFile.filename));
-            // If the member's source location matches the container location's starting line (with offset),
-            // the member is inherited from a mixin and not present in the model file.
-            boolean isMemberMixedIn = memberShapeSourceLocationLine == containerLocationRange.getStart().getLine() + 1;
-
-            if (isContainerInAnotherFile || isMemberMixedIn) {
+            // If the member's source location is within the container location range, it is being defined
+            // or re-defined there.
+            boolean isMemberDefinedInContainer =
+                    memberShapeSourceLocationLine >= containerLocationRange.getStart().getLine()
+                    && memberShapeSourceLocationLine <= containerLocationRange.getEnd().getLine();
+
+            // If the member has mixins, and was not defined within the container, use the mixin source location.
+            if (memberShape.getMixins().size() > 0 && !isMemberDefinedInContainer) {
+                ShapeId mixinSource = memberShape.getMixins().iterator().next();
+                // If the mixin source location has been determined, use its location now.
+                if (locations.containsKey(mixinSource)) {
+                    locations.put(memberShape.getId(), locations.get(mixinSource));
+                // If the mixin source location has not yet been determined, save to re-visit later.
+                } else {
+                    membersToUpdateMap.put(memberShape.getId(), mixinSource);
+                }
+            } else if (isContainerInAnotherFile) {
                 locations.put(memberShape.getId(), createInheritedMemberLocation(containerLocation));
                 // Otherwise, determine the correct location by trimming comments, empty lines and applied traits.
             } else {
@@ -187,6 +203,13 @@ private void collectMemberLocations(ShapeId containerId, List<MemberShape> membe
         }
     }
 
+    // If a mixed-in member is not redefined within its containing structure, set its location to the mixin member.
+    private void updateElidedMemberLocation(ShapeId member, ShapeId sourceMember) {
+        if (locations.containsKey(sourceMember)) {
+            locations.put(member, locations.get(sourceMember));
+        }
+    }
+
     // Use an empty range at the container's start since inherited members are not present in the model file.
     private static Location createInheritedMemberLocation(Location containerLocation) {
         Position startPosition = containerLocation.getRange().getStart();

diff --git a/src/test/java/software/amazon/smithy/lsp/ext/SmithyProjectTest.java b/src/test/java/software/amazon/smithy/lsp/ext/SmithyProjectTest.java
@@ -107,8 +107,8 @@ public void ignoresUnmodeledApplyStatements() throws Exception {
             // Member is unchanged by apply
             correctLocation(locationMap, "com.main#SomeOpInput$body", 14, 4, 14, 16);
 
-            // The mixed in member should have an empty position
-            correctLocation(locationMap, "com.main#SomeOpInput$isTest", 12, 0, 12, 0);
+            // The mixed in member should have the source location from the mixin.
+            correctLocation(locationMap, "com.main#SomeOpInput$isTest", 8, 4, 8, 19);
 
             // Structure shape unchanged by apply
             correctLocation(locationMap, "com.main#ArbitraryStructure", 25, 0, 27, 1);
@@ -199,12 +199,12 @@ public void definitionLocationsV2() throws Exception {
             correctLocation(locationMap, "com.foo#FalseInlinedReversedFooInput", 193, 0, 195, 1);
             correctLocation(locationMap, "com.foo#FalseInlinedReversedBarOutput", 197, 0, 199, 1);
 
-            // Elided members with empty ranges.
-            correctLocation(locationMap, "com.foo#ElidedUserInfo$id", 134, 0, 134, 0);
-            correctLocation(locationMap, "com.foo#ElidedGetUserFooInput$email", 143, 13, 143, 13);
-            correctLocation(locationMap, "com.foo#ElidedGetUserFooInput$status", 143, 13, 143, 13);
-            correctLocation(locationMap, "com.foo#ElidedGetUserBarOutput$email", 149, 14, 149, 14);
-            correctLocation(locationMap, "com.foo#ElidedGetUserBarOutput$id", 149, 14, 149, 14);
+            // Elided members from source mixin structure.
+            correctLocation(locationMap, "com.foo#ElidedUserInfo$id", 117, 4, 117, 14);
+            correctLocation(locationMap, "com.foo#ElidedGetUserFooInput$email", 114, 4, 114, 17);
+            correctLocation(locationMap, "com.foo#ElidedGetUserFooInput$status",  122, 4, 122, 18);
+            correctLocation(locationMap, "com.foo#ElidedGetUserBarOutput$email",  114, 4, 114, 17);
+            correctLocation(locationMap, "com.foo#ElidedGetUserBarOutput$id", 117, 4, 117, 14);
         }
     }
 
@@ -358,8 +358,6 @@ public void shapeIdFromLocationV2() throws Exception {
                     new Position(14,18)).get());
             assertEquals(ShapeId.from("com.foo#GetUser"), project.getShapeIdFromLocation(uri,
                     new Position(125,13)).get());
-            assertEquals(ShapeId.from("com.foo#GetUserFooInput$email"), project.getShapeIdFromLocation(uri,
-                    new Position(126,13)).get());
             assertEquals(ShapeId.from("com.foo#GetUserFooInput$optional"), project.getShapeIdFromLocation(uri,
                     new Position(127,14)).get());
             assertEquals(ShapeId.from("com.foo#GetUserBarOutput"), project.getShapeIdFromLocation(uri,