From 0bb0d97b7570921343c799fd99cd2a5b27d39852 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Wed, 7 Oct 2026 16:56:17 +0800 Subject: [PATCH] fix(compiler): keep parentheses that JavaScript requires (issue #510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The emitter dropped every OutputExpression::Parenthesized, so `(v) => ({value: v}).value` emitted as `(v) => {value:v}.value` — invalid JS. ngtsc emits such expressions verbatim via WrappedNodeExpr; we rebuild the AST, so parens that survive the strip phase must print. - emitter.rs: `Parenthesized` keeps its parens unless the inner expression already self-parenthesizes (BinaryOperator, Conditional, Comma, UnaryOperator with `parens`). Arrow expression bodies use a new `emits_leading_brace` helper that walks the leftmost token through member access, calls and tagged templates, replacing the whole-body LiteralMap check. - strip_nonrequired_parentheses.rs: handle `IrExpression::Parenthesized` (our pipe-aware ingest produces them; upstream's single-tree strip covers them) and mark required parens on IR `Binary` operands for `**`/`??`/`&&`/`||`. The unary `**`-base check covers `!`/`typeof`/`void` too — upstream only needs `UnaryOperatorExpr` because the TS printer re-adds parens; we emit raw JS. Nested `((-1)) ** 2` marks the outer wrapper so `(-1) ** 2` survives. Fixes #510 --- .../src/output/emitter.rs | 69 ++++-- .../phases/strip_nonrequired_parentheses.rs | 140 ++++++++++- .../tests/arrow_paren_emit_test.rs | 232 ++++++++++++++++++ 3 files changed, 423 insertions(+), 18 deletions(-) create mode 100644 crates/oxc_angular_compiler/tests/arrow_paren_emit_test.rs diff --git a/crates/oxc_angular_compiler/src/output/emitter.rs b/crates/oxc_angular_compiler/src/output/emitter.rs index ec41e7e98..389902f0d 100644 --- a/crates/oxc_angular_compiler/src/output/emitter.rs +++ b/crates/oxc_angular_compiler/src/output/emitter.rs @@ -752,7 +752,30 @@ impl JsEmitter { self.visit_expression(&e.expr, ctx); } OutputExpression::Parenthesized(e) => { + // Keep the parens. `Parenthesized` carries source parens — + // ngtsc emits such expressions verbatim via WrappedNodeExpr — + // or a required paren the strip_nonrequired_parentheses phase + // kept. Dropping them breaks `({a: 1}).a`, `(-1) ** 2`, + // `(1).toString()` and `(obj?.m)()` (issue #510). Upstream's + // emitter drops ParenthesizedExpr parens only because it + // re-adds them contextually via `shouldParenthesize` and never + // sees source parens at all. + // + // Skip printing when the inner expression already wraps + // itself, or `((a + b))` sources would emit double parens. + let self_parenthesized = matches!( + e.expr.as_ref(), + OutputExpression::BinaryOperator(_) + | OutputExpression::Conditional(_) + | OutputExpression::Comma(_) + ) || matches!(&e.expr.as_ref(), OutputExpression::UnaryOperator(u) if u.parens); + if !self_parenthesized { + ctx.print("("); + } self.visit_expression(&e.expr, ctx); + if !self_parenthesized { + ctx.print(")"); + } } OutputExpression::Comma(e) => { ctx.print("("); @@ -781,24 +804,18 @@ impl JsEmitter { ctx.print(") =>"); match &e.body { ArrowFunctionBody::Expression(body_expr) => { - // Check if the body is an object literal (needs parens). - // Also unwrap Parenthesized wrapper, which comes from converting - // OXC's ParenthesizedExpression (e.g. `() => ({ key: val })`). - let inner = match body_expr.as_ref() { - OutputExpression::Parenthesized(p) => p.expr.as_ref(), - other => other, - }; - // Objects the converter keeps as written arrive as raw source. - let is_object_literal = match inner { - OutputExpression::LiteralMap(_) => true, - OutputExpression::RawSource(raw) => raw.source.starts_with('{'), - _ => false, - }; - if is_object_literal { + // The body needs parens when its emitted text starts + // with `{` — a bare object literal, or an expression + // whose leftmost token is one, like + // `({value: v}).value` (issue #510). A `Parenthesized` + // body prints its own parens when needed, so a raw + // `LiteralMap` inside it is still covered. + let needs_parens = emits_leading_brace(body_expr); + if needs_parens { ctx.print("("); } self.visit_expression(body_expr, ctx); - if is_object_literal { + if needs_parens { ctx.print(")"); } } @@ -1192,6 +1209,28 @@ impl Default for JsEmitter { // ============================================================================ /// Convert a binary operator to its JavaScript string representation. +/// Whether the emitted text of `expr` begins with `{` — the JavaScript +/// block-statement hazard that requires parentheses. Walks the leftmost +/// token through member access, calls and tagged templates +/// (`({a: 1}).a`, `({a: 1})()`, `({a: 1})`t``). `Parenthesized` is never +/// "leading `{`": it prints its own parens whenever its inner expression +/// would need them. +/// +/// Expressions that self-parenthesize (`BinaryOperator`, `Conditional`, +/// `Comma`) or print an operator/keyword first (`new `, `!`, `typeof`, +/// `void`, unary `+`/`-`) return `false` — their first token isn't `{`. +fn emits_leading_brace(expr: &OutputExpression<'_>) -> bool { + match expr { + OutputExpression::LiteralMap(_) => true, + OutputExpression::RawSource(raw) => raw.source.starts_with('{'), + OutputExpression::ReadProp(e) => emits_leading_brace(&e.receiver), + OutputExpression::ReadKey(e) => emits_leading_brace(&e.receiver), + OutputExpression::InvokeFunction(e) => emits_leading_brace(&e.fn_expr), + OutputExpression::TaggedTemplateLiteral(e) => emits_leading_brace(&e.tag), + _ => false, + } +} + fn binary_operator_to_str(op: BinaryOperator) -> &'static str { match op { BinaryOperator::Equals => "==", diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/strip_nonrequired_parentheses.rs b/crates/oxc_angular_compiler/src/pipeline/phases/strip_nonrequired_parentheses.rs index 288365c86..8a3476cd2 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/strip_nonrequired_parentheses.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/strip_nonrequired_parentheses.rs @@ -22,7 +22,7 @@ use oxc_allocator::{Allocator, Box as ArenaBox}; use crate::ast::expression::{AngularExpression, BinaryOperator}; use crate::ir::expression::{ - IrExpression, VisitorContextFlag, transform_expressions_in_create_op, + IrBinaryOperator, IrExpression, VisitorContextFlag, transform_expressions_in_create_op, transform_expressions_in_update_op, }; use crate::pipeline::compilation::{ComponentCompilationJob, HostBindingCompilationJob}; @@ -304,6 +304,21 @@ fn check_ir_expression_for_required_parens( check_ir_expression_for_required_parens(&cc.expr, required); } IrExpression::Binary(binary) => { + // IR-level binaries need the same "required parens" marking as + // upstream's `o.BinaryOperatorExpr` walk — pipe-bearing operands + // become IrExpressions, so `(| pipe)`-adjacent parens live here. + match binary.operator { + IrBinaryOperator::Exponentiation => { + check_ir_exponentiation_parens(binary, required); + } + IrBinaryOperator::NullishCoalesce => { + check_ir_nullish_coalescing_parens(binary, required); + } + IrBinaryOperator::And | IrBinaryOperator::Or => { + check_ir_and_or_parens(binary, required); + } + _ => {} + } check_ir_expression_for_required_parens(&binary.lhs, required); check_ir_expression_for_required_parens(&binary.rhs, required); } @@ -522,9 +537,18 @@ fn check_ast_expression_for_required_parens( fn check_exponentiation_parens(expr: &AngularExpression<'_>, required: &mut HashSet<*const ()>) { if let AngularExpression::Binary(binary) = expr { if binary.operation == BinaryOperator::Power { - // Check if left side is a parenthesized unary expression + // Check if left side is a parenthesized unary expression. Upstream + // checks only `UnaryOperatorExpr` because its output goes through + // the TypeScript printer, which re-adds parens; we emit JS + // directly, so `!`/`typeof`/`void` bases need parens kept too. if let AngularExpression::ParenthesizedExpression(paren) = &binary.left { - if matches!(paren.expression, AngularExpression::Unary(_)) { + if matches!( + paren.expression, + AngularExpression::Unary(_) + | AngularExpression::PrefixNot(_) + | AngularExpression::TypeofExpression(_) + | AngularExpression::VoidExpression(_) + ) { // Mark this parenthesized expression as required let ptr = paren as *const _ as *const (); required.insert(ptr); @@ -599,6 +623,103 @@ fn is_logical_and_or(expr: &AngularExpression<'_>) -> bool { } } +/// `-2 ** 3` needs `(-2) ** 3` — mirror of [`check_exponentiation_parens`] +/// for IR expressions (`ir.Binary` carries the AST `Binary` hazard too). +fn check_ir_exponentiation_parens( + binary: &crate::ir::expression::BinaryExpr<'_>, + required: &mut HashSet<*const ()>, +) { + if let IrExpression::Parenthesized(paren) = binary.lhs.as_ref() + && ir_is_unary(&paren.expr) + { + required.insert(paren.as_ref() as *const _ as *const ()); + } +} + +/// `(a && b) ?? c`, `a ?? (b && c)`, `(a ? b : c) ?? d` — mirror of +/// [`check_nullish_coalescing_parens`] for IR expressions. +fn check_ir_nullish_coalescing_parens( + binary: &crate::ir::expression::BinaryExpr<'_>, + required: &mut HashSet<*const ()>, +) { + for operand in [binary.lhs.as_ref(), binary.rhs.as_ref()] { + if let IrExpression::Parenthesized(paren) = operand + && (ir_is_logical_and_or(&paren.expr) || ir_is_conditional(&paren.expr)) + { + required.insert(paren.as_ref() as *const _ as *const ()); + } + } +} + +/// `(a ?? b) && c` — mirror of [`check_and_or_parens`] for IR expressions. +fn check_ir_and_or_parens( + binary: &crate::ir::expression::BinaryExpr<'_>, + required: &mut HashSet<*const ()>, +) { + if let IrExpression::Parenthesized(paren) = binary.lhs.as_ref() + && ir_is_nullish_coalesce(&paren.expr) + { + required.insert(paren.as_ref() as *const _ as *const ()); + } +} + +/// The hazard checks see `AngularExpression` shapes at the AST level; at the +/// IR level the same operand may be an `IrExpression` or still a wrapped AST. +/// "Unary" here means every unary-precedence operator (`+`, `-`, `!`, +/// `typeof`, `void`) — all forbidden as the `**` base without parens. +/// Upstream checks only `UnaryOperatorExpr` because its emit round-trips +/// through the TypeScript printer; our emitter prints raw JS. +fn ir_is_unary(expr: &IrExpression<'_>) -> bool { + match expr { + // Look through nested parens: in `((-1)) ** 2` the outer wrapper is + // the required one (the inner then strips to `(-1)`). + IrExpression::Parenthesized(p) => ir_is_unary(&p.expr), + IrExpression::Unary(_) + | IrExpression::Not(_) + | IrExpression::Typeof(_) + | IrExpression::Void(_) => true, + IrExpression::Ast(ast) => matches!( + ast.as_ref(), + AngularExpression::Unary(_) + | AngularExpression::PrefixNot(_) + | AngularExpression::TypeofExpression(_) + | AngularExpression::VoidExpression(_) + ), + _ => false, + } +} + +fn ir_is_logical_and_or(expr: &IrExpression<'_>) -> bool { + match expr { + IrExpression::Parenthesized(p) => ir_is_logical_and_or(&p.expr), + IrExpression::Binary(b) => { + matches!(b.operator, IrBinaryOperator::And | IrBinaryOperator::Or) + } + IrExpression::Ast(ast) => is_logical_and_or(ast), + _ => false, + } +} + +fn ir_is_conditional(expr: &IrExpression<'_>) -> bool { + match expr { + IrExpression::Parenthesized(p) => ir_is_conditional(&p.expr), + IrExpression::Ternary(_) => true, + IrExpression::Ast(ast) => matches!(ast.as_ref(), AngularExpression::Conditional(_)), + _ => false, + } +} + +fn ir_is_nullish_coalesce(expr: &IrExpression<'_>) -> bool { + match expr { + IrExpression::Parenthesized(p) => ir_is_nullish_coalesce(&p.expr), + IrExpression::Binary(b) => b.operator == IrBinaryOperator::NullishCoalesce, + IrExpression::Ast(ast) => { + matches!(ast.as_ref(), AngularExpression::Binary(b) if b.operation == BinaryOperator::NullishCoalescing) + } + _ => false, + } +} + /// Strip non-required parentheses from an IR expression. fn strip_parens_in_expression<'a>( expr: &mut IrExpression<'a>, @@ -618,6 +739,19 @@ fn strip_parens_in_expression<'a>( } } } + + // Handle IR-level parenthesized expressions the same way. Upstream has a + // single expression tree, so its strip pass covers these; our pipe-aware + // ingest produces `IrExpression::Parenthesized` (e.g. `(a | async) || b`), + // which used to be invisible here — the emitter dropped the parens. + if let IrExpression::Parenthesized(paren) = expr { + let ptr = paren.as_ref() as *const _ as *const (); + if !required.contains(&ptr) { + let inner = + std::mem::replace(paren.expr.as_mut(), IrExpression::empty(allocator, None)); + *expr = inner; + } + } } /// Strips non-required parentheses for host binding compilation. diff --git a/crates/oxc_angular_compiler/tests/arrow_paren_emit_test.rs b/crates/oxc_angular_compiler/tests/arrow_paren_emit_test.rs new file mode 100644 index 000000000..f6f3b2c83 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/arrow_paren_emit_test.rs @@ -0,0 +1,232 @@ +//! Arrow bodies whose leftmost token is `{` must keep their parentheses +//! (issue #510): `(v) => ({value: v}).value` must not emit as +//! `(v) => {value:v}.value` — invalid JS. ngtsc keeps the source text via +//! `WrappedNodeExpr`; our converter rebuilds the AST, so the emitter walks +//! the body's leftmost token (`emits_leading_brace`). + +use oxc_allocator::Allocator; +use oxc_angular_compiler::{ + CompilationMode, TransformOptions, + output::emitter::JsEmitter, + parser::html::HtmlParser, + pipeline::{emit::compile_template, ingest::ingest_component}, + transform::html_to_r3::{HtmlToR3Transform, TransformOptions as HtmlTransformOptions}, + transform_angular_file, +}; +use oxc_str::Ident; + +fn compile(source: &str) -> String { + let allocator = Allocator::default(); + let options = + TransformOptions { compilation_mode: CompilationMode::Full, ..Default::default() }; + let result = transform_angular_file(&allocator, "test.ts", source, Some(&options), None); + assert!(!result.has_errors(), "should not have errors, got: {:?}", result.diagnostics); + result.code.chars().filter(|c| !c.is_whitespace()).collect() +} + +/// Compile a template through the IR pipeline (ingest → phases → emit) so the +/// strip_nonrequired_parentheses phase and the emitter are both exercised. +fn compile_tpl(template: &str) -> String { + let allocator = Allocator::default(); + let parser = HtmlParser::with_expansion_forms(&allocator, template, "test.html"); + let html_result = parser.parse(); + assert!(html_result.errors.is_empty()); + let transformer = HtmlToR3Transform::new( + &allocator, + template, + HtmlTransformOptions { angular_version: None, ..HtmlTransformOptions::default() }, + ); + let r3_result = transformer.transform(&html_result.nodes); + assert!(r3_result.errors.is_empty()); + let mut job = ingest_component(&allocator, Ident::from("TestComponent"), r3_result.nodes); + let result = compile_template(&mut job); + let emitter = JsEmitter::new(); + let mut output = String::new(); + for decl in &result.declarations { + output.push_str(&emitter.emit_statement(decl)); + } + output.push_str(&emitter.emit_statements(&result.template_fn.statements)); + output.chars().filter(|c| !c.is_whitespace()).collect() +} + +/// The issue's `@Input transform` repro: member access on a parenthesized +/// object literal inside an arrow body. +#[test] +fn input_transform_paren_object_member() { + let code = compile( + "import {Directive, Input} from '@angular/core'; +@Directive({selector: '[d]'}) +export class D { + @Input({transform: (v: any) => ({value: v}).value}) a: any; +} +", + ); + assert!( + code.contains("transform:(v)=>({value:v}).value"), + "parens around the object must be kept, got:\n{code}" + ); + assert!( + !code.contains("=>({value:v}") || code.contains("=>({value:v}).value"), + "inner object must stay parenthesized, got:\n{code}" + ); +} + +/// The issue's provider-factory repro. +#[test] +fn provider_factory_paren_object_member() { + let code = compile( + "import {Component} from '@angular/core'; +@Component({selector: 'c', template: '', providers: [{provide: 'T', useFactory: () => ({a: 1}).a}]}) +export class C {} +", + ); + assert!( + code.contains("useFactory:()=>({a:1}).a"), + "parens around the object must be kept, got:\n{code}" + ); +} + +/// `() => ({...})` (issue #43) still emits the object parens exactly once. +#[test] +fn plain_object_arrow_body_still_parenthesized() { + let code = compile( + "import {Component} from '@angular/core'; +@Component({selector: 'c', template: '', providers: [{provide: 'T', useFactory: () => ({a: 1})}]}) +export class C {} +", + ); + assert!( + code.contains("useFactory:()=>({a:1})"), + "object-literal arrow body must stay parenthesized, got:\n{code}" + ); + assert!(!code.contains("=>({a:1}))"), "no double parens, got:\n{code}"); + assert!(!code.contains("=>(({a:1})"), "no double parens, got:\n{code}"); +} + +/// Deeper chains: call on a parenthesized object, nested member access, and +/// source parens on a plain identifier (kept, as ngtsc emits them verbatim). +#[test] +fn chained_and_unneeded_parens() { + let code = compile( + "import {Component} from '@angular/core'; +@Component({selector: 'c', template: '', providers: [ + {provide: 'A', useFactory: () => ({f: () => 1}).f()}, + {provide: 'B', useFactory: () => ({o: {x: 2}}).o.x}, + {provide: 'C', useFactory: (v: any) => (v)}, +]}) +export class C {} +", + ); + assert!(code.contains("useFactory:()=>({f:()=>1}).f()"), "call on object, got:\n{code}"); + assert!( + code.contains("useFactory:()=>({o:{x:2}}).o.x"), + "nested member on object, got:\n{code}" + ); + assert!(code.contains("useFactory:(v)=>(v)"), "source parens kept verbatim, got:\n{code}"); +} + +/// Source parens that JavaScript requires for reasons beyond a leading `{` +/// must survive too (Codex review): unary in an exponentiation base, a +/// number-literal member access, an optional-chain callee, and an arrow +/// invoked immediately. +#[test] +fn other_required_source_parens_survive() { + let code = compile( + "import {Component} from '@angular/core'; +@Component({selector: 'c', template: '', providers: [ + {provide: 'A', useFactory: () => (-1) ** 2}, + {provide: 'B', useFactory: () => (1).toString()}, + {provide: 'C', useFactory: (o: any) => (o?.m)()}, +]}) +export class C {} +", + ); + // BinaryOperator wraps itself in parens in our emit style. + assert!(code.contains("()=>((-1)**2)"), "unary base of ** needs parens, got:\n{code}"); + assert!( + code.contains("()=>(1).toString()"), + "number-literal member access needs parens, got:\n{code}" + ); + assert!(code.contains("(o)=>(o?.m)()"), "optional-chain call needs parens, got:\n{code}"); +} + +/// Partial mode emits the same arrow in `setClassMetadata`/`ngDeclareComponent`. +#[test] +fn partial_mode_paren_object_member() { + let allocator = Allocator::default(); + let options = + TransformOptions { compilation_mode: CompilationMode::Partial, ..Default::default() }; + let result = transform_angular_file( + &allocator, + "test.ts", + "import {Directive, Input} from '@angular/core'; +@Directive({selector: '[d]'}) +export class D { + @Input({transform: (v: any) => ({value: v}).value}) a: any; +} +", + Some(&options), + None, + ); + assert!(!result.has_errors(), "got: {:?}", result.diagnostics); + let code: String = result.code.chars().filter(|c| !c.is_whitespace()).collect(); + assert!( + code.contains("transform:(v)=>({value:v}).value"), + "partial-mode metadata must keep parens, got:\n{code}" + ); +} + +// --------------------------------------------------------------------------- +// Template pipeline: strip_nonrequired_parentheses must drop source parens +// except the ones JavaScript grammar requires — the emitter now prints any +// Parenthesized that survives, so a missed strip or a missed required mark +// both show in the output. +// --------------------------------------------------------------------------- + +/// Every unary-precedence operator is a forbidden `**` base — `!x ** 2`, +/// `typeof x ** 2`, `void x ** 2` are SyntaxErrors, like `-x ** 2`. Upstream's +/// strip phase only keeps `UnaryOperatorExpr` parens because its emit goes +/// through the TS printer; we emit JS directly, so all four must survive. +#[test] +fn template_unary_bases_of_exponentiation_keep_parens() { + for (tpl, needle) in [ + (r"
{{ (-1) ** 2 }}
", "((-1)**2)"), + (r"
{{ (!a) ** 2 }}
", "((!ctx.a)**2)"), + (r"
{{ (typeof v) ** 2 }}
", "((typeofctx.v)**2)"), + (r"
{{ (void 0) ** 2 }}
", "((void0)**2)"), + // Nested parens: the inner wrapper is required and survives the strip. + (r"
{{ ((-1)) ** 2 }}
", "((-1)**2)"), + ] { + let code = compile_tpl(tpl); + assert!(code.contains(needle), "{tpl} emitted:\n{code}"); + } +} + +/// Non-required parens in templates strip — `(x)`, `(pipe) || b`, and a +/// parenthesized safe-read receiver emit without them (upstream parity). +#[test] +fn template_nonrequired_parens_strip() { + let code = compile_tpl(r"
{{ (x) }}
"); + assert!(code.contains("Interpolate(ctx.x)"), "got:\n{code}"); + + let code = compile_tpl(r"
{{ ((data$ | async) || fb)?.name }}
"); + assert!( + code.contains("(i0.ɵɵpipeBind1(2,1,ctx.data$)||ctx.fb)"), + "pipe parens must strip, got:\n{code}" + ); + assert!( + !code.contains("(i0.ɵɵpipeBind1(2,1,ctx.data$))||"), + "no extra parens around the pipe, got:\n{code}" + ); +} + +/// `??`/`&&`/`||`/`?:` mixes keep their required parens (TS-compat hazards). +#[test] +fn template_nullish_and_logical_parens_survive() { + let code = compile_tpl(r"
{{ (a ?? b) && c }}
"); + assert!(code.contains("((ctx.a??ctx.b)&&ctx.c)"), "got:\n{code}"); + + let code = compile_tpl(r"
{{ (a ? b : c) ?? d }}
"); + assert!(code.contains("??ctx.d"), "got:\n{code}"); + assert!(code.contains("((ctx.a?ctx.b:ctx.c)??ctx.d)"), "ternary ?? parens, got:\n{code}"); +}