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

Unify OperatorPrecedence enums #16071

Open
1 of 2 tasks
MichaReiser opened this issue Feb 10, 2025 · 6 comments · Fixed by #16162
Open
1 of 2 tasks

Unify OperatorPrecedence enums #16071

MichaReiser opened this issue Feb 10, 2025 · 6 comments · Fixed by #16162
Labels
help wanted Contributions especially welcome internal An internal refactor or improvement

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Feb 10, 2025

Description

There are now 3 OperatorPrecedence structs in ruff

  • Formatter (only the expression one, the pattern one is different)
  • Linter
  • Parser
  1. Move the Linter OperatorPrecedence struct into ruff-python-ast and expose a precedence() method on Expr (and `ExpressionRef) and ideally on each Expression node.
  2. Try to replace some of the other OperatorPrecedence structs with the one now defined in the AST crate

Ideally, these would be two separate PRs

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Feb 10, 2025
@MichaReiser MichaReiser added the help wanted Contributions especially welcome label Feb 10, 2025
@junhsonjb
Copy link
Contributor

I can take this on, I have a few questions though

  • what is the ExpressionRef type? I'm grepping around and I only see LiteralExpressionRef, is that what you're referring to?
  • when you say "Expression node", are you referring to the Expr struct that's implemented in ruff_python_linter/src/nodes.rs?

@MichaReiser
Copy link
Member Author

what is the ExpressionRef type? I'm g

Oh, I forgot that it was renamed. It's ExprRef.

when you say "Expression node", are you referring to the Expr struct that's implemented in ruff_python_linter/src/nodes.rs?

Yes, but the file is in the ruff_python_ast crate (source)

@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Feb 14, 2025

This needs some good design. The fix results in #15919 weren't as nice as I'd want them to be. I found no easy way to avoid parenthesizing when unnecessary:

  2 ** -2  # Intended result: 0.25
# ^^^^^^^ This doesn't need parentheses

  -2 ** 2  # Intended result: 4
# ^^ This does

The newest OperatorPrecedence accepts an expression and output the precedence of the operator that expression represents, but it doesn't consider the context.

# This entire subscript resolves to the precedence of the subscript operator,
# while its slice resolves to that of the unary `-`.
lorem[-ipsum]

# `[]` binds more tightly than `-`,
# so a simple precedence comparison would result in the expected output for this case,
# where `-` is supposed to apply to `lorem` alone:
(-lorem)[ipsum]  # Necessary

# On the other hand, the slice expression doesn't need parenthesizing.
# The comparison result alone isn't enough to avoid this:
lorem[(-ipsum)]  # Unnecessary

A more complex example:

  (a + 1)(
# ^^^^^^^ This needs extra parentheses, as binary `+` binds less tightly than `()`
      a + 1,
#     ^^^^^ This doesn't, as it is an argument
      b := 0,
#     ^^^^^^ This also doesn't; bare named expressions can be passed positionally.
      c = (d := -1)
#         ^^^^^^^^^ But this does.
  )

@junhsonjb
Copy link
Contributor

Just sent out a PR. I wrote most of the code before @InSyncWithFoo's comment. It doesn't attempt to address the problems with parenthesizing yet but I think may still be a good step towards unifying the many definitions of OperatorPrecedence 🙂

@junhsonjb
Copy link
Contributor

I can get started on the second PR for this issue soon. Should be interesting to see how nicely the other versions of OperatorPrecedence play with each other 👀

In regards to the parenthesizing issue, I think that should either be a third PR on this issue or a separate issue -- would you agree @InSyncWithFoo? To be honest, I have no idea how to even approach the problem yet but it seems like a really cool learning opportunity, especially if I can be pointed in the right direction 🙂

@InSyncWithFoo
Copy link
Contributor

@junhsonjb The parenthesizing problem is indeed big and deserves its own PR. That said, I also don't have a good idea of what the logic should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants