Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Oct 15, 2025

Summary

This PR unifies the two different ways Ruff and ty construct syntax errors. Ruff has been storing the primary message in the diagnostic itself, while ty attached the message to the primary annotation:

> ruff check try.py
invalid-syntax: name capture `x` makes remaining patterns unreachable
 --> try.py:2:10
  |
1 | match 42:
2 |     case x: ...
  |          ^
3 |     case y: ...
  |

Found 1 error.
> uvx ty check try.py
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
Checking ------------------------------------------------------------ 1/1 files                                                                                                 
error[invalid-syntax]
 --> try.py:2:10
  |
1 | match 42:
2 |     case x: ...
  |          ^ name capture `x` makes remaining patterns unreachable
3 |     case y: ...
  |

Found 1 diagnostic

I think there are benefits to both approaches, and I do like ty's version, but I feel like we should pick one (and it might help with #20901 eventually). I slightly prefer Ruff's version, so I went with that. Hopefully this isn't too controversial, but I'm happy to close this if it is.

Note that this shouldn't change any other diagnostic formats in ty because Diagnostic::primary_message was already falling back to the primary annotation message if the diagnostic message was empty. As a result, I think this change will partially resolve the FIXME therein.

Test Plan

Existing tests with updated snapshots

Summary
--

This PR unifies the two different ways Ruff and ty construct syntax errors. Ruff
has been storing the primary message in the diagnostic itself, while ty attached
the message to the primary annotation:

```
> ruff check try.py
invalid-syntax: name capture `x` makes remaining patterns unreachable
 --> try.py:2:10
  |
1 | match 42:
2 |     case x: ...
  |          ^
3 |     case y: ...
  |

Found 1 error.
> uvx ty check try.py
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
Checking ------------------------------------------------------------ 1/1 files                                                                                                 error[invalid-syntax]
 --> try.py:2:10
  |
1 | match 42:
2 |     case x: ...
  |          ^ name capture `x` makes remaining patterns unreachable
3 |     case y: ...
  |

Found 1 diagnostic
```

I think there are benefits to both approaches, and I do like ty's version, but I
feel like we should pick one (and it might help with #20901 eventually). I
slightly prefer Ruff's version, so I went with that. Hopefully this isn't too
controversial, but I'm happy to close this if it is.

Note that this shouldn't change any other diagnostic formats in ty because
[`Diagnostic::primary_message`](https://github.com/astral-sh/ruff/blob/98d27c412810e157f8a65ea75726d66676628225/crates/ruff_db/src/diagnostic/mod.rs#L174)
was already falling back to the primary annotation message if the diagnostic
message was empty. As a result, I think this change will partially resolve the
FIXME therein.

Test Plan
--

Existing tests with updated snapshots
@ntBre ntBre added ty Multi-file analysis & type inference diagnostics Related to reporting of diagnostics. labels Oct 15, 2025
@github-actions
Copy link
Contributor

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@github-actions
Copy link
Contributor

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@github-actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 15, 2025

CodSpeed Performance Report

Merging #20903 will improve performances by 6.51%

Comparing brent/standardize-syntax-error-message (d2e8d76) with main (9de34e7)1

Summary

⚡ 1 improvement
✅ 50 untouched

Benchmarks breakdown

Mode Benchmark BASE HEAD Change
WallTime medium[colour-science] 11.2 s 10.5 s +6.51%

Footnotes

  1. No successful run was found on main (fd568f0) during the generation of this report, so 9de34e7 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@ntBre ntBre marked this pull request as ready for review October 15, 2025 18:51
@AlexWaygood
Copy link
Member

Merging #20903 will improve performances by 6.51%

yeah, that's just a merge race with #20377, judging by the footnote that Codspeed put at the bottom of that comment 😆

@ntBre
Copy link
Contributor Author

ntBre commented Oct 15, 2025

Yes good call, I don't think this should affect performance 😆

Comment on lines +36 to 44
Copy link
Member

Choose a reason for hiding this comment

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

Something like this might be nice here (but not blocking):

error[invalid-syntax]: cannot use an asynchronous comprehension inside of a synchronous comprehension on Python 3.10
 --> src/mdtest_snippet.py:6:19
  |
4 | async def f():
5 |     # error: 19 [invalid-syntax] "cannot use an asynchronous comprehension inside of a synchronous comprehension on Python 3.10 (syntax…
6 |     return {n: [x async for x in elements(n)] for n in range(3)}
  |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ Syntax was added in 3.11
7 | async def test():
8 |     # error: [not-iterable] "Object of type `range` is not async-iterable"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that too, or an info sub-diagnostic. I think either option will need something like #20901, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(or just a separate constructor from the very generic invalid_syntax for unsupported syntax errors)

@ntBre ntBre merged commit e64d772 into main Oct 16, 2025
43 checks passed
@ntBre ntBre deleted the brent/standardize-syntax-error-message branch October 16, 2025 15:56
dcreager added a commit that referenced this pull request Oct 16, 2025
…rable

* origin/main:
  [ty] Support dataclass-transform `field_specifiers` (#20888)
  Bump 0.14.1 (#20925)
  Standardize syntax error construction (#20903)
  [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376)
  [ty] Fix panic 'missing root' when handling completion request (#20917)
  [ty] Run file watching tests serial when using nextest (#20918)
  [ty] Add version hint for failed stdlib attribute accesses (#20909)
  More CI improvements (#20920)
  [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
dcreager added a commit that referenced this pull request Oct 17, 2025
* main:
  [ty] Prefer declared type for invariant collection literals (#20927)
  [ty] Don't track inferability via different `Type` variants (#20677)
  [ty] Use declared variable types as bidirectional type context (#20796)
  [ty] Avoid unnecessarily widening generic specializations (#20875)
  [ty] Support dataclass-transform `field_specifiers` (#20888)
  Bump 0.14.1 (#20925)
  Standardize syntax error construction (#20903)
  [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376)
  [ty] Fix panic 'missing root' when handling completion request (#20917)
  [ty] Run file watching tests serial when using nextest (#20918)
  [ty] Add version hint for failed stdlib attribute accesses (#20909)
  More CI improvements (#20920)
  [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
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.

4 participants