Skip to content

fix: preserve parentheses for same-precedence different operators#67

Merged
f41gh7 merged 2 commits intomasterfrom
fix/preserve-parens-same-precedence-ops
Apr 14, 2026
Merged

fix: preserve parentheses for same-precedence different operators#67
f41gh7 merged 2 commits intomasterfrom
fix/preserve-parens-same-precedence-ops

Conversation

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot commented Apr 13, 2026

Summary

  • Fix needBinaryOpArgParens to preserve parentheses when child and parent binary operators have the same precedence but are different operators (e.g., + vs -, * vs /)
  • Previously, a + (b - c) was serialized as a + b - c, changing evaluation order from a + (b - c) to (a + b) - c
  • Affects AppendString, Prettify, ExpandWithExprs, and Optimize code paths

Root Cause

needBinaryOpArgParens only compared operator precedence levels. When two operators shared the same precedence (e.g., +/- at priority 4, *///% at priority 5), parentheses were dropped if the sub-expression was a "simple leaf chain." This is only safe when the operators are identical (since they're left-associative), but not when they differ.

Fix

Added an isRight parameter to needBinaryOpArgParens to distinguish left vs right child position:

  • Right child of left-associative op: require parens when child op differs (e.g., a + (b - c))
  • Left child of right-associative op: require parens when child op differs (safety net for ^ precedence group)

Test plan

  • Added 19 new test cases in TestParseSuccess covering all same-precedence operator combinations
  • All existing tests pass
  • go vet clean
  • gofmt clean

🤖 Generated with Claude Code


Summary by cubic

Fixes binary expression serialization to keep parentheses when child and parent operators share precedence but differ, so re-parsing doesn’t change evaluation order.

  • Bug Fixes
    • Updated needBinaryOpArgParens(arg, parentOp, isRight) to factor child side and require parens when differing operators at the same precedence would change associativity.
    • Keeps forms like a + (b - c), a * (b / c), a and (b unless c), and a ^ (b ^ c) intact across AppendString, Prettify, ExpandWithExprs, and Optimize.
    • Added 20 tests and updated example output to 100 * (disk_free_bytes{...} / disk_total_bytes{...}).

Written for commit 2d9fa2c. Summary will update on new commits.

The needBinaryOpArgParens function only compared operator precedence,
so explicit parentheses on same-precedence but different operators
were dropped during serialization. For example, `a + (b - c)` was
serialized as `a + b - c`, which changes evaluation order when
re-parsed.

Add an isRight parameter to distinguish left vs right child position,
and require parentheses when child and parent operators differ at the
same precedence level and dropping parens would change associativity.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.70%. Comparing base (8ba28d7) to head (2d9fa2c).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
parser.go 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   89.67%   89.70%   +0.02%     
==========================================
  Files          11       11              
  Lines        2984     3010      +26     
==========================================
+ Hits         2676     2700      +24     
- Misses        210      211       +1     
- Partials       98       99       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor Author

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

@f41gh7 f41gh7 requested review from f41gh7 and makasim April 14, 2026 09:41
@f41gh7 f41gh7 self-assigned this Apr 14, 2026
Copy link
Copy Markdown
Contributor

@f41gh7 f41gh7 left a comment

Choose a reason for hiding this comment

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

LGTM

@f41gh7 f41gh7 merged commit 1f774c9 into master Apr 14, 2026
5 checks passed
@f41gh7 f41gh7 deleted the fix/preserve-parens-same-precedence-ops branch April 14, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants