From d470ed70cb8574060654215c3932e9f4ebee6407 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Tue, 24 Feb 2026 13:18:04 +0800 Subject: [PATCH] fix(angular): fix 5 compiler divergences from Angular reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. (High) @let self-reference: mark name as declared AFTER transforming the declaration op, so self-references like `@let x = x + 1` are replaced with `undefined` — matching Angular's backward walk. 2. (Medium) @for invalid-shape recovery: return None when expression parsing fails or track is missing, matching Angular which returns null node instead of synthesizing a broken ForLoopBlock. 3. (Medium) @if invalid main-parameter recovery: only push the main branch (and @else if branches) when the expression parses successfully, matching Angular's conditional push. 4. (Medium) @switch invalid parameter count: parse the first parameter when present (even with extra params), only use empty binding when there are no parameters — matching Angular's createSwitchBlock. 5. (Low) Reify conditional contract: replace panic with OxcDiagnostic::error for missing processed expression, matching the StoreLet precedent in the same file and the project convention of diagnostics over panics. Co-Authored-By: Claude Opus 4.6 --- .../src/pipeline/phases/reify/mod.rs | 13 +- .../phases/remove_illegal_let_references.rs | 32 +++- .../src/transform/html_to_r3.rs | 158 ++++++++++-------- .../tests/integration_test.rs | 117 ++++++++++++- .../tests/r3_template_transform_test.rs | 24 ++- .../integration_test__let_self_reference.snap | 17 ++ 6 files changed, 271 insertions(+), 90 deletions(-) create mode 100644 crates/oxc_angular_compiler/tests/snapshots/integration_test__let_self_reference.snap diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs b/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs index c712fa3c6c..58d6ecbed5 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/reify/mod.rs @@ -829,16 +829,15 @@ fn reify_update_op<'a>( Some(create_repeater_stmt(allocator, expr)) } UpdateOp::Conditional(cond) => { - // Use processed expression (built by conditionals phase) or fall back to test + // Use processed expression (built by conditionals phase). + // Angular asserts that processed is always set by this point + // (throws "Conditional test was not set." in reify.ts:698). let expr = if let Some(ref processed) = cond.processed { convert_ir_expression(allocator, processed, expressions, root_xref) - } else if let Some(ref test) = cond.test { - convert_ir_expression(allocator, test, expressions, root_xref) } else { - OutputExpression::Literal(Box::new_in( - LiteralExpr { value: LiteralValue::Null, source_span: None }, - allocator, - )) + diagnostics + .push(OxcDiagnostic::error("AssertionError: Conditional test was not set.")); + return None; }; let context_value = cond .context_value diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/remove_illegal_let_references.rs b/crates/oxc_angular_compiler/src/pipeline/phases/remove_illegal_let_references.rs index 90e99f6b2d..db2167f6d8 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/remove_illegal_let_references.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/remove_illegal_let_references.rs @@ -73,22 +73,32 @@ pub fn remove_illegal_let_references(job: &mut ComponentCompilationJob<'_>) { None => continue, }; - // We process by iterating and tracking which let names have been "declared" - // A let name is "declared" when we encounter its Variable op + // We process by iterating and tracking which let names have been "declared". + // A let name is "declared" AFTER we finish transforming its Variable op. + // This matches Angular which walks backward from the declaration op itself, + // replacing self-references (e.g. `@let x = x + 1`) with `undefined`. let mut declared_names: Vec> = Vec::new(); for op in view.update.iter_mut() { - // Check if this is a Variable op that declares a let - if let UpdateOp::Variable(var) = op { + // Check if this op declares a let variable — extract the name before + // transforming so we can mark it as declared AFTER the transform. + let newly_declared = if let UpdateOp::Variable(var) = &*op { if var.kind == SemanticVariableKind::Identifier { if let IrExpression::StoreLet(_) = var.initializer.as_ref() { - // Mark this name as declared (after this point, refs are legal) - declared_names.push(var.name.clone()); + Some(var.name.clone()) + } else { + None } + } else { + None } - } + } else { + None + }; - // Replace any LexicalRead with undeclared let names with undefined + // Replace any LexicalRead with undeclared let names with undefined. + // This runs BEFORE marking the current op's name as declared, so + // self-references in the declaration op are also replaced. let let_names_ref = &let_names; let declared_ref = &declared_names; @@ -113,6 +123,12 @@ pub fn remove_illegal_let_references(job: &mut ComponentCompilationJob<'_>) { }, VisitorContextFlag::NONE, ); + + // Mark this name as declared AFTER transforming, so subsequent ops + // can legally reference it, but the declaration op itself cannot. + if let Some(name) = newly_declared { + declared_names.push(name); + } } } } diff --git a/crates/oxc_angular_compiler/src/transform/html_to_r3.rs b/crates/oxc_angular_compiler/src/transform/html_to_r3.rs index 4700a07197..70b5ec9a0b 100644 --- a/crates/oxc_angular_compiler/src/transform/html_to_r3.rs +++ b/crates/oxc_angular_compiler/src/transform/html_to_r3.rs @@ -10,7 +10,7 @@ use oxc_span::{Atom, Span}; use rustc_hash::{FxHashMap, FxHashSet}; use crate::ast::expression::{ - ASTWithSource, AbsoluteSourceSpan, AngularExpression, BindingType, ParseSpan, ParsedEventType, + AbsoluteSourceSpan, AngularExpression, BindingType, ParseSpan, ParsedEventType, }; use crate::ast::html::{ BlockType, HtmlAttribute, HtmlBlock, HtmlComponent, HtmlDirective, HtmlElement, HtmlExpansion, @@ -2025,30 +2025,38 @@ impl<'a> HtmlToR3Transform<'a> { }); } - let children = self.visit_children(&block.children); + // Match Angular's createIfBlock: only push the main branch when + // parseConditionalBlockParameters succeeds (returns non-null). + // When parameters are empty, Angular returns null and skips the branch. + if main_params.expression.is_some() { + let children = self.visit_children(&block.children); - // Create i18n placeholder if inside an i18n context - let i18n = - self.create_block_placeholder("if", &[], block.span, block.start_span, block.end_span); + // Create i18n placeholder if inside an i18n context + let i18n = self.create_block_placeholder( + "if", + &[], + block.span, + block.start_span, + block.end_span, + ); - let main_branch = R3IfBlockBranch { - expression: main_params.expression.map(|e| e.ast), - children, - expression_alias: main_params.expression_alias, - source_span: block.span, - start_source_span: block.start_span, - end_source_span: block.end_span, - name_span: block.name_span, - i18n, - }; - branches.push(main_branch); + let main_branch = R3IfBlockBranch { + expression: main_params.expression.map(|e| e.ast), + children, + expression_alias: main_params.expression_alias, + source_span: block.span, + start_source_span: block.start_span, + end_source_span: block.end_span, + name_span: block.name_span, + i18n, + }; + branches.push(main_branch); + } // Validate connected blocks and process @else if and @else blocks let mut has_else = false; for (i, connected) in connected_blocks.iter().enumerate() { - let children = self.visit_children(&connected.children); - match connected.block_type { BlockType::ElseIf => { // Parse @else if parameters (condition and optional "as" alias) @@ -2088,26 +2096,32 @@ impl<'a> HtmlToR3Transform<'a> { }); } - // Create i18n placeholder if inside an i18n context - let i18n = self.create_block_placeholder( - "else if", - &[], - connected.span, - connected.start_span, - connected.end_span, - ); + // Match Angular: only push the branch when params are non-null + // (i.e., when an expression was successfully parsed). + if params.expression.is_some() { + let children = self.visit_children(&connected.children); - let branch = R3IfBlockBranch { - expression: params.expression.map(|e| e.ast), - children, - expression_alias: params.expression_alias, - source_span: connected.span, - start_source_span: connected.start_span, - end_source_span: connected.end_span, - name_span: connected.name_span, - i18n, - }; - branches.push(branch); + // Create i18n placeholder if inside an i18n context + let i18n = self.create_block_placeholder( + "else if", + &[], + connected.span, + connected.start_span, + connected.end_span, + ); + + let branch = R3IfBlockBranch { + expression: params.expression.map(|e| e.ast), + children, + expression_alias: params.expression_alias, + source_span: connected.span, + start_source_span: connected.start_span, + end_source_span: connected.end_span, + name_span: connected.name_span, + i18n, + }; + branches.push(branch); + } } BlockType::Else => { // Validation: check for duplicate @else @@ -2133,7 +2147,12 @@ impl<'a> HtmlToR3Transform<'a> { } has_else = true; + // @else has no condition (null expression) and no alias + let children = self.visit_children(&connected.children); + // Create i18n placeholder if inside an i18n context + // (must be after visit_children to match @if/@else if ordering + // and preserve i18n placeholder numbering) let i18n = self.create_block_placeholder( "else", &[], @@ -2141,8 +2160,6 @@ impl<'a> HtmlToR3Transform<'a> { connected.start_span, connected.end_span, ); - - // @else has no condition (null expression) and no alias let branch = R3IfBlockBranch { expression: None, children, @@ -2233,32 +2250,8 @@ impl<'a> HtmlToR3Transform<'a> { }); } - let item = params.item; - let expression = params.expression; - let context_variables = params.context_variables; - - // Get track expression or create empty one for error recovery. - // Only report missing-track error if the expression itself parsed successfully - // (matching Angular which returns null params and skips track validation on parse failure). - let (track_by, track_keyword_span) = if let Some(track_info) = params.track_by { - (track_info.expression, track_info.keyword_span) - } else { - if !expression_parse_failed { - // Track is required but missing - report error and create empty for error recovery - self.report_error("@for loop must have a \"track\" expression", block.start_span); - } - let empty_ast = ASTWithSource { - ast: self.create_empty_expression(block.span), - source: None, - location: Atom::from(""), - absolute_offset: block.span.start, - }; - (empty_ast, block.name_span) - }; - - let children = self.visit_children(&block.children); - - // Process and validate connected @empty block + // Process and validate connected @empty block. + // Angular processes connected blocks before checking params, so we do too. let mut empty: Option> = None; for connected in connected_blocks { @@ -2304,6 +2297,28 @@ impl<'a> HtmlToR3Transform<'a> { } } + // Match Angular's createForLoop: if expression parsing failed entirely + // (params === null in Angular), return None — no ForLoopBlock is emitted. + if expression_parse_failed { + return None; + } + + let item = params.item; + let expression = params.expression; + let context_variables = params.context_variables; + + // Match Angular's createForLoop: if track is missing and expression parsed OK, + // report the error and return None (Angular only creates the node when + // params !== null AND params.trackBy !== null). + let (track_by, track_keyword_span) = if let Some(track_info) = params.track_by { + (track_info.expression, track_info.keyword_span) + } else { + self.report_error("@for loop must have a \"track\" expression", block.start_span); + return None; + }; + + let children = self.visit_children(&block.children); + // Calculate the outer span to encompass @empty block if present let end_source_span = if let Some(last) = connected_blocks.last() { last.end_span } else { block.end_span }; @@ -2349,14 +2364,19 @@ impl<'a> HtmlToR3Transform<'a> { use crate::ast::html::{BlockType, HtmlNode}; use crate::ast::r3::{R3SwitchBlockCase, R3SwitchBlockCaseGroup}; - // Validation: @switch must have exactly one parameter - let expression = if block.parameters.len() == 1 { + // Validation: @switch must have exactly one parameter. + // Match Angular's createSwitchBlock: always parse the first parameter when present + // (even if there are extra parameters), only use empty when there are no parameters. + // Validation errors are reported by validateSwitchBlock in Angular; we report inline. + if block.parameters.len() != 1 { + self.report_error("@switch block must have exactly one parameter", block.start_span); + } + let expression = if !block.parameters.is_empty() { let expr_str = block.parameters[0].expression.as_str(); let parsed = self.binding_parser.parse_binding(expr_str, block.parameters[0].span); parsed.ast } else { - self.report_error("@switch block must have exactly one parameter", block.start_span); - self.create_empty_expression(block.span) + self.binding_parser.parse_binding("", block.span).ast }; let mut groups = Vec::new_in(self.allocator); diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index 07eec55ef7..4cbbfc904a 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -5,7 +5,7 @@ use oxc_allocator::Allocator; use oxc_angular_compiler::{ - AngularVersion, ResolvedResources, TransformOptions as ComponentTransformOptions, + AngularVersion, R3Node, ResolvedResources, TransformOptions as ComponentTransformOptions, output::ast::FunctionExpr, output::emitter::JsEmitter, parser::html::HtmlParser, @@ -741,6 +741,20 @@ fn test_let_with_pipe_multiple_in_child_view_varoffset() { insta::assert_snapshot!("let_with_pipe_multiple_in_child_view_varoffset", js); } +// ============================================================================ +// @let self-reference / forward-reference Tests +// ============================================================================ + +#[test] +fn test_let_self_reference_replaced_with_undefined() { + // A @let that references itself (self-reference) should have that reference + // replaced with `undefined`, matching Angular's behavior. + // Angular walks backward from the declaration op and replaces LexicalRead + // in the declaration op itself. + let js = compile_template_to_js(r"@let x = x + 1;
{{x}}
", "TestComponent"); + insta::assert_snapshot!("let_self_reference", js); +} + // ============================================================================ // ng-content Tests // ============================================================================ @@ -5204,3 +5218,104 @@ fn test_for_loop_multiple_index_aliases_in_track() { "Track expression should rewrite all $index aliases, but `j` was not rewritten.\nGenerated JS:\n{js}" ); } + +// ============================================================================ +// Error Recovery Conformance Tests (R3 Transform Level) +// ============================================================================ + +/// Transforms an Angular template to R3 AST and returns the nodes + error messages. +/// Unlike `compile_template_to_js`, this does NOT panic on parse/transform errors. +fn transform_to_r3(template: &str) -> (std::vec::Vec, bool) { + let allocator = Allocator::default(); + + let parser = HtmlParser::with_expansion_forms(&allocator, template, "test.html"); + let html_result = parser.parse(); + + let mut errors: std::vec::Vec = + html_result.errors.iter().map(|e| e.msg.clone()).collect(); + + let transformer = HtmlToR3Transform::new(&allocator, template, TransformOptions::default()); + let r3_result = transformer.transform(&html_result.nodes); + + errors.extend(r3_result.errors.iter().map(|e| e.msg.clone())); + + // Check if any ForLoopBlock nodes exist in the result + let has_for_block = r3_result.nodes.iter().any(|n| matches!(n, R3Node::ForLoopBlock(_))); + (errors, has_for_block) +} + +/// Returns (errors, r3_nodes_debug) for deeper node inspection. +fn transform_to_r3_nodes(template: &str) -> (std::vec::Vec, std::vec::Vec) { + let allocator = Allocator::default(); + + let parser = HtmlParser::with_expansion_forms(&allocator, template, "test.html"); + let html_result = parser.parse(); + + let mut errors: std::vec::Vec = + html_result.errors.iter().map(|e| e.msg.clone()).collect(); + + let transformer = HtmlToR3Transform::new(&allocator, template, TransformOptions::default()); + let r3_result = transformer.transform(&html_result.nodes); + + errors.extend(r3_result.errors.iter().map(|e| e.msg.clone())); + + let node_types: std::vec::Vec = r3_result + .nodes + .iter() + .map(|n| match n { + R3Node::ForLoopBlock(_) => "ForLoopBlock".to_string(), + R3Node::IfBlock(b) => format!("IfBlock(branches={})", b.branches.len()), + R3Node::SwitchBlock(_) => "SwitchBlock".to_string(), + R3Node::Text(_) => "Text".to_string(), + R3Node::Element(_) => "Element".to_string(), + R3Node::BoundText(_) => "BoundText".to_string(), + other => format!("{other:?}").chars().take(30).collect(), + }) + .collect(); + + (errors, node_types) +} + +#[test] +fn test_for_block_no_expression_returns_none() { + // Finding 2: @for with no expression should return None (no ForLoopBlock node), + // matching Angular's behavior where parseForLoopParameters returns null. + let (errors, has_for_block) = transform_to_r3("@for {
}"); + assert!( + !has_for_block, + "Angular returns null node when @for expression fails to parse, but Rust emitted a ForLoopBlock" + ); + assert!(!errors.is_empty(), "Should report a parse error for @for without expression"); +} + +#[test] +fn test_for_block_missing_track_returns_none() { + // Finding 2: @for with valid expression but missing track should return None, + // matching Angular's behavior (params.trackBy === null → node stays null). + let (errors, has_for_block) = transform_to_r3("@for (item of items) {
}"); + assert!( + !has_for_block, + "Angular returns null node when @for has no track expression, but Rust emitted a ForLoopBlock" + ); + assert!( + errors.iter().any(|e| e.contains("track")), + "Should report an error about missing track expression. Errors: {errors:?}" + ); +} + +#[test] +fn test_if_block_no_expression_skips_main_branch() { + // Finding 3: @if with no parameters should not push a main branch, + // matching Angular where parseConditionalBlockParameters returns null. + let (errors, node_types) = transform_to_r3_nodes("@if {
}"); + // The IfBlock should have 0 branches (main branch skipped) + for node_type in &node_types { + if node_type.starts_with("IfBlock") { + assert_eq!( + node_type, "IfBlock(branches=0)", + "Angular skips the main branch when parameters are missing, expected 0 branches" + ); + } + } + assert!(!errors.is_empty(), "Should report a parse error for @if without expression"); +} diff --git a/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs b/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs index 29326f6791..386d22654b 100644 --- a/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs +++ b/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs @@ -1644,18 +1644,32 @@ mod error_recovery_ast_shape { #[test] fn if_block_with_missing_expression_should_still_produce_ast() { // `@if {content}` - missing expression - // Oxc produces an IfBlock node for error recovery + // Angular creates an IfBlock with 0 branches (main branch skipped + // because parseConditionalBlockParameters returns null). let result = humanize_ignore_errors("@if {content}"); - // We don't need to match Angular exactly here - just verify no crash - // and that the AST is reasonable for error recovery + // The IfBlock node is still emitted (with 0 branches) — verify no crash. assert!(!result.is_empty()); } #[test] - fn for_block_with_missing_params_should_still_produce_ast() { + fn for_block_with_missing_params_returns_no_node() { // `@for () {content}` - missing parameters + // Angular's createForLoop returns null node when parseForLoopParameters + // fails (expression doesn't match " of "). let result = humanize_ignore_errors("@for () {content}"); - assert!(!result.is_empty()); + // Angular returns null node, so no ForLoopBlock should appear. + let has_for = result.iter().any(|r| { + r.first() + .map(|v| match v { + HumanValue::Str(s) => s == "ForLoopBlock", + _ => false, + }) + .unwrap_or(false) + }); + assert!( + !has_for, + "Angular returns null for @for with invalid params, but Rust produced a ForLoopBlock" + ); } } diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_self_reference.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_self_reference.snap new file mode 100644 index 0000000000..a1a204ccda --- /dev/null +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__let_self_reference.snap @@ -0,0 +1,17 @@ +--- +source: crates/oxc_angular_compiler/tests/integration_test.rs +expression: js +--- +function TestComponent_Template(rf,ctx) { + if ((rf & 1)) { + i0.ɵɵtext(0," "); + i0.ɵɵelementStart(1,"div"); + i0.ɵɵtext(2); + i0.ɵɵelementEnd(); + } + if ((rf & 2)) { + const x_r1 = (undefined + 1); + i0.ɵɵadvance(2); + i0.ɵɵtextInterpolate(x_r1); + } +}