[ty] Double click to insert inlay hint#21600
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
crates/ty_ide/src/inlay_hints.rs
Outdated
| let Some(definition_name) = definition.name(db) else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
This causes issues for some definition kinds, like Unknown.
|
The parts that aren't the auto-import stuff look reasonable to me (just not as familiar with auto-import stuff). Might make sense to have the auto-import stuff be a follow-up? |
|
Yes, I agree. Perhaps @BurntSushi would be able to advise or help with the auto import? I can remove the auto import for now. Perhaps we can put this behind a experimental flag for now also? I will also try to add more invalid syntax places. I also think it would be useful if we had alternative display for types that emit invalid synatx, like I'd still like to add a function signature with |
I'm on the fence about it because you can just... not double-click (and undo if you do). I think we can land the minimal implementation and then disable/flag it if it's a problem for some reason.
Yeah agreed. |
|
I don't think it's strictly a blocker to not catch all the invalid syntax. It's something we'll probably burn down really fast in followups and the only stakes is a user getting mad that a feature they manually invoked did something useless. |
|
Actually ok I think it's slightly higher stakes -- "user gets very confused" is probably more likely. But still, followups. |
true, but at least for now if you double click on some type with a Though, we may not get the user feedback about missing invalid syntax types if we hid it. Happy to keep it open now, and let the issues trickle in. |
crates/ty_ide/src/inlay_hints.rs
Outdated
| new_text: (*content).to_string(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
What you have here looks reasonable to me. What's going wrong?
There was a problem hiding this comment.
I will review more tonight. But off the top of my head.
T[: typing.TypeVar] = TypeVar("T")the definition name is TypeVar so we from typing import TypeVar for example, but this doesnt help us. as we now have the annotation : typing.TypeVar.
Obviously not a great example because we should not type hint TypeVar (also a good place to not allow insert option`.
But for other types this would cause problems.
So its mostly an issue with the names.
There was a problem hiding this comment.
Oh I see. You might need to use ImportAction::symbol_text, since the import might need to be fully qualified for correctness in some cases.
With that said, I think the specific issue you're hitting is that you're always requesting a from import statement via ImportRequest::import_from. If you want to respect an existing qualified import, then you'll want ImportRequest::import. So you'll probably need to detect which one you have and then use the right request. (Note though that it is only a request. So you still need to use ImportAction::symbol_text.)
I built the importer API with completions in mind. It might make sense to change ImportAction::symbol_text to return an Edit instead of a &str for your use case.
There was a problem hiding this comment.
Okay, thanks. I'll leave this out for this PR i think, and have a go at it separately.
One thing i am thinking is that I don't know how many different formats of types we have like this. If it were just TypeVar and typing.TypeVar I could easily distinguish between them, but I don't know other ways this could show.
There was a problem hiding this comment.
FWIW, T: TypeVar = TypeVar("T") is an invalid TypeVar declaration that ty will complain about, so we maybe shouldn't be offering a code action to add that to the user's code 😅
|
A rough heuristic for invalid syntax that will let you find most things: i believe |
|
Nice okay, do you think its worth it for us to scan over the label after getting the TypeDetails to check for invalid characters? Or should i just search for them in the display.rs file and manually set them to all be invalid |
|
It's really tempting to just do the |
|
Is it ridiculous to try to make some property tests for this? testing that after the edits have been accepted, that we have valid python syntax? And possibly no diagnostics on the file? |
|
More tests is more better, I would have been lazy and just snapshot-tested like the rest of the IDE stuff 😅 |
|
I will try it then, see how it goes. |
|
I will say we have a release tomorrow and I'd love to get some version of this PR in for that release (not a huge deal if not but I think this will really solidify inlay hints as Useful). |
crates/ty_ide/src/inlay_hints.rs
Outdated
| fn dont_allow_edits(stmt_assign: &ruff_python_ast::StmtAssign) -> bool { | ||
| if stmt_assign.targets.len() > 1 { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Oh great call on this -- I might also call this is_valid_syntax, since that's ultimately the crux?
There was a problem hiding this comment.
I guess its "if we add an annotation is it valid syntax".
There was a problem hiding this comment.
But I'm not sure how to name that, "is_valid_syntax" doesn't seem exactly correct.
There was a problem hiding this comment.
annotations_are_valid_syntax?
| class F: | ||
| @property | ||
| def whatever(self): ... | ||
|
|
||
| ab: property = F.whatever |
There was a problem hiding this comment.
(can be a followup) i have a sneaking suspicion this is a fake type
There was a problem hiding this comment.
yeah this didnt look great
| impl<'db> TypeDefinition<'db> { | ||
| pub fn definition<'a>(&'a self) -> Option<&'a Definition<'db>> { | ||
| match self { | ||
| Self::Module(_) => None, | ||
| Self::Class(definition) | ||
| | Self::Function(definition) | ||
| | Self::TypeVar(definition) | ||
| | Self::TypeAlias(definition) | ||
| | Self::SpecialForm(definition) | ||
| | Self::NewType(definition) => Some(definition), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this code from the auto-import stuff?
There was a problem hiding this comment.
yeah, will remove, thanks
|
Hell yes hell yeah let's kick this things tires |
…d-typevar * origin/main: (30 commits) [ty] Double click to insert inlay hint (#21600) [ty] Switch the error code from `unresolved-attribute` to `possibly-missing-attribute` for submodules that may not be available (#21618) [ty] Substitute for `typing.Self` when checking protocol members (#21569) [ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` [ty] Add hint about resolved Python version when a user attempts to import a member added on a newer version (#21615) Use release commit for actions/checkout (#21610) [ty] Add failing mdtest for known `Protocol` panic (#21594) [`parser`] Fix panic when parsing IPython escape command expressions (#21480) Fix cargo shear in CI (#21609) Update actions/checkout digest to c2d88d3 (#21601) Update dependency ruff to v0.14.6 (#21603) Update astral-sh/setup-uv action to v7.1.4 (#21602) Update Rust crate clap to v4.5.53 (#21604) Update taiki-e/install-action action to v2.62.56 (#21608) Update Rust crate hashbrown to v0.16.1 (#21605) Update Rust crate indexmap to v2.12.1 (#21606) Update Rust crate syn to v2.0.111 (#21607) [ty] Check method definitions on subclasses for Liskov violations (#21436) [ty] Fix panic for unclosed string literal in type annotation position (#21592) [ty] Fix rendering of unused suppression diagnostic (#21580) ...
* main: [ty] Extend Liskov checks to also cover classmethods and staticmethods (astral-sh#21598) Dogfood ty on the `scripts` directory (astral-sh#21617) [ty] support generic aliases in `type[...]`, like `type[C[int]]` (astral-sh#21552) [ty] Retain the function-like-ness of `Callable` types when binding `self` (astral-sh#21614) [ty] Distinguish "unconstrained" from "constrained to any type" (astral-sh#21539) Disable ty workspace diagnostics for VSCode users (astral-sh#21620) [ty] Double click to insert inlay hint (astral-sh#21600) [ty] Switch the error code from `unresolved-attribute` to `possibly-missing-attribute` for submodules that may not be available (astral-sh#21618) [ty] Substitute for `typing.Self` when checking protocol members (astral-sh#21569) [ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` [ty] Add hint about resolved Python version when a user attempts to import a member added on a newer version (astral-sh#21615)
Summary
Resolves astral-sh/ty#317 (comment).
I can't get the auto import working great.
I haven't added many places where we specify that the type display is invalid syntax.
Test Plan
Nothing yet