Skip to content

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented 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:

{
	"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.

Screen.Recording.2025-08-07.at.16.08.26.mov

@dhruvmanila dhruvmanila added configuration Related to settings and configuration server Related to the LSP server ty Multi-file analysis & type inference labels Aug 7, 2025
@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 ✅

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@dhruvmanila dhruvmanila marked this pull request as ready for review August 7, 2025 10:40
@dhruvmanila dhruvmanila requested review from UnboundVariable and removed request for carljm, dcreager and sharkdp August 7, 2025 10:40
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.

I think it's safe to assume that clients that don't support dynamic registration probably also don't support the configuration request.

Should we register the rename provider for those statically if the experimental rename feature is enabled in the initialization options? We should probably do the same for workspace diagnsotics unless we're already doing this.

Comment on lines 573 to +574
self.register_diagnostic_capability(client);
self.register_rename_capability(client);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't incorrect but the LSP supports sending multiple registrations in a single request. Should we change the regsiter_* to a register_dynamic_capability method that registers all dynamic capabilities (collects the one it must remove and then collects the one it needs to add)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wanted to do something like that but the difference here is that for the rename capability we do want to unregister the capability but then we should avoid re-registering the capability. This scenario is only valid when we support didChangeConfiguration but I don't want to make any assumptions that might be incorrect later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I could have two separate methods unregister_dynamic_capability and register_dynamic_capability which would solve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to do this as a follow-up.

@dhruvmanila
Copy link
Member Author

I think it's safe to assume that clients that don't support dynamic registration probably also don't support the configuration request.

I'm not sure if we can assume that as dynamic registrations are scoped to individual capabilities and workspace configuration is an independent capability. So, a client might have dynamic registration for one capability while it might not for another.

For workspace diagnostics, the server checks whether dynamic registration is supported. If not, it will statically register for the diagnostic capability during initialization but without looking at the ty.diagnosticMode value. It will always advertise as supporting workspace diagnostics.

We could do the same for the rename capability which is that we would check whether dynamic registration is supported for the rename capability and if it's not supported, statically register it during initialization. We could then check in the rename request handler whether the user has enabled this or not and return early if not.

Does this make sense?

@MichaReiser
Copy link
Member

We could do the same for the rename capability which is that we would check whether dynamic registration is supported for the rename capability and if it's not supported, statically register it during initialization. We could then check in the rename request handler whether the user has enabled this or not and return early if not.

I didn't see a meaningful value that the client could return in that case. That's why I suspect the best second solution is to only register the provider if the settings (resolved at this point) also enable it. It seems a better experience than one where users can't opt into the setting at all.

@dhruvmanila
Copy link
Member Author

I didn't see a meaningful value that the client could return in that case. That's why I suspect the best second solution is to only register the provider if the settings (resolved at this point) also enable it. It seems a better experience than one where users can't opt into the setting at all.

Ok, so are you saying that the server would check the resolved initialization options and check whether the client supports dynamic registration for the relevant capability (rename and diagnostic) and set the server capability according the relevant server setting (ty.experimental.rename and ty.diagnosticMode)?

Comment on lines +299 to +306
} else {
// Otherwise, we check whether user has enabled rename support via the resolved settings
// from initialization options.
global_settings
.is_rename_enabled()
.then(|| OneOf::Right(server_rename_options()))
};

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the behavior that was discussed in the review feedback.

@dhruvmanila dhruvmanila enabled auto-merge (squash) August 7, 2025 12:52
dhruvmanila added a commit to astral-sh/ty-vscode that referenced this pull request Aug 7, 2025
## Summary

This is a complementary PR to
astral-sh/ruff#19800 that adds the
`ty.experimental.rename` setting in the extension.

## Test Plan

Refer to the test plan in the PR.
@dhruvmanila dhruvmanila merged commit 7b6abfb into main Aug 7, 2025
37 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/experimental-rename branch August 7, 2025 12:54
dhruvmanila added a commit to astral-sh/ty that referenced this pull request Aug 7, 2025
## Summary

This PR adds reference documentation for the two new editor settings
that were added in astral-sh/ruff#19800 and
astral-sh/ruff#19780.

## Test Plan

<img width="2491" height="1374" alt="Screenshot 2025-08-07 at 19 24 44"
src="https://github.com/user-attachments/assets/f0fc32c5-1045-4fd9-900d-c4e7e429d54f"
/>
dcreager added a commit that referenced this pull request Aug 7, 2025
* main:
  Update Rust toolchain to 1.89 (#19807)
  [ty] Add `ty.inlayHints.variableTypes` server option (#19780)
  [ty] Add failing tests for tuple subclasses (#19803)
  [ty] Add `ty.experimental.rename` server setting (#19800)
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
  ...
epitech314 added a commit to epitech314/ty that referenced this pull request Nov 12, 2025
## Summary

This PR adds reference documentation for the two new editor settings
that were added in astral-sh/ruff#19800 and
astral-sh/ruff#19780.

## Test Plan

<img width="2491" height="1374" alt="Screenshot 2025-08-07 at 19 24 44"
src="https://github.com/user-attachments/assets/f0fc32c5-1045-4fd9-900d-c4e7e429d54f"
/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configuration Related to settings and configuration 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