-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Refactor inlay hints structure to use separate parts #20052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ty] Refactor inlay hints structure to use separate parts #20052
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
"character": 1 | ||
}, | ||
"label": ": Literal[1]", | ||
"label": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These change because we now use InlayHintLabel::LabelParts
instead of InlayHintLabel::String
|
crates/ty_ide/src/inlay_hints.rs
Outdated
let mut label = InlayHintLabel::simple(ty.display(db).to_string(), None); | ||
|
||
label.prepend_str(": "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is roughly how rust analyzer does it and we need to separate these for the goto targets
crates/ty_ide/src/inlay_hints.rs
Outdated
|
||
#[derive(Debug, Clone)] | ||
pub struct InlayHintLabel { | ||
pub parts: SmallVec<[InlayHintLabelPart; 1]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i should change this to 2. @MichaReiser would you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive done it, i think it makes sense for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay in this situation we only use 1 of the InlayHintLabelPart
, but when we add the goto, we will use 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense today because all our usages have exactly two parts. But it's hard to tell, depending on how you plan to use this going forward.
A too-large number has the downside that InlayHintLabel
becomes larger overall, wasting memory when we only need to store 1 or more than two elements. This is especially problematic if we create many InlayHintLabel
s. I don't think this is a big concern here.
I'm not even sure if it's worth using a SmallVec
here. We'll need to serialize all that data later on, we might as well just store a Vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently because we don't use the target
variable in InlayHintLabel
. This means we just have one part
so we will only use one of the elements in the small vec.
I'll change to vec for now anyway.
Could you extend the summary? I think I've an idea why we may want this but I'd rather be sure (and documenting this will also help a future reader who comes across this PR and wonders why it works the way it does) |
crates/ty_ide/src/inlay_hints.rs
Outdated
impl InlayHint { | ||
pub fn type_hint(position: TextSize, ty: Type, db: &dyn Db) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be useful to add basic documentation for all the pub
items in this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to make more things private
crates/ty_ide/src/inlay_hints.rs
Outdated
match &mut *self.parts { | ||
[InlayHintLabelPart { text, target: None }, ..] => text.insert_str(0, s), | ||
_ => self.parts.insert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I thought that the catch-all branch is for the empty vector ([]
) but I don't think that's true, the branch that isn't covered here is an InlayHintLabelPart
where target
is Some
. Can you expand on why does the string needs to be added only when navigation target is not present? Based on the method name, I would expect this to add the string regardless of whether target is present or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ive just removed these methods, i think its better for us to just be more explicit with what the specific parts are anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because that would allow us to have the navigation target that is relevant for that specific part of the label? For example, in : Type
, it's just the "Type" part that's possible to do go to definition.
Feel free to re-request for review once it's ready. |
could you re run that cargo test, looks like a flake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'm assuming that this was fixed? |
* main: [`ruff`] Preserve relative whitespace in multi-line expressions (`RUF033`) (astral-sh#19647) [ty] Optimize TDD atom ordering (astral-sh#20098) [`airflow`] Extend `AIR311` and `AIR312` rules (astral-sh#20082) [ty] Preserve qualifiers when accessing attributes on unions/intersections (astral-sh#20114) [ty] Fix the inferred interface of specialized generic protocols (astral-sh#19866) [ty] Infer slightly more precise types for comprehensions (astral-sh#20111) [ty] Add more tests for protocols (astral-sh#20095) [ty] don't eagerly unpack aliases in user-authored unions (astral-sh#20055) [`flake8-use-pathlib`] Update links to the table showing the correspondence between `os` and `pathlib` (astral-sh#20103) [`flake8-use-pathlib`] Make `PTH100` fix unsafe because it can change behavior (astral-sh#20100) [`flake8-use-pathlib`] Delete unused `Rule::OsSymlink` enabled check (astral-sh#20099) [ty] Add search paths info to unresolved import diagnostics (astral-sh#20040) [`flake8-logging-format`] Add auto-fix for f-string logging calls (`G004`) (astral-sh#19303) Add a `ScopeKind` for the `__class__` cell (astral-sh#20048) Fix incorrect D413 links in docstrings convention FAQ (astral-sh#20089) [ty] Refactor inlay hints structure to use separate parts (astral-sh#20052)
…20052) ## Summary Our internal inlay hints structure (`ty_ide::InlayHint`) now more closely resembles `lsp_types::InlayHint`. This mainly allows us to convert to `lsp_types::InlayHint` with less hassle, but it also allows us to manage the different parts of the inlay hint better, which in the future will allow us to implement features like goto on the type part of the type inlay hint. It also really isn't important to store a specific `Type` instance in the `InlayHintContent`. So we remove this and use `InlayHintLabel` instead which just shows the representation of the type (along with other information). We see a similar structure used in rust-analyzer too.
Summary
Our internal inlay hints structure (
ty_ide::InlayHint
) now more closely resembleslsp_types::InlayHint
.This mainly allows us to convert to
lsp_types::InlayHint
with less hassle, but it also allows us to manage the different parts of the inlay hint better, which in the future will allow us to implement features like goto on the type part of the type inlay hint.It also really isn't important to store a specific
Type
instance in theInlayHintContent
. So we remove this and useInlayHintLabel
instead which just shows the representation of the type (along with other information).We see a similar structure used in rust-analyzer too.