-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Standardize syntax error construction #20903
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
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
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
CodSpeed Performance ReportMerging #20903 will improve performances by 6.51%Comparing Summary
Benchmarks breakdown
Footnotes |
|
Yes good call, I don't think this should affect performance 😆 |
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.
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"
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.
I was thinking about that too, or an info sub-diagnostic. I think either option will need something like #20901, though.
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.
(or just a separate constructor from the very generic invalid_syntax for unsupported syntax errors)
…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)
* 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)
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:
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_messagewas 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