Skip to content

Comments

[ty] Implemented support for "rename" language server feature#19551

Merged
dhruvmanila merged 3 commits intoastral-sh:mainfrom
UnboundVariable:rename
Aug 7, 2025
Merged

[ty] Implemented support for "rename" language server feature#19551
dhruvmanila merged 3 commits intoastral-sh:mainfrom
UnboundVariable:rename

Conversation

@UnboundVariable
Copy link
Collaborator

This PR adds support for the "rename" language server feature. It builds upon existing functionality used for "go to references".

The "rename" feature involves two language server requests. The first is a "prepare rename" request that determines whether renaming should be possible for the identifier at the current offset. The second is a "rename" request that returns a list of file ranges where the rename should be applied.

Care must be taken when attempting to rename symbols that span files, especially if the symbols are defined in files that are not part of the project. We don't want to modify code in the user's Python environment or in the vendored stub files.

I found a few bugs in the "go to references" feature when implementing "rename", and those bug fixes are included in this PR.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@AlexWaygood AlexWaygood removed their request for review July 25, 2025 07:05
@MichaReiser MichaReiser added server Related to the LSP server ty Multi-file analysis & type inference labels Jul 25, 2025
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great.

Do you know how editors handle renames if multiple LSPs provide the feature?

I'm asking because our go to definition implementation still has some "holes". Meaning, ty will miss renames where another LSP (that users might run side to side with ty) would do a proper rename. Or do editors merge the rename result from all LSPs (union vs taking the result from one LSP)?

If editors only pick the result from one LSP, it migth then be best to wait landing this feature and add an explicit client setting for users to opt in to our experimental renaming.

We're fine landing this if editors union the result.

@UnboundVariable
Copy link
Collaborator Author

UnboundVariable commented Jul 25, 2025

If editors only pick the result from one LSP, it migth then be best to wait landing this feature and add an explicit client setting for users to opt in to our experimental renaming.

Editors don't merge results from multiple language servers for the "rename" feature. There's really no good way to do it. Instead, they pick one language server and use its results.

We can certainly wait to land this feature if you're concerned about this. It's up to you.

It sounds like we'll be able to plug the "nonlocal" and "global" hole soon, so maybe it's best if we wait for that fix to land first.

@MichaReiser
Copy link
Member

Yeah, it might be good to keep this disable for the moment. We could also land this but not register the provider and @dhruvmanila could enable it once he added support for client settings.

@dhruvmanila
Copy link
Member

I can take this up now that we have proper client settings infrastructure in place.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@dhruvmanila dhruvmanila merged commit b005cdb into astral-sh:main Aug 7, 2025
38 checks passed
dcreager added a commit that referenced this pull request Aug 7, 2025
* origin/main:
  [ty] Implemented support for "rename" language server feature (#19551)
  [ty] Reduce size of member table (#19572)
  [ty] Move server capabilities creation (#19798)
  [ty] Repurpose `FunctionType.into_bound_method_type` to return `BoundMethodType` (#19793)
  [ty] Validate writes to `TypedDict` keys (#19782)
  [ty] Add support for using the test command emitted when a mdtest fails (#19794)
dhruvmanila added a commit that referenced this pull request Aug 7, 2025
## Summary

This PR is a follow-up from #19551
and adds a new `ty.experimental.rename` setting to conditionally
register for the rename capability. The complementary PR in ty VS Code
extension is astral-sh/ty-vscode#111.

This is done using dynamic registration after the settings have been
resolved. The experimental group is part of the global settings because
they're applied for all workspaces that are managed by the client.

## Test Plan

Add E2E tests.

In VS Code, with the following setting:
```json
{
	"ty.experimental.rename": "true",
	"python.languageServer": "None"
}
```

I get the relevant log entry:
```
2025-08-07 16:05:40.598709000 DEBUG client_response{id=3 method="client/registerCapability"}: Registered rename capability
```

And, I'm able to rename a symbol. Once I set it to `false`, then I can
see this log entry:

```
2025-08-07 16:08:39.027876000 DEBUG Rename capability is disabled in the client settings
```

And, I don't see the "Rename Symbol" open in the VS Code dropdown.


https://github.com/user-attachments/assets/501659df-ba96-4252-bf51-6f22acb4920b
dcreager added a commit that referenced this pull request Aug 7, 2025
* dcreager/bound-typevar: (24 commits)
  more comment fix
  this isn't true anymore
  fix inner function in overload implementation ecosystem error
  Update Rust toolchain to 1.89 (#19807)
  [ty] Add `ty.inlayHints.variableTypes` server option (#19780)
  synthetic typevars for type materializations are bound
  [ty] Add failing tests for tuple subclasses (#19803)
  [ty] Add `ty.experimental.rename` server setting (#19800)
  clippy
  make BoundTypeVarInstance interned
  [ty] Implemented support for "rename" language server feature (#19551)
  [ty] Reduce size of member table (#19572)
  [ty] Move server capabilities creation (#19798)
  separate types!
  tmp
  allow KnownInstance::TypeVar in type expressions
  bind typevar in Generic/Protocol base class reference
  [ty] Repurpose `FunctionType.into_bound_method_type` to return `BoundMethodType` (#19793)
  remove unneeded GenericContext::with_binding_context
  create KnownInstanceType::TypeVar for PEP 695 too
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Related to the LSP server ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants