Fix as_string() dropping parentheses around a comparison that is the left operand of another comparison - #3239
nikonikolov wants to merge 1 commit into
Conversation
…operand of another comparison Comparisons chain rather than associate, but AsStringVisitor._should_wrap treated Compare like a left-associative operator, so the left operand was never parenthesized: '(a is None) == (b is None)' round-tripped to 'a is None == (b is None)', which Python parses as the semantically different chained comparison '(a is None) and (None == (b is None))'. Comparators on the right only kept their parentheses because is_left=False happened to disagree with the assumed left-associativity. At equal precedence, a Compare operand of a Compare is now always wrapped, on either side. Genuine chained comparisons are unaffected: their operands sit below Compare precedence and take the existing paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eb85057 to
0fd0b5f
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes as_string() so nested comparisons keep necessary parentheses and don’t accidentally turn into Python chained comparisons when unparsing.
Changes:
- Update
AsStringVisitor._should_wrapto always parenthesize comparison operands at equal precedence when the parent is aCompare. - Add regression tests for nested comparisons used as the left operand of another comparison.
- Document the bugfix in the changelog fragment for #3239.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
astroid/nodes/as_string.py |
Adjusts wrapping rules so nested Compare expressions are parenthesized correctly. |
tests/test_nodes.py |
Adds a regression test ensuring exact-text round-trips for nested comparisons and a control for real chained comparisons. |
tests/testdata/python3/data/operator_precedence.py |
Adds precedence/round-trip fixtures covering nested comparisons. |
doc/whatsnew/fragments/3239.bugfix |
Documents the behavioral change and references the issue. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if node_precedence == child_precedence: | ||
| if isinstance(node, nodes.Compare): | ||
| # Comparisons chain rather than associate, so a comparison | ||
| # operand always needs parentheses: (a < b) == (c < d) | ||
| return True | ||
| if is_left != node.op_left_associative(): | ||
| # 3 - (4 - 5) | ||
| # (2**3)**4 | ||
| return True |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3239 +/- ##
=======================================
Coverage 93.65% 93.65%
=======================================
Files 93 93
Lines 11613 11616 +3
=======================================
+ Hits 10876 10879 +3
Misses 737 737
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Type of Changes
Description
as_string()dropped the parentheses around a comparison used as the leftoperand of another comparison:
The unparsed line is a chained comparison —
(a is None) and (None == (b is None))— a semantically different expression.
AsStringVisitor._should_wraphandled equal precedence purely via associativity, andCompareused the default "left associative", so a leftCompareoperand was neverwrapped. But comparisons chain rather than associate, so a
Compareoperand of aComparemust be parenthesized on either side (the right side was only wrappedbecause
is_left=Falsehappened to disagree with the assumed left-associativity).At equal precedence, a
Compareoperand of aCompareis now always wrapped.Genuine chained comparisons (
a < b < c) are unaffected: their operands sit belowCompareprecedence and take the existing paths.Regression tests: exact-text round-trips in
AsStringTest.test_nested_compare_as_left_operand,plus new lines in
tests/testdata/python3/data/operator_precedence.py(both fail withoutthe fix).
🤖 Generated with Claude Code