Skip to content

Conversation

@Gankra
Copy link
Contributor

@Gankra Gankra commented Dec 3, 2025

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:

x: "foobar"
    ^^^^^^

Which the supression code-action hands the end of to Tokens::after which 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).

@Gankra Gankra added bug Something isn't working ty Multi-file analysis & type inference server Related to the LSP server labels Dec 3, 2025
@AlexWaygood AlexWaygood removed their request for review December 3, 2025 22:22
@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 3, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@Gankra
Copy link
Contributor Author

Gankra commented Dec 3, 2025

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.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 3, 2025

mypy_primer results

Changes were detected when running on open source projects
beartype (https://github.com/beartype/beartype)
- beartype/claw/_package/clawpkgtrie.py:66:29: warning[unsupported-base] Unsupported class base with type `<class 'dict[str, PackagesTrieBlacklist]'> | <class 'dict[str, Divergent]'>`
- beartype/claw/_package/clawpkgtrie.py:247:29: warning[unsupported-base] Unsupported class base with type `<class 'dict[str, PackagesTrieWhitelist]'> | <class 'dict[str, Divergent]'>`
- Found 494 diagnostics
+ Found 492 diagnostics

scikit-build-core (https://github.com/scikit-build/scikit-build-core)
+ src/scikit_build_core/_logging.py:153:13: warning[unsupported-base] Unsupported class base with type `<class 'Mapping[str, Style]'> | <class 'Mapping[str, Divergent]'>`
- Found 38 diagnostics
+ Found 39 diagnostics

No memory usage changes detected ✅

@carljm carljm removed their request for review December 3, 2025 22:50

/// 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

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

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());
Copy link
Member

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

@sharkdp
Copy link
Contributor

sharkdp commented Dec 4, 2025

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.

@sharkdp sharkdp merged commit 6491932 into main Dec 4, 2025
41 checks passed
@sharkdp sharkdp deleted the gankra/hover-crash branch December 4, 2025 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working server Related to the LSP server ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash on hover of stringified unknown type hint

4 participants