Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix location for elided members #98

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Fix location for elided members #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47c0500
Fix source location for elided member
srchase Apr 18, 2023
e8bcb5d
Update src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollec…
srchase Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down
33 changes: 28 additions & 5 deletions src/main/java/software/amazon/smithy/lsp/ext/FileCachingCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down
18 changes: 8 additions & 10 deletions src/test/java/software/amazon/smithy/lsp/ext/SmithyProjectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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,
Expand Down