[ty] Show related information in diagnostic #17359
Conversation
|
MichaReiser
force-pushed
the
micha/diagnostic-related-information
branch
2 times, most recently
from
4799b43 to
87633b3
Compare
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
changed the title
MichaReiser
force-pushed
the
micha/diagnostic-related-information
branch
from
87633b3 to
5a6035d
Compare
MichaReiser
force-pushed
the
micha/diagnostic-related-information
branch
from
5a6035d to
fe74e0e
Compare
|
This looks great to me, but I'll probably leave review to @BurntSushi |
AlexWaygood
requested review from
BurntSushi and
dhruvmanila
and removed request for
AlexWaygood
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
For now, this only renders subdiagnostics and annotations with location information. I played with showing other annotations as well, but the rendering in VS code looked awful (I mainly played with the overload diagnostic).
I think specifically for overloads this seems like a reasonable amount of information to display using related information because a user can hover over the function variable to look at the possible overloads. For example, here the first group is the ty diagnostics while the last group is the hover content:
How this plays out with other diagnostics, that's something that we can play around in follow-ups but this is a good start as you've mentioned.
I know Neovim doesn't support related information but I'm not sure about editors other than VS Code like Zed.
| /// The type returned implements the `std::fmt::Display` trait. In most | ||
| /// cases, just converting it to a string (or printing it) will do what | ||
| /// you want. | ||
| pub fn concise_message(&self) -> ConciseMessage { |
There was a problem hiding this comment.
This looks the same as Diagnostic::concise_message. I think @BurntSushi might have better recommendation but we could use a trait to avoid the duplication.
There was a problem hiding this comment.
I would actually prefer the duplication over a trait. :-)
If the duplication gets obnoxious, another path we could follow is split the common parts of Diagnostic and SubDiagnostic out into a new internal helper type, and implement what we need there. But I don't think that's quite worth doing yet.
My bias is that I tend to be pretty trait-averse.
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
This looks like a great start to me!
| /// The type returned implements the `std::fmt::Display` trait. In most | ||
| /// cases, just converting it to a string (or printing it) will do what | ||
| /// you want. | ||
| pub fn concise_message(&self) -> ConciseMessage { |
There was a problem hiding this comment.
I would actually prefer the duplication over a trait. :-)
If the duplication gets obnoxious, another path we could follow is split the common parts of Diagnostic and SubDiagnostic out into a new internal helper type, and implement what we need there. But I don't think that's quite worth doing yet.
My bias is that I tend to be pretty trait-averse.
…rals * origin/main: [ty] Add hint that PEP 604 union syntax is only available in 3.10+ (#18192) Unify `Message` variants (#18051) [`airflow`] Update `AIR301` and `AIR311` with the latest Airflow implementations (#17985) [`airflow`] Move rules from `AIR312` to `AIR302` (#17940) [ty] Small LSP cleanups (#18201) [ty] Show related information in diagnostic (#17359) Default `src.root` to `['.', '<project_name>']` if the directory exists (#18141)
* main: [ty] Use first matching constructor overload when inferring specializations (#18204) [ty] Add hint that PEP 604 union syntax is only available in 3.10+ (#18192) Unify `Message` variants (#18051) [`airflow`] Update `AIR301` and `AIR311` with the latest Airflow implementations (#17985) [`airflow`] Move rules from `AIR312` to `AIR302` (#17940) [ty] Small LSP cleanups (#18201) [ty] Show related information in diagnostic (#17359) Default `src.root` to `['.', '<project_name>']` if the directory exists (#18141)
Summary
Uses the
relatedInformationfield in diagnostics to render additional data.For now, this only renders subdiagnostics and annotations with location information. I played with showing other annotations as well, but the rendering in VS code looked awful (I mainly played with the overload diagnostic). I think this is an improvement over the status quo and we can iterate on it as we go.
Closes astral-sh/ty#170
Test Plan
Screen.Recording.2025-05-17.at.20.34.46.mov