[ty] Inlay hint auto import#22111
Merged
BurntSushi merged 25 commits intoastral-sh:mainfrom Feb 12, 2026
Merged
Conversation
MatthewMckee4
commented
Dec 20, 2025
Typing conformance resultsNo changes detected ✅ |
|
AlexWaygood
added
server
MatthewMckee4
commented
Dec 20, 2025
crates/ty_ide/src/inlay_hints.rs
Outdated
Comment on lines
7615
to
7618
| from typing import Any | ||
|
|
||
| def foo(x: Any): | ||
| a: Any | Literal["some"] = getattr(x, 'foo', "some") |
Contributor
Author
There was a problem hiding this comment.
Having an issue here. We are not importing Literal here.
With some printlns we get see that <special-form 'typing.Literal'> is resolving to a definition but we can't find a name for it.
Not to mention us trying import str, for the actual literal type.
details.label = "Any | Literal[\"some\"]"
details.targets = [0..3, 6..13, 14..20]
Type: <special-form 'typing.Literal'>
qualified_label_part
type_definition = SpecialForm(Definition { [salsa id]: Id(1486) })
definition = Definition { [salsa id]: Id(1486) }
definition_name = None
Type: Literal["some"]
qualified_label_part
type_definition = Class(Definition { [salsa id]: Id(6135) })
definition = Definition { [salsa id]: Id(6135) }
definition_name = Some("str")
importing "str".
Contributor
Author
There was a problem hiding this comment.
Not really sure where to go from here, other than marking literals as invalid syntax, which seems wrong.
MatthewMckee4
commented
Dec 20, 2025
MatthewMckee4
commented
Dec 20, 2025
|
|
||
| import foo | ||
|
|
||
| a: foo.B[foo.A[D[int, list[str | foo.A[foo.B[int]]]]]] = foo.C().foo() |
Contributor
Author
There was a problem hiding this comment.
Should this annotation that we insert be the one we show in the inlay hint?
It currently isn't, but interested to what what people think
Member
There was a problem hiding this comment.
I think it would be more expensive to show the inlay hint with correct qualifications right? Especially since we have to do that up-front. So I think I'd prefer to start with the less expensive option.
MatthewMckee4
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Contributor
Author
|
Would appreciate some initial feedback. thanks |
Contributor
Author
|
Already I have seen issues with |
MichaReiser
reviewed
Dec 22, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thanks for working on this.
I started reviewing this PR but I don't feel like I've enough context myself to do it successfully.
Would you mind adding some more details to your summary explaining your approach (and what you looked at)? This would help me avoid some duplicate work
crates/ty_ide/src/inlay_hints.rs
Outdated
Comment on lines
29
to
35
| db: &dyn Db, | ||
| expr: &Expr, | ||
| rhs: &Expr, | ||
| ty: Type, | ||
| db: &dyn Db, | ||
| file: File, | ||
| parsed: &ParsedModuleRef, | ||
| allow_edits: bool, |
Member
There was a problem hiding this comment.
The arguments here get a bit out of hand and we might also benefit from caching Stylist::from_tokens. It could make sense to have a InlayHintBuilter or similar that's stateful and holds on to db, file (SemanticModel?), ParsedMOduleRef, allow_auto_import` and possibly more
Contributor
Author
There was a problem hiding this comment.
I have added a context as the first argument
struct InlayHintImporterContext<'a, 'db> {
db: &'db dyn Db,
file: File,
dynamic_importer: &'a mut DynamicImporter<'a, 'db>,
}
crates/ty_ide/src/inlay_hints.rs
Outdated
| } | ||
|
|
||
| // Get the possible qualified label part | ||
| let qualified_label_part = |dynamic_importer: &mut DynamicImporter| { |
Member
There was a problem hiding this comment.
Have you looked at what we do in completions to import missing symbols?
ruff/crates/ty_ide/src/completion.rs
Lines 1293 to 1335 in fc8122c
Contributor
Author
There was a problem hiding this comment.
Yeah I have.
I took some things from it, but some things are different in that we have less certainty about whether we should qualify the name (i think).
And we want to be sure about whether to "import" of "import from"
crates/ty_ide/src/inlay_hints.rs
Outdated
| let mut dynamic_importer = should_auto_import.then(|| { | ||
| let importer = Importer::new(db, &stylist, file, source.as_str(), parsed); | ||
| let members = importer.members_in_scope_at(expr.into(), expr.range().start()); | ||
| DynamicImporter::new(importer, members) |
Member
There was a problem hiding this comment.
How much slower do our inlay hints become due to imports? I wonder if it's time to switch to lazily computing inlay hints.
Contributor
Author
There was a problem hiding this comment.
I have not tested this at all yet.
Contributor
Author
There was a problem hiding this comment.
A small ish test added to the PR description
Member
There was a problem hiding this comment.
The performance result look concerning to me, considering that the file is very small. It might be worth testing with a larger file, to see how that influences the response times.
I still think that, ideally, we do auto imports in a separte resolve request (where we just maintain enough information in the inlay so that we can resolve the auto import later)
Contributor
Author
|
@MichaReiser thanks! I have added more content to the PR description. |
Member
|
@Gankra you've probably the most context on this PR |
charliermarsh
force-pushed
the
inlay-hint-auto-import
branch
from
6f4638b to
3eeddc3
Compare
charliermarsh
force-pushed
the
inlay-hint-auto-import
branch
from
3eeddc3 to
027df7c
Compare
charliermarsh
force-pushed
the
inlay-hint-auto-import
branch
from
b8819bf to
3e241f3
Compare
BurntSushi
reviewed
Feb 11, 2026
Member
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
With the lazy importing here, how does the performance test fair?
I think even if it makes the status quo a little slower, that seems okay and we can improve on it later. But it might be worth checking inlay hints on a big-but-real Python file and seeing how it fairs.
I think this otherwise LGTM, although I'm not that familiar with the inlay hint implementation. The import handling looks good to me though.
|
|
||
| import foo | ||
|
|
||
| a: foo.B[foo.A[D[int, list[str | foo.A[foo.B[int]]]]]] = foo.C().foo() |
Member
There was a problem hiding this comment.
I think it would be more expensive to show the inlay hint with correct qualifications right? Especially since we have to do that up-front. So I think I'd prefer to start with the less expensive option.
charliermarsh
approved these changes
Feb 11, 2026
Member
|
I will leave to @BurntSushi to finalize tomorrow. |
Contributor
Author
|
Thanks for the rebase |
Member
|
I did the test in the OP, and I think this does still make perf a bit worse, but it seems better with the lazy/deduplicated importing implemented. This is on current And with this PR: I did this by opening neovim in a bare bones configuration and toggling inlay hints with I also opened some files in Home Assistant where getting inlay hint requests takes a bit longer (sometimes 50-60ms for one file on my machine). But between I think Micha's suggestion
Would be good future work. (Possibly also for completions too.) But I'm comfortable bringing this in now given that it's a nice quality of life improvement. |
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
Part of astral-sh/ty#1625
This PR implements the auto importing for unimported symbols introduced when "applying" an inlay hint.
Screencast_20251222_120138.webm
There are several things to consider here.
We have all of the information about the symbols we need to import,
from the type display details.
So we need to get the definition of the type and attempt to import that.
Also from the type display details, we get the name of the symbol that is
displayed in the inlay hint, this is useful for the following reason.
If we have an inlay hint that contains the same symbol name twice, but
from different symbols, bar.A and baz.A. the label we see in the inlay
hint will be exactly
bar.Aandbaz.A. From this we know that we shouldnot try to add imports statement
from bar import Aandfrom baz import A,because these will conflict. I believe this is all of the information we can
get that can tell us if we should "qualify" a symbol (bar.A or A).
So, we check if the label (like "bar.A") contains ".", this could possibly
be improved. If it does we force a "import " statement via:
Otherwise, we try to add a "from import " statement via:
We don't force here since it is okay if we still end up adding a
"import " statement and we qualify the name via
"import_action.symbol_text()".
We also do not consider the symbol being a module. I think this is fine.
Because we store these new imports, and the importer cant work with dynamiclly imported members, we can get two from imports from the same module with two different import statements, like the following.
Current Issues
I have seen issues with Literal (we don't find the right symbol to import) and None (we import NoneType)
Test Plan
Update several ty_ide tests and added some more.
I think i will need to add even more.
Performance Test
In main.py, we get the following results.
Before
After