[ty] Add hover and Go to Declaration support for subscript literals#22837
Merged
charliermarsh merged 5 commits intoastral-sh:mainfrom Feb 11, 2026
Merged
[ty] Add hover and Go to Declaration support for subscript literals#22837charliermarsh merged 5 commits intoastral-sh:mainfrom
charliermarsh merged 5 commits intoastral-sh:mainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
|
Hugo-Polloli
commented
Jan 24, 2026
crates/ty_ide/src/hover.rs
Outdated
| HoverContent::TypedDictKey { key, ty } => { | ||
| let (ty_string, syntax) = self.ty_string_and_syntax(ty); | ||
| self.kind | ||
| .fenced_code_block(format!("(key) {key}: {ty_string}"), syntax) |
Contributor
Author
There was a problem hiding this comment.
Based on astral-sh/ty#2477, I fully copied the format of pyright, but tell me if you had something else in mind!
Member
There was a problem hiding this comment.
Currently for this snippet:
from typing import TypedDict
class F(TypedDict):
x: int
"""docs for x"""
def bar(f: F):
f["x"]the on-hover is:
Screenshot
What if it was "(key of F) x: int" instead of "(key) x: int"? I think that would be quite nice
Contributor
Author
There was a problem hiding this comment.
I tried to fiddle with markdown to highlight it the same way you suggested: "(key of F) x: int" but did not manage to do it, though I'm not familiar with how VSCode handles the display, I only used the existing plumbing.
This looks like this for now:
Member
There was a problem hiding this comment.
it looks good to me -- thanks!
Hugo-Polloli
force-pushed
the
subscript-literal-hover
branch
from
ba6edb9 to
fc7f112
Compare
Hugo-Polloli
requested review from
AlexWaygood,
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
reviewed
Jan 24, 2026
crates/ty_ide/src/hover.rs
Outdated
| HoverContent::TypedDictKey { key, ty } => { | ||
| let (ty_string, syntax) = self.ty_string_and_syntax(ty); | ||
| self.kind | ||
| .fenced_code_block(format!("(key) {key}: {ty_string}"), syntax) |
Member
There was a problem hiding this comment.
Currently for this snippet:
from typing import TypedDict
class F(TypedDict):
x: int
"""docs for x"""
def bar(f: F):
f["x"]the on-hover is:
Screenshot
What if it was "(key of F) x: int" instead of "(key) x: int"? I think that would be quite nice
Hugo-Polloli
force-pushed
the
subscript-literal-hover
branch
from
fc7f112 to
6c3a53f
Compare
Member
|
Manually testing on my https://github.com/astral-sh/docstring-adder repo, this still doesn't do a great job for slices, e.g. |
Contributor
Author
Oh sorry, to be fully honest I hadn't even thought about handling slices and was fully focused on "pure" literals. I think we could keep this PR minimal as is, BUT I think it would be worth it for me to take a crack at it and see how much more work it requires, then I can make a decision depending on the answer! |
Hugo-Polloli
force-pushed
the
subscript-literal-hover
branch
from
6c3a53f to
698bce3
Compare
Contributor
Author
|
Slices had a few edge cases to cover (esp. around |
AlexWaygood
approved these changes
Jan 25, 2026
Gankra
reviewed
Jan 26, 2026
Contributor
Gankra
left a comment
Gankra
left a comment
There was a problem hiding this comment.
I have a lot of nits but I'm not sure whether they're worth blocking on.
| if expr.is_slice_expr() | ||
| && let Some(subscript) = enclosing_subscript_with_slice_range(expr.range()) | ||
| { | ||
| return Some(GotoTarget::Expression(subscript.into())); |
Contributor
There was a problem hiding this comment.
It kinda feels like this wants to be its own GotoTarget, in the same way GotoTarget::Call is?
Contributor
There was a problem hiding this comment.
In particular I expect these will want a different goto-declaration implementation, as e.g. cmd+clicking on a typed dict key should jump to the declaration of the field? (You don't need to implement that in this PR, but, making a distinct goto-target makes it easy to specialize that).
Contributor
Author
There was a problem hiding this comment.
I created astral-sh/ty#2630 and added a TODO that references it, is that ok?
Contributor
Author
There was a problem hiding this comment.
hey! since i've had the time to do it while this PR was still open, and since I depend on the previous commits, I've taken the liberty of pushing a new commit for the go to declaration feature to this PR and, i've updated the PR body and title to reflect that
crates/ty_ide/src/goto.rs
Outdated
Comment on lines
681
to
694
| let signed_literal_expr_range = |expr: ast::ExprRef<'_>| -> Option<TextRange> { | ||
| if expr.is_literal_expr() { | ||
| return Some(expr.range()); | ||
| } | ||
|
|
||
| let unary_op = expr.unary_op_expr()?; | ||
| if matches!(unary_op.op, ast::UnaryOp::USub | ast::UnaryOp::UAdd) | ||
| && unary_op.operand.is_literal_expr() | ||
| { | ||
| Some(unary_op.range()) | ||
| } else { | ||
| None | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
This closure is only called once, it's unclear to me why it exists. A Build-your-own-try-block?
Contributor
Author
There was a problem hiding this comment.
Oops at some point in my work i must've been using it twice and factored it out, but then forgot to inline it after changing things up, should be fixed now
crates/ty_ide/src/hover.rs
Outdated
Comment on lines
2953
to
2955
| hover.lines().next(), | ||
| Some("int"), | ||
| "case {index} expected `int` hover, got:\n{hover}" |
Contributor
There was a problem hiding this comment.
I'm not a huge fan of these loop-tests for maintenance reasons... we prefer snapshots precisely because this is "ui" and we want to see the full "render" and if it changes. (and if it does, right now you can cargo insta accept all changes and this breaks that)
But in this case I also don't think it's particularly robust. You should make it a list of str or something so that we distinguish whether we're returning the elements from the indexes.
Contributor
There was a problem hiding this comment.
I am however sympathetic to there being So Many Corner Cases that it's really noisy to have a copy for each, so I don't want to block on the first detail.
Contributor
There was a problem hiding this comment.
I do think these tests are missing cases where the subscript stuff shouldn't fire -- in particular slice[-x] or whatnot.
Contributor
Author
There was a problem hiding this comment.
Ok so I kept the spirit of looping over the case, but I generate a full snapshot output now, is that a better way? Also added a case with a non-literal subscript
crates/ty_ide/src/goto.rs
Outdated
|
|
||
| // Special-case subscripts: if the cursor lands on the index/key, slice bound, or slice | ||
| // inside `value[...]`, retarget the covering node to the parent `ExprSubscript`. | ||
| // This avoids "no hover" on literals and lets hover/goto show the subscript result type. |
Contributor
There was a problem hiding this comment.
Might also be worth elucidating that this specifically still prefers value[a<CURSOR>b] being treated as a hover of ab, and only hovers of literals-in-subscripts are "promoted" to a hover of the subscript.
Hugo-Polloli
force-pushed
the
subscript-literal-hover
branch
from
698bce3 to
1475a30
Compare
Contributor
Author
Not really nits! Those were all rly valid, every changes is contained within commit expand coverage using snapshots |
Hugo-Polloli
force-pushed
the
subscript-literal-hover
branch
from
1475a30 to
0f90db9
Compare
Hugo-Polloli
changed the title
Contributor
|
@Gankra do you have time to take another look at this and see if you're happy with the way your comments were addressed, and land it if so? |
Hugo-Polloli
force-pushed
the
subscript-literal-hover
branch
from
0f90db9 to
2c28e84
Compare
Contributor
Author
|
I had a small conflict in |
Contributor
Author
|
Looks like the CI job hit some restrictions (prob a transient server issue??) Seems unrelated to my changes though, re-running should fix the problem but I can't re-run it myself |
Member
thanks, I reran it -- all green again now 🎉 |
charliermarsh
force-pushed
the
subscript-literal-hover
branch
from
2c28e84 to
8e9530d
Compare
Memory usage reportMemory usage unchanged ✅ |
charliermarsh
changed the title
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
5 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.
Summary
Fixes astral-sh/ty#1410; fixes astral-sh/ty#2477; fixes astral-sh/ty#2630
Improve hover behavior for subscript expressions when the cursor is on a literal subscript index/key (e.g.
values[0],person["name"]). For TypedDict string-literal keys, render a denser hover line that includes the key name (e.g.(key of Person) name: str) and include the field docstring when available. Also support go-to-declaration on TypedDict keys (cmd+click) to jump to the corresponding field declaration.Test Plan
hover_subscript_literal_indexto cover hovering on literal subscript indices (e.g.values[0]).hover_typed_dict_key_literalto assert(key) name: str(and field docstring) for TypedDict string-literal keys.Also see attached screenshots from a vscode test run:
