Skip to content
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

[syntax-errors] Improve error message and range for pre-PEP-614 decorator syntax errors #16581

Merged
merged 3 commits into from
Mar 17, 2025

Conversation

AlexWaygood
Copy link
Member

Summary

A small followup to #16386. We now tell the user exactly what it was about their decorator that constituted invalid syntax on Python <3.9, and the range now highlights the specific sub-expression that is invalid rather than highlighting the whole decorator

Test Plan

Inline snapshots are updated, and new ones are added.

@AlexWaygood AlexWaygood added the parser Related to the parser label Mar 9, 2025
@AlexWaygood AlexWaygood requested a review from ntBre March 9, 2025 22:32
@AlexWaygood AlexWaygood added the preview Related to preview mode features label Mar 9, 2025

Verified

This commit was signed with the committer’s verified signature.
danielroe Daniel Roe
@AlexWaygood AlexWaygood force-pushed the alex/syntax-error-msgs branch from 08badbb to 63e2de2 Compare March 9, 2025 22:34
Copy link
Contributor

github-actions bot commented Mar 9, 2025

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.

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

@MichaReiser
Copy link
Member

MichaReiser commented Mar 10, 2025

@ntBre and I discussed this and we deliberately decided against it because:

  • It introduces more complexity and an extra cost for every added syntax
  • It seems low priority considering that < Py39 is end of life

I'm not opposed to this change. I just want to make clear that we deliberately decided against it because we wanted to focus our time on higher priority syntax errors and reduce complexity

Comment on lines 93 to 94
3 | @await bar
| ^^^^^^^^^ Syntax Error: Cannot use `await` expression outside function-call arguments in a decorator on Python 3.8 (syntax was added in Python 3.9)
Copy link
Member

@dhruvmanila dhruvmanila Mar 10, 2025

Choose a reason for hiding this comment

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

I actually find the "outside function-call arguments in a decorator" part a bit confusing because it made me think that this is only allowed in function-call arguments. I'd possibly remove it and keep it simply "Cannot use await expression in a decorator on Python 3.8 ..." but then the message cannot be used in a general way because, in this example, the await expression is only not allowed at the top-level (@(await x)) while @decorator(await x) would be ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. So you interpreted "outside function-call arguments" as meaning "If I add parentheses, it will be valid syntax"?

Hmm, I'm not sure how to clarify that :/ adding the parentheses doesn't make it a function call -- but I can see how a beginner might find the language confusing. Would you find "outside call expression arguments" any clearer?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply, what I thought it could be interpreted is related to the function that's being decorated and not the decorator itself. But, I think what you've is fine as well. We can iterate if anyone finds it confusing.

wip

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@AlexWaygood AlexWaygood force-pushed the alex/syntax-error-msgs branch from b034baf to 4158b95 Compare March 10, 2025 13:39
@AlexWaygood AlexWaygood merged commit 38bfda9 into main Mar 17, 2025
21 checks passed
@AlexWaygood AlexWaygood deleted the alex/syntax-error-msgs branch March 17, 2025 11:17
dcreager added a commit that referenced this pull request Mar 17, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* main: (25 commits)
  [syntax-errors] Parenthesized context managers before Python 3.9 (#16523)
  [ci]: Disable wheel testing on `ppc64le` (#16793)
  [red-knot] Stabilize `negation_reverses_subtype_order` property test (#16801)
  [red-knot] Emit error if int/float/complex/bytes/boolean literals appear in type expressions outside `typing.Literal[]` (#16765)
  [ci] Use `git diff` instead of `changed-files` GH action (#16796)
  [syntax-errors] Improve error message and range for pre-PEP-614 decorator syntax errors (#16581)
  [`flake8-bandit`] Allow raw strings in `suspicious-mark-safe-usage` (`S308`) #16702 (#16770)
  [`refurb`] Avoid panicking `unwrap` in `verbose-decimal-constructor` (`FURB157`) (#16777)
  [red-knot] Add `--color` CLI option (#16758)
  [internal]: Upgrade salsa (#16794)
  Pin dependencies (#16791)
  [internal]: Update indirect dependencies (#16792)
  [ci]: Fixup codspeed upgrade (#16790)
  Update Rust crate compact_str to 0.9.0 (#16785)
  Update Rust crate clap to v4.5.32 (#16778)
  Update Rust crate codspeed-criterion-compat to v2.9.1 (#16784)
  Update Rust crate quote to v1.0.40 (#16782)
  Update Rust crate ordermap to v0.5.6 (#16781)
  Update cloudflare/wrangler-action action to v3.14.1 (#16783)
  Update Rust crate env_logger to v0.11.7 (#16779)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Related to the parser preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants