-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Fix crash when hovering an unknown string annotation #21782
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Note that, despite all expectations from even me, this crash has nothing to do with all my IDE string annotation work. This is just a pure code-action bug. |
|
|
|
||
| /// Regression test for a panic when a code-fix diagnostic points at a string annotation | ||
| #[test] | ||
| fn code_action_invalid_string_annotations() -> Result<()> { |
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.
Can we make this an integration test instead of an e2e test (they're so verbose and the snapshots are somewhat hard to review). See
ruff/crates/ty_ide/src/code_action.rs
Line 86 in 258b1fd
| fn add_ignore() { |
| ruff_python_ast::token::TokenAt::Single(token) => token.range(), | ||
| ruff_python_ast::token::TokenAt::Between(..) => range, | ||
| }; | ||
| let tokens_after = parsed.tokens().after(token_range.end()); |
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 feels rather fragile if we have to do this for many fixes, and we should explore alternative ways to handle string annotations if this becomes a frequent problem, or consider as an alternative solution for how to store types for expressions in stringified type annotations.
Something I had in mind for a long time and that can also become handy if we ever want to support checking Python embedded in other languages (e.g. Python in Markdown) is the concept of virtual files. r-a has something very similar, where code references actual code in a file OR code generated by a macro. The way they solve this is by having a HIRFile. In our case, HIRFile would be a salsa::Supertype enum over:
#[derive(salsa::Supertype)]
enum HIRFile {
File(File),
/// Stringified type annotation,
EmbeddedExpression(EmbeddedExpression),
/// Python in a markdown file
EmbeddedBlock(EmbeddedBlock),
}
#[salsa::interned]
struct EmbeddedExpression {
file: File, // Or HIRFile to support markdown docstring in a python code snippet in a python file?
range: TextRange,
}
#[salsa::Interned]
struct EmbeddedBlock {
file: File,
range: TextRange
}parsed_module and TypeInferenceBuilder would take a HIRFile as an argument and so would other queries. Obviously, this requires more design work to flesh out the details. E.g., how does this work with looking up the scope? But it expresses the concept of nested ASTs (or even documents) in a more explicit way
|
I'm merging this to include it in a release (after checking in with Micha), since it looks like the review comments can be addressed post-merge. |
Summary
I have no idea what I'm doing with the fix (all the interesting stuff is in the second commit).
The basic problem is the compiler emits the diagnostic:
Which the supression code-action hands the end of to
Tokens::afterwhich then panics because that function panics if handed an offset that is in the middle of a token.Fixes astral-sh/ty#1748
Test Plan
Many tests added (only the e2e test matters).