[ty] Change goto-def for class constructors to always go to class definition#23071
Merged
BurntSushi merged 2 commits intomainfrom Feb 9, 2026
Merged
[ty] Change goto-def for class constructors to always go to class definition#23071BurntSushi merged 2 commits intomainfrom
BurntSushi merged 2 commits intomainfrom
Conversation
BurntSushi
requested review from
AlexWaygood,
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
BurntSushi
added
server
BurntSushi
requested review from
dhruvmanila
and removed request for
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
BurntSushi
force-pushed
the
ag/goto-class-definition-from-constructor
branch
from
2cd98f1 to
55fc171
Compare
Typing conformance resultsNo changes detected ✅ |
|
BurntSushi
force-pushed
the
ag/goto-class-definition-from-constructor
branch
from
55fc171 to
4caa0bf
Compare
dhruvmanila
approved these changes
Feb 9, 2026
Comment on lines
+986
to
+993
| for node in covering_node | ||
| .parent() | ||
| .into_iter() | ||
| .chain(std::iter::once(node)) | ||
| { | ||
| return Some(GotoTarget::Call { | ||
| call, | ||
| callable: name.into(), | ||
| }); | ||
| if let AnyNodeRef::ExprCall(call) = node | ||
| && let ast::Expr::Name(ref name) = *call.func | ||
| { |
Member
There was a problem hiding this comment.
I'm not really sure why do we need to loop over the two nodes here when only one of them is going to be a ExprCall unless I'm missing something? I don't mind this, just wanted to understand it.
Member
Author
There was a problem hiding this comment.
I added this to account for the case when the cursor is on (. Previously, we were assuming we were on a Name node, and thus the parent was the ExprCall. But if we're on the (, then the covering node we get back is directly the ExprCall. So we try the parent first, and if that fails, try the current node to find the right ExprCall.
Comment on lines
+1308
to
+1313
| // Note that we specifically only look for a opening parenthesis | ||
| // here. Matching on a closing parenthesis seems ambiguous. e.g., | ||
| // what do you do in the case of `func(a<CURSOR>)`? Arguably the | ||
| // cursor is "inside" the function call. In any case, when AG tried | ||
| // to match on a closing parenthesis here, a bunch of tests failed | ||
| // in an undesirable way. |
Member
There was a problem hiding this comment.
I think this is correct that we shouldn't try to match it against the closing parenthesis when inside the call and there are arguments because those should navigate as per the argument type, right?
Member
Author
There was a problem hiding this comment.
I'm honestly not sure. I think our current tests definitely assert that. And it seems reasonable to me. I think the most important thing is that we're consistent.
Specifically, consider foo(argument). If your cursor is "on" the ), then it looks like this: foo(argument<CURSOR>). In that case, the cursor isn't really "on" the argument. It's on the ). But visually (usually), the cursor is between argument and ), and so it seems like it could be confusing.
In any case, I think this is something we can iterate on in response to user feedback.
…inition Previously, we would include the class definition in addition to the `__init__` and `__new__` methods, if present. But it seems that this generally leads to unintuitive behavior from the perspective of users. It's also not what pyright does. This was also discussed a bit in the PR adding this functionality: #20014. Fixes astral-sh/ty#2218, Ref astral-sh/ty#2639
…es you too constructor method This commit builds on the previous by tweaking the behavior of goto-def on `(`. This lets users jump to the constructor method(s) directly instead of the class definition. Fixes astral-sh/ty#2218
BurntSushi
force-pushed
the
ag/goto-class-definition-from-constructor
branch
from
4caa0bf to
539c739
Compare
|
I would have preferred to keep the behavior of getting both.. That way I'm immediately moved to the first location (line with Any chance that this could be made configurable? |
Member
Author
|
My sense is that more folks were surprised by the previous behavior. It's also not how pyright works. I'm not quite sure this is the kind of thing we want to offer a configuration knob for. |
Member
|
I realized that, unless this is a standard behavior, users would probably not know that this difference exists. It might be worth documenting this somewhere, probably https://docs.astral.sh/ty/features/language-server/ ? It's not a priority but something that I've realized. |
BurntSushi
added a commit
to astral-sh/ty
that referenced
this pull request
Feb 10, 2026
Member
Author
|
@dhruvmanila Aye, I opened astral-sh/ty#2772 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, the ty LSP would also include
__init__and__new__targets along with the class definition. In some LSP clients (like VS
Code), this would bring up a prompt to ask the user to select one. It
seems like this is generally unexpected by users (and in particular,
deviates from pyright). This PR changes the ty LSP to behave more like
pyright here, and only include the class definition in the response.
This does also make the change to enable users to jump to the
constructor method directly by invoking goto-definition when the cursor
is on the parentheses of the call.
Reviewers might find reviewing commit-by-commit to be helpful here.
The first commit makes the change over to class definitions only.
The second commit adds special support for goto-definition when
the cursor is on a parenthesis.
Fixes astral-sh/ty#2218, Ref astral-sh/ty#2639