[ty] hover + goto-declaration for subscript literals#22837
[ty] hover + goto-declaration for subscript literals#22837Hugo-Polloli wants to merge 5 commits intoastral-sh:mainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
|
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) |
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!
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
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:
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
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) |
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
|
Manually testing on my https://github.com/astral-sh/docstring-adder repo, this still doesn't do a great job for slices, e.g. |
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
|
Slices had a few edge cases to cover (esp. around |
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())); |
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?
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).
There was a problem hiding this comment.
I created astral-sh/ty#2630 and added a TODO that references it, is that ok?
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
| 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 | ||
| } | ||
| }; |
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?
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
| hover.lines().next(), | ||
| Some("int"), | ||
| "case {index} expected `int` hover, got:\n{hover}" |
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.
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.
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.
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. |
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
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
|
@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? |
4 participants
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:
