fix(angular-compiler): preserve operator precedence in emitted binary expressions#2275
Conversation
… expressions The JSEmitter flattened parentheses when emitting binary expressions, producing invalid JS when ?? was mixed with ||/&& (a SyntaxError) and wrong semantics when lower-precedence operators were children of higher-precedence ones (e.g. (a ?? 0) + b emitted as a ?? 0 + b). Add a precedence table and childNeedsParens() helper to visitBinaryOperatorExpr so parens are inserted only when the AST nesting requires them. Also broaden assignment wrapping from Assign-only to isAssignment() to cover compound assignments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for analog-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
scope:angular-compiler
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis pull request introduces operator precedence and associativity handling for JavaScript binary expression emission. The changes add a precedence map and parenthesization helper function to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce focused logic for operator precedence and associativity in a single module, supported by well-structured test cases. Review complexity stems from understanding precedence rules, associativity direction, and edge cases (particularly nullish coalescing interaction with logical operators), but the scope is contained to one specific problem domain. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/angular-compiler/src/lib/integration.spec.ts`:
- Around line 236-256: Add an end-to-end integration test that covers the
regression where a nullish-coalescing expression emitted next to logical
operators causes a SyntaxError: create tests in the same describe block using
compile(...) and expectCompiles(...) that exercise templates like '<div
[attr.x]="(value() ?? 0) || other"></div>' and '<div [attr.x]="(value() ?? 0) &&
other"></div>' (use the same NodeComponent pattern with value = signal<number |
null>(null)), and assert the compiled output contains the parenthesized nullish
expression e.g. '(ctx.value() ?? 0) ||' or '(ctx.value() ?? 0) &&' so the code
generator preserves grouping when mixing ?? with || and &&.
In `@packages/angular-compiler/src/lib/js-emitter.spec.ts`:
- Around line 162-167: Add a second lightweight spec that asserts compound
assignments are wrapped too: create an expression using
o.BinaryOperator.PlusAssign (e.g., const expr = bin(o.BinaryOperator.PlusAssign,
v('x'), lit(1))) and assert expect(emitAngularExpr(expr)).toBe('(x += 1)'); this
pins the broadened behavior in visitBinaryOperatorExpr() which now uses
ast.isAssignment().
In `@packages/angular-compiler/src/lib/js-emitter.ts`:
- Around line 90-124: The childNeedsParens function must also detect unary-like
left-hand operands of the exponentiation operator and force parentheses: when
parentOp === o.BinaryOperator.Exponentiation and isRhs === false, return true if
child is an instance of o.UnaryOperatorExpr, o.NotExpr, o.TypeofExpr, or
o.VoidExpr (in addition to the existing BinaryOperatorExpr handling); add this
check near the top of childNeedsParens so unary expressions on the LHS of ** are
wrapped to produce e.g. (-(a)) ** b.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef985fbe-9d77-4c9d-8178-2360483b630d
📒 Files selected for processing (3)
packages/angular-compiler/src/lib/integration.spec.tspackages/angular-compiler/src/lib/js-emitter.spec.tspackages/angular-compiler/src/lib/js-emitter.ts
Pin the broadened isAssignment() behavior with += and ??= assertions as requested in PR review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thank you, added the additional test cases. Awesome project btw 🚀 great work as always |
|
Thanks! This has merge conflicts as I pushed up a refactor of the integration tests |
# Conflicts: # packages/angular-compiler/src/lib/integration.spec.ts
2 participants
visitBinaryOperatorExprflattened parentheses when emitting binary expressions, producing invalid JS when??was mixed with||/&&(a SyntaxError) and wrong semantics when lower-precedence operators were children of higher-precedence ones (e.g.(a ?? 0) + bemitted asa ?? 0 + b)childNeedsParens()helper so parens are inserted only when the AST nesting requires them — handles precedence differences, left/right associativity, and the JS spec's nullish coalescing mixing prohibitionAssign-only toisAssignment()to cover compound assignments (+=,-=, etc.)PR Checklist
Closes #
Affected scope
Recommended merge strategy for maintainer [optional]
Commit preservation note [optional]
What is the new behavior?
Test plan
nx format:checkpnpm buildpnpm testDoes this PR introduce a breaking change?
Other information
[optional] What gif best describes this PR or how it makes you feel?