-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Separate BitXorOr
into BitXor
and BitOr
precedence
#16844
Separate BitXorOr
into BitXor
and BitOr
precedence
#16844
Conversation
@@ -28,8 +28,10 @@ pub enum OperatorPrecedence { | |||
/// Precedence of comparisons (`<`, `<=`, `>`, `>=`, `!=`, `==`), |
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.
(This comment is really related to the code on lines 11-18, but GitHub won't let me comment on that block of unchanged code)
The None
, Yield
, and Starred
variants don't show up in Python's official list of Operator Precedences. None
mostly makes sense to me, as a virtual precedence to represent the absolute lowest value. But I'm curious about why Yield
and Starred
were placed in their respective spots. Is there any documentation about this? Or context that can be shared?
[This question is more about my understanding than about the logic in this PR, I just thought this was a good place to ask -- please let me know if there's somewhere else I should raise the question!]
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 think it's just an easier way to model the grammar of those expressions in terms of binding power (precedence levels). The Yield
and Starred
variant don't show up in the precedence table but they're part of the grammar spec (https://docs.python.org/3/reference/grammar.html) because they're implementation details of the parser (refer to yield_expr
and star_named_expression
grammar rule) and the rules for them would be dependent on the surrounding context.
|
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.
Thanks!
OperatorPrecedence::BitXorOr
into BitXor
(higher) and BitOr (ruff_python_ast
)BitXorOr
into BitXor
and BitOr
precedence
* main: (26 commits) Use the common `OperatorPrecedence` for the parser (#16747) [red-knot] Check subtype relation between callable types (#16804) [red-knot] Check whether two callable types are equivalent (#16698) [red-knot] Ban most `Type::Instance` types in type expressions (#16872) Special-case value-expression inference of special form subscriptions (#16877) [syntax-errors] Fix star annotation before Python 3.11 (#16878) Recognize `SyntaxError:` as an error code for ecosystem checks (#16879) [red-knot] add test cases result in false positive errors (#16856) Bump 0.11.1 (#16871) Allow discovery of venv in VIRTUAL_ENV env variable (#16853) Split git pathspecs in change determination onto separate lines (#16869) Use the correct base commit for change determination (#16857) Separate `BitXorOr` into `BitXor` and `BitOr` precedence (#16844) Server: Allow `FixAll` action in presence of version-specific syntax errors (#16848) [`refurb`] Fix starred expressions fix (`FURB161`) (#16550) [`flake8-executable`] Add pytest and uv run to help message for `shebang-missing-python` (`EXE003`) (#16855) Show more precise messages in invalid type expressions (#16850) [`flake8-executables`] Allow `uv run` in shebang line for `shebang-missing-python` (`EXE003`) (#16849) Add `--exit-non-zero-on-format` (#16009) [red-knot] Ban list literals in most contexts in type expressions (#16847) ...
Summary
This change follows up on the bug-fix requested in #16747 --
ruff_python_ast::OperatorPrecedence
had an enum variant,BitXorOr
, which which gave the same precedence to the|
and^
operators. This goes against Python's documentation for operator precedence, so this PR changes the code so that it's correct.This is part of the overall effort to unify redundant definitions of
OperatorPrecedence
throughout the codebase (#16071)Test Plan
Because this is an internal change, I only ran existing tests to ensure nothing was broken.