Skip to content

[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.

Sign up for GitHub

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

Merged
sharkdp merged 3 commits into from
Dec 4, 2025
+275 −2
Merged

[ty] Fix crash when hovering an unknown string annotation #21782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8fb50df
add a ton of tests
Gankra Dec 3, 2025
ee55b7d
fix crash
Gankra Dec 3, 2025
5250e67
add snapshot
Gankra Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions crates/ty_ide/src/find_references.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,42 @@ cls = MyClass
assert_snapshot!(test.references(), @"No references found");
}

#[test]
fn references_string_annotation_recursive() {
let test = cursor_test(
r#"
ab: "a<CURSOR>b"
"#,
);

assert_snapshot!(test.references(), @r#"
info[references]: Reference 1
--> main.py:2:1
|
2 | ab: "ab"
| ^^
|

info[references]: Reference 2
--> main.py:2:6
|
2 | ab: "ab"
| ^^
|
"#);
}

#[test]
fn references_string_annotation_unknown() {
let test = cursor_test(
r#"
x: "foo<CURSOR>bar"
"#,
);

assert_snapshot!(test.references(), @"No references found");
}

#[test]
fn references_match_name_stmt() {
let test = cursor_test(
Expand Down
35 changes: 35 additions & 0 deletions crates/ty_ide/src/goto_declaration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,6 +1073,41 @@ def another_helper(path):
assert_snapshot!(test.goto_declaration(), @"No goto target found");
}

#[test]
fn goto_declaration_string_annotation_recursive() {
let test = cursor_test(
r#"
ab: "a<CURSOR>b"
"#,
);

assert_snapshot!(test.goto_declaration(), @r#"
info[goto-declaration]: Declaration
--> main.py:2:1
|
2 | ab: "ab"
| ^^
|
info: Source
--> main.py:2:6
|
2 | ab: "ab"
| ^^
|
"#);
}

#[test]
fn goto_declaration_string_annotation_unknown() {
let test = cursor_test(
r#"
x: "foo<CURSOR>bar"
"#,
);

assert_snapshot!(test.goto_declaration(), @"No goto target found");
}

#[test]
fn goto_declaration_nested_instance_attribute() {
let test = cursor_test(
Expand Down
54 changes: 54 additions & 0 deletions crates/ty_ide/src/goto_type_definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -964,6 +964,60 @@ mod tests {
assert_snapshot!(test.goto_type_definition(), @"No goto target found");
}

#[test]
fn goto_type_string_annotation_recursive() {
let test = cursor_test(
r#"
ab: "a<CURSOR>b"
"#,
);

assert_snapshot!(test.goto_type_definition(), @r#"
info[goto-type-definition]: Type definition
--> stdlib/ty_extensions.pyi:20:1
|
19 | # Types
20 | Unknown = object()
| ^^^^^^^
21 | AlwaysTruthy = object()
22 | AlwaysFalsy = object()
|
info: Source
--> main.py:2:6
|
2 | ab: "ab"
| ^^
|
"#);
}

#[test]
fn goto_type_string_annotation_unknown() {
let test = cursor_test(
r#"
x: "foo<CURSOR>bar"
"#,
);

assert_snapshot!(test.goto_type_definition(), @r#"
info[goto-type-definition]: Type definition
--> stdlib/ty_extensions.pyi:20:1
|
19 | # Types
20 | Unknown = object()
| ^^^^^^^
21 | AlwaysTruthy = object()
22 | AlwaysFalsy = object()
|
info: Source
--> main.py:2:5
|
2 | x: "foobar"
| ^^^^^^
|
"#);
}

#[test]
fn goto_type_match_name_stmt() {
let test = cursor_test(
Expand Down
54 changes: 54 additions & 0 deletions crates/ty_ide/src/hover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,60 @@ mod tests {
assert_snapshot!(test.hover(), @"Hover provided no content");
}

#[test]
fn hover_string_annotation_recursive() {
let test = cursor_test(
r#"
ab: "a<CURSOR>b"
"#,
);

assert_snapshot!(test.hover(), @r#"
Unknown
---------------------------------------------
```python
Unknown
```
---------------------------------------------
info[hover]: Hovered content is
--> main.py:2:6
|
2 | ab: "ab"
| ^-
| ||
| |Cursor offset
| source
|
"#);
}

#[test]
fn hover_string_annotation_unknown() {
let test = cursor_test(
r#"
x: "foo<CURSOR>bar"
"#,
);

assert_snapshot!(test.hover(), @r#"
Unknown
---------------------------------------------
```python
Unknown
```
---------------------------------------------
info[hover]: Hovered content is
--> main.py:2:5
|
2 | x: "foobar"
| ^^^-^^
| | |
| | Cursor offset
| source
|
"#);
}

#[test]
fn hover_overload_type_disambiguated1() {
let test = CursorTest::builder()
Expand Down
10 changes: 8 additions & 2 deletions crates/ty_python_semantic/src/suppression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,9 +413,15 @@ pub fn create_suppression_fix(db: &dyn Db, file: File, id: LintId, range: TextRa
}

// Always insert a new suppression at the end of the range to avoid having to deal with multiline strings
// etc.
// etc. Also make sure to not pass a sub-token range to `Tokens::after`.
let parsed = parsed_module(db, file).load(db);
let tokens_after = parsed.tokens().after(range.end());
let tokens = parsed.tokens().at_offset(range.end());
let token_range = match tokens {
ruff_python_ast::token::TokenAt::None => range,
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());
Copy link
Member

@MichaReiser MichaReiser Dec 4, 2025

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


// Same as for `line_end` when building up the `suppressions`: Ignore newlines
// in multiline-strings, inside f-strings, or after a line continuation because we can't
Expand Down
36 changes: 36 additions & 0 deletions crates/ty_server/tests/e2e/code_actions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,3 +309,39 @@ html.parser

Ok(())
}

/// Regression test for a panic when a code-fix diagnostic points at a string annotation
#[test]
fn code_action_invalid_string_annotations() -> Result<()> {
Copy link
Member

@MichaReiser MichaReiser Dec 4, 2025

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() {

let workspace_root = SystemPath::new("src");
let foo = SystemPath::new("src/foo.py");
let foo_content = r#"
ab: "foobar"
"#;

let ty_toml = SystemPath::new("ty.toml");
let ty_toml_content = "";

let mut server = TestServerBuilder::new()?
.with_workspace(workspace_root, None)?
.with_file(ty_toml, ty_toml_content)?
.with_file(foo, foo_content)?
.enable_pull_diagnostics(true)
.build()
.wait_until_workspaces_are_initialized();

server.open_text_document(foo, &foo_content, 1);

// Wait for diagnostics to be computed.
let diagnostics = server.document_diagnostic_request(foo, None);
let range = full_range(foo_content);
let code_action_params = code_actions_at(&server, diagnostics, foo, range);

// Get code actions for the line with the unused ignore comment.
let code_action_id = server.send_request::<CodeActionRequest>(code_action_params);
let code_actions = server.await_response::<CodeActionRequest>(&code_action_id);

insta::assert_json_snapshot!(code_actions);

Ok(())
}
52 changes: 52 additions & 0 deletions ...server/tests/e2e/snapshots/e2e__code_actions__code_action_invalid_string_annotations.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
---
source: crates/ty_server/tests/e2e/code_actions.rs
expression: code_actions
---
[
{
"title": "Ignore 'unresolved-reference' for this line",
"kind": "quickfix",
"diagnostics": [
{
"range": {
"start": {
"line": 1,
"character": 5
},
"end": {
"line": 1,
"character": 11
}
},
"severity": 1,
"code": "unresolved-reference",
"codeDescription": {
"href": "https://ty.dev/rules#unresolved-reference"
},
"source": "ty",
"message": "Name `foobar` used when not defined",
"relatedInformation": []
}
],
"edit": {
"changes": {
"file://<temp_dir>/src/foo.py": [
{
"range": {
"start": {
"line": 1,
"character": 12
},
"end": {
"line": 1,
"character": 12
}
},
"newText": " # ty:ignore[unresolved-reference]"
}
]
}
},
"isPreferred": false
}
]
Loading