Implement references and rename #213

milesziemer · 2025-03-26T19:23:59Z

Adds support for 'textDocument/references', 'textDocument/rename', and
'textDocument/prepareRename'. These all work very similarly, so I
implemented them together.

This implementation only works for root shapes, not members or resource
identifiers/properties (which I call resource fields for brevity).
Originally I intended to make it work for members, but for 'complete'
member support, you really need to support resource fields, otherwise if
you tried to rename a member, it wouldn't rename the resource field it
references, and finding references wouldn't show fully accurate
information. The problem is, the relationship between members and
resource fields can be implicit, and/or determined by a number of
different traits (resourceIdentifier, property, references,
nestedProperties). And some of these traits refer to the resource
field by string name. Current KnowledgeIndex implementations don't
support going from an arbitrary member -> field bindings and back. Plus,
we can't reliably use KnowledgeIndicies because they usually operate on
a complete, valid model, whereas the language server wants to operate on
the minimum subset of a possibly invalid model.

I think it would be possible still to support members in the future,
either with some clever code in the language server or enhancements to
the smithy library, but I didn't want to block rename/references for
regular shapes on that.

Some specific implementation details to note:

Rename/References throw ResponseErrorException on an unsupported
position (like a member name), or when the rename would be invalid
(i.e. renaming a shape from a dependency, or providing an invalid
shape name). This is what the spec says you should do, and we should
update other features to do the same.
Rename disambiguates shapes that would be conflicting after the rename
by using an absolute shape id, and removes conflicting use statements.
Rename/References both work on references to members of the target
shape, i.e. com.foo#Foo$bar, only if the request position is before
the $, otherwise it assumes you're getting the reference of a
member.
Prepare returns the range of the identifier to be renamed (or throws
an error as specified in 1.), so if you go to rename com.foo#Bar,
the rename range will be the range of Bar.

Other notable changes:

Changed DocumentId to only have distinct types for root shapes and
members. The type will be ROOT when the cursor is in any part of
the shape id before a member, and MEMBER only when the cursor is
actually within the member part of the shape id. The rest of the
other possible id types didn't seem to be useful enough to keep.
Added a line field to Syntax.Node.Str, which makes it much
more efficient to get the range of a Str or Ident.
Fixed an issue in TextWithPositions when there are multiple positions
on the same line.

Rev: Fix shape search resolution

Fixes an issue with ShapeSearch::findShape where the local namespace was
checked before imports. It was inconsistent with the way the model
loader resolves shapes, so you could end up jumping to the wrong
definition/references, or renaming the wrong shape.

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

Adds support for 'textDocument/references', 'textDocument/rename', and 'textDocument/prepareRename'. These all work very similarly, so I implemented them together. This implementation only works for root shapes, not members or resource identifiers/properties (which I call resource fields for brevity). Originally I intended to make it work for members, but for 'complete' member support, you really need to support resource fields, otherwise if you tried to rename a member, it wouldn't rename the resource field it references, and finding references wouldn't show fully accurate information. The problem is, the relationship between members and resource fields can be implicit, and/or determined by a number of different traits (`resourceIdentifier`, `property`, `references`, `nestedProperties`). And some of these traits refer to the resource field by string name. Current KnowledgeIndex implementations don't support going from an arbitrary member -> field bindings and back. Plus, we can't reliably use KnowledgeIndicies because they usually operate on a complete, valid model, whereas the language server wants to operate on the minimum subset of a possibly invalid model. I think it would be possible still to support members in the future, either with some clever code in the language server or enhancements to the smithy library, but I didn't want to block rename/references for regular shapes on that. Some specific implementation details to note: 1. Rename/References throw ResponseErrorException on an unsupported position (like a member name), or when the rename would be invalid (i.e. renaming a shape from a dependency, or providing an invalid shape name). This is what the spec says you should do, and we should update other features to do the same. 2. Rename disambiguates shapes that would be conflicting after the rename by using an absolute shape id, and removes conflicting use statements. 3. Rename/References both work on references to members of the target shape, i.e. `com.foo#Foo$bar`, only if the request position is before the `$`, otherwise it assumes you're getting the reference of a member. 4. Prepare returns the range of the identifier to be renamed (or throws an error as specified in 1.), so if you go to rename `com.foo#Bar`, the rename range will be the range of `Bar`. Other notable changes: 1. Changed `DocumentId` to only have distinct types for root shapes and members. The type will be `ROOT` when the cursor is in any part of the shape id before a member, and `MEMBER` only when the cursor is actually within the member part of the shape id. The rest of the other possible id types didn't seem to be useful enough to keep. 2. Added a `line` field to Syntax.Node.Str, which makes it _much_ more efficient to get the range of a Str or Ident. 3. Fixed an issue in TextWithPositions when there are multiple positions on the same line.

