From 47c0500a151658034bb0c61f7c58b45c6437866c Mon Sep 17 00:00:00 2001 From: Chase Coalwell <782571+srchase@users.noreply.github.com> Date: Tue, 18 Apr 2023 12:47:28 -0600 Subject: [PATCH 1/2] Fix source location for elided member --- build.gradle | 2 +- .../smithy/lsp/ext/FileCachingCollector.java | 33 ++++++++++++++++--- .../smithy/lsp/ext/SmithyProjectTest.java | 18 +++++----- 3 files changed, 37 insertions(+), 16 deletions(-) diff --git a/build.gradle b/build.gradle index 0fbba5eb..0e33051e 100644 --- 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 index 5c992f8e..b7577bea 100644 --- 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 fileCache; private Map> operationsWithInlineInputOutputMap; private Map> containerMembersMap; + private Map membersToUpdateMap; @Override public Map collectDefinitionLocations(Model model) { @@ -59,6 +60,7 @@ public Map 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); @@ -66,6 +68,8 @@ public Map collectDefinitionLocations(Model model) { 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 past. + membersToUpdateMap.forEach(this::updateElidedMemberLocation); return this.locations; } @@ -146,11 +150,23 @@ private void collectMemberLocations(ShapeId containerId, List 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 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 index cf9f95d8..3022abda 100644 --- 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, From e8bcb5d8994537285085e9173bf927226dad8360 Mon Sep 17 00:00:00 2001 From: Chase Coalwell <782571+srchase@users.noreply.github.com> Date: Tue, 18 Apr 2023 15:31:02 -0600 Subject: [PATCH 2/2] Update src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java Co-authored-by: Steven Yuan --- .../software/amazon/smithy/lsp/ext/FileCachingCollector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java b/src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java index b7577bea..a814f4ec 100644 --- a/src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java +++ b/src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java @@ -68,7 +68,7 @@ public Map collectDefinitionLocations(Model model) { 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 past. + // 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; }