Skip to content

[ty] Improve diagnostics for syntax errors in forward annotations#25158

Open
AlexWaygood wants to merge 4 commits into
mainfrom
string-annotation-error-spans
Open

[ty] Improve diagnostics for syntax errors in forward annotations#25158
AlexWaygood wants to merge 4 commits into
mainfrom
string-annotation-error-spans

Conversation

@AlexWaygood
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood commented May 14, 2026

Summary

Fixes astral-sh/ty#1627.

Here's an example diagnostic with the current release of ty:

image

On this branch, this diagnostic becomes:

image

The exact span of the node that creates the syntax error is now retained and highlighted in the diagnostic.

Implementation

Propagating the range of the node inside the string annotation into the diagnostic is trivial. However, naively implementing that quickly revealed that this would make diagnostics like this unsuppressable:

x: """list[
    yield from range(42)
]"""

The primary range of the diagnostic is now specifically the yield from range(42) part of the string rather than the string node as a whole. But I cannot add a ty: ignore comment that is either on or above the yield from range(42) part of the string -- the "comment" there would just become part of the string. In order to ensure that these diagnostics are suppressible (and testable in mdtest), therefore, it was necessary to add a way to attach a custom primary range to a diagnostic separate to that diagnostic's suppression range. This is something we've talked about doing lots of times before, so I hope it isn't too controversial.

This in itself was also fairly trivial, but I soon realised that in order for mdtest to be able to recognise # error assertions correctly, we would need to retain the custom suppression ranges for diagnostics iff the testing feature was activated. This ended up feeling slightly icky (lots of #[cfg(feature = "testing")] attributes everywhere), but I'm not sure I see another way.

The second commit of this PR improves the consistency of our parser error messages in general when it comes to capitalization.

Test Plan

Mdtests extended and updated

@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented May 14, 2026

Typing conformance results

No changes detected ✅

Current numbers
The percentage of diagnostics emitted that were expected errors held steady at 89.36%. The percentage of expected errors that received a diagnostic held steady at 85.49%. The number of fully passing files held steady at 88/134.

@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented May 14, 2026

Memory usage report

Memory usage unchanged ✅

@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented May 14, 2026

ecosystem-analyzer results

No diagnostic changes detected ✅

Full report with detailed diff (timing results)

@AlexWaygood AlexWaygood force-pushed the string-annotation-error-spans branch from d8cb69f to b48c5b0 Compare May 14, 2026 13:35
@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented May 14, 2026

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Comment on lines +5 to +7
[options]
exclude-newer = "2026-05-06T17:15:42Z"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added automatically by uv run crates/ty_python_semantic/mdtest.py. I assume it's a change in newer versions of uv?

@AlexWaygood AlexWaygood added ty Multi-file analysis & type inference diagnostics Related to reporting of diagnostics. labels May 14, 2026
@AlexWaygood AlexWaygood marked this pull request as ready for review May 14, 2026 13:40
Comment thread crates/ty_python_semantic/src/types/context.rs Outdated
Comment thread crates/ty_python_semantic/src/types/context.rs Outdated
// Suppress diagnostics in unreachable code. This checks both whether
// the scope itself is unreachable and whether the specific statement or
// expression containing this diagnostic is unreachable.
if !ctx.is_range_reachable(range) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does is_range_reachable work with ranges that point into string annotations?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why it wouldn't: the range inside a string annotation is still relative to the start of the file and is_range_reachable just uses contains_range:

entry_range.contains_range(range) && !is_reachable(db, use_def, constraint)

Comment thread crates/ruff_db/src/diagnostic/mod.rs Outdated
@AlexWaygood AlexWaygood force-pushed the string-annotation-error-spans branch from 71a8c76 to 2b4be22 Compare May 15, 2026 19:59
@AlexWaygood AlexWaygood marked this pull request as draft May 15, 2026 19:59
@AlexWaygood AlexWaygood force-pushed the string-annotation-error-spans branch 3 times, most recently from 0cf71d5 to 078b217 Compare May 15, 2026 22:09
@AlexWaygood AlexWaygood force-pushed the string-annotation-error-spans branch 5 times, most recently from 6852d23 to d445f2c Compare May 16, 2026 17:59
@AlexWaygood AlexWaygood force-pushed the string-annotation-error-spans branch from d445f2c to c57a5d8 Compare May 16, 2026 18:40
@AlexWaygood
Copy link
Copy Markdown
Member Author

Okay, I've removed mdtest's custom assertion-matching mechanism. The relevant APIs in the mdtest crate now accept closures that determine whether an assertion should match a given diagnostic, which allows us to hook into ty's error-suppression logic rather than handrolling something totally different in mdtest. I think this should hopefully be extensible for the ongoing project to use mdtests in Ruff too

@AlexWaygood AlexWaygood marked this pull request as ready for review May 16, 2026 18:59
@AlexWaygood AlexWaygood assigned MichaReiser and unassigned carljm May 16, 2026
@AlexWaygood AlexWaygood requested a review from MichaReiser May 16, 2026 18:59
@AlexWaygood AlexWaygood force-pushed the string-annotation-error-spans branch from c57a5d8 to 8462ff8 Compare May 16, 2026 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics. ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

invalid-syntax-in-forward-annotation does not preserve error spans

3 participants