Fixes an issue with ShapeSearch::findShape where the local namespace was checked before imports. It was inconsistent with the way the model loader resolves shapes, so you could end up jumping to the wrong definition/references, or renaming the wrong shape.

Test assertions weren't working properly with windows line separators

yefrig

Left some small comments but otherwise LGTM.

Also tested functionality by building this PR locally and using it in Neovim 0.12.

yefrig · 2025-03-27T15:12:17Z

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

+        LOGGER.finest("PrepareRename");
+        String uri = params.getTextDocument().getUri();
+        ProjectAndFile projectAndFile = state.findProjectAndFile(uri);
+        if (projectAndFile == null) {


would be nice if we could extract some of this logic to a helper function but not sure if it's worth the trouble given the different return types of each request method

Yea I agree. There's also a lot of duplicated setup code in each of the handlers, for example the ReferencesHandler.Config class which is shared with rename. I'm thinking I'll follow up with some changes to refactor all that

yefrig · 2025-03-27T16:50:18Z

src/main/java/software/amazon/smithy/lsp/language/ReferencesHandler.java

+        private static ResponseErrorException notSupported() {
+            var error = new ResponseError();
+            error.setCode(ResponseErrorCode.RequestFailed);
+            error.setMessage("Not supported here.");


Suggested change

error.setMessage("Not supported here.");

error.setMessage("References not supported here.");

yefrig · 2025-03-27T16:56:59Z

src/main/java/software/amazon/smithy/lsp/language/References.java

+        addPendingReferences();
+    }
+
+    private void collect(Syntax.Statement statement) {


I don't know if I like collect() since the function returns void and uses side effects

yefrig · 2025-03-27T17:11:05Z

src/main/java/software/amazon/smithy/lsp/language/RenameHandler.java

+            var conflictingShape = ShapeSearch.findShape(sourceFile.getParse(), conflictingId, config.model())
+                    .orElse(null);
+            if (conflictingShape == null) {
+                return;
+            }


can this be improved? it looks awkward lol but maybe Optional is not that flexible

yefrig · 2025-03-27T17:16:18Z

src/main/java/software/amazon/smithy/lsp/syntax/Syntax.java

         * identifiers, so this class a single subclass {@link Ident}.
         */
        public static sealed class Str extends Node {
+            final int line;


is this supposed to be line number?

Made this lineNumber

milesziemer added 2 commits March 26, 2025 13:29

Fix shape search resolution

2813db5

Fixes an issue with ShapeSearch::findShape where the local namespace was checked before imports. It was inconsistent with the way the model loader resolves shapes, so you could end up jumping to the wrong definition/references, or renaming the wrong shape.

milesziemer requested a review from a team as a code owner March 26, 2025 19:24

milesziemer requested review from sugmanue and yefrig and removed request for sugmanue March 26, 2025 19:24

milesziemer force-pushed the rename-4 branch from 86dcb9a to 2b03edf Compare March 26, 2025 19:37

Fix windows ci

95a56aa

Test assertions weren't working properly with windows line separators

milesziemer force-pushed the rename-4 branch from 2b03edf to 95a56aa Compare March 26, 2025 19:44

yefrig approved these changes Mar 27, 2025

View reviewed changes

Address comments

d13f418

milesziemer force-pushed the rename-4 branch from 70412f8 to d13f418 Compare March 27, 2025 19:19

milesziemer merged commit 21f0d5c into smithy-lang:main Mar 27, 2025
3 checks passed

This was referenced Apr 16, 2025

Support find-replace references #16

Closed

Add the ability to rename shapes #106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement references and rename #213

Implement references and rename #213

Uh oh!

milesziemer commented Mar 26, 2025

Uh oh!

yefrig left a comment •

edited

Loading

Uh oh!

yefrig Mar 27, 2025

Uh oh!

milesziemer Mar 27, 2025

Uh oh!

yefrig Mar 27, 2025

Uh oh!

milesziemer Mar 27, 2025

Uh oh!

yefrig Mar 27, 2025

Uh oh!

yefrig Mar 27, 2025

Uh oh!

yefrig Mar 27, 2025

Uh oh!

milesziemer Mar 27, 2025

Uh oh!

milesziemer Mar 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	error.setMessage("Not supported here.");
	error.setMessage("References not supported here.");

Implement references and rename #213

Implement references and rename #213

Uh oh!

Conversation

milesziemer commented Mar 26, 2025

Uh oh!

yefrig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yefrig left a comment •

edited

Loading