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

Generator always parenthesizes element in list comprehension #5777

Closed
dhruvmanila opened this issue Jul 15, 2023 · 1 comment · Fixed by #6198
Closed

Generator always parenthesizes element in list comprehension #5777

dhruvmanila opened this issue Jul 15, 2023 · 1 comment · Fixed by #6198
Labels
bug Something isn't working fixes Related to suggested fixes for violations help wanted Contributions especially welcome

Comments

@dhruvmanila
Copy link
Member

Yeah, this seems like a bug:

assert_round_trip!("[i*2 for i in range(5)]");
// thread 'source_code::generator::tests::tmp_test' panicked at 'assertion failed: `(left == right)`
//  left: `"[(i * 2) for i in range(5)]"`,
// right: `"[i*2 for i in range(5)]"`', crates/ruff_python_ast/src/source_code/generator.rs:1491:9
// note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
// test source_code::generator::tests::tmp_test ... FAILED

Originally posted by @dhruvmanila in #4880 (comment)

@dhruvmanila
Copy link
Member Author

Omg, too much macro use in our Generator. This is a precedence issue that's probably worth opening an issue for.

This code decides whether to parenthesize an expression or not

let (op, prec) = opprec!(bin, op, BoolOp, And("and", AND), Or("or", OR));
group_if!(prec, {
let mut first = true;
for val in values {
self.p_delim(&mut first, op);
self.unparse_expr(val, prec + 1);
}
});

The oprec macro took me a while to understand but precedence either gets assigned the AND (33) or OR (31) precedence (the same is true for bin op, it's just that they use slightly different values for the precedence)

Now, the generator adds parentheses if the parent's precedence is higher. We use the MAX precedence for the list comprehension, which means all expressions get parenthesized

range: _range,
}) => {
self.p("[");
self.unparse_expr(elt, precedence::MAX);
self.unparse_comp(generators);
self.p("]");
}
Expr::SetComp(ast::ExprSetComp {
elt,
generators,

The fix requires identifying the right precedence and passing it through. A second (unrelated) refactor could be to use an enum for the precedence instead of an u8.

Originally posted by @MichaReiser in #4880 (comment)

@dhruvmanila dhruvmanila added bug Something isn't working fixes Related to suggested fixes for violations labels Jul 15, 2023
@MichaReiser MichaReiser added the help wanted Contributions especially welcome label Jul 20, 2023
dhruvmanila added a commit that referenced this issue Jul 31, 2023
## Summary

This PR adds a new precedence level for the comprehension element. This fixes
the generator to not add parentheses around the comprehension element every
time.

The new precedence level is `COMPREHENSION_ELEMENT` and it should occur after
the `NAMED_EXPR` precedence level because named expressions are always parenthesized.

This matches the behavior of Python `ast.unparse` and tested with the
following snippet:

```python
import ast

code = ""
ast.unparse(ast.parse(code))
```

## Test Plan

Add a bunch of test cases for all the valid nodes at that position.

fixes: #5777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations help wanted Contributions especially welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants