From 84845ee5db42f8dbcab772332eb610e44a1b785e Mon Sep 17 00:00:00 2001 From: LongYinan Date: Sat, 21 Feb 2026 23:11:34 +0800 Subject: [PATCH] fix(angular): fix 6 compiler divergences from Angular reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Add missing @switch validation: report errors for non-block children, enforce exactly one @case parameter, single @default, and no @default parameters 2. Carry viewport trigger options through ingest into DeferOnOp IR 3. Change generateArrowFunctions phase to Kind::Both with host support 4. Add view.functions traversal to next_context_merging phase 5. Emit null arg for unresolved target slots in defer reify (viewport, interaction, hover triggers) 6. Add debug_assert for unexpected multi-line arrow functions Item 5 from the review (@switch @default reorder) was verified as a false positive: Angular does reorder @default last, just not in ingestSwitchBlock — the generateConditionalExpressions phase splices @default out as the ternary fallback. The existing Rust reorder is correct; updated the doc comment to explain why. Co-Authored-By: Claude Opus 4.6 --- .../src/pipeline/ingest.rs | 21 ++-- .../phases/generate_arrow_functions.rs | 39 ++++++- .../src/pipeline/phases/mod.rs | 6 +- .../pipeline/phases/next_context_merging.rs | 7 ++ .../pipeline/phases/reify/statements/defer.rs | 53 +++++---- .../src/transform/html_to_r3.rs | 61 ++++++++-- .../tests/integration_test.rs | 4 +- .../tests/r3_template_transform_test.rs | 109 ++++++++++++++++++ ...oading_timer_consts_after_i18n_consts.snap | 2 +- .../integration_test__defer_on_viewport.snap | 2 +- 10 files changed, 258 insertions(+), 46 deletions(-) diff --git a/crates/oxc_angular_compiler/src/pipeline/ingest.rs b/crates/oxc_angular_compiler/src/pipeline/ingest.rs index 273bacb10..a5a25d2a7 100644 --- a/crates/oxc_angular_compiler/src/pipeline/ingest.rs +++ b/crates/oxc_angular_compiler/src/pipeline/ingest.rs @@ -2884,9 +2884,11 @@ fn create_binary_modulo<'a>( /// Creates one CREATE op per case (ConditionalOp for first, ConditionalBranchCreateOp for rest) /// and one UPDATE op (ConditionalUpdateOp) containing all conditions. /// -/// IMPORTANT: Angular always processes @default LAST, regardless of where it appears in -/// the template. This affects slot allocation and function naming. To match Angular's behavior, -/// we reorder the groups to put the @default group at the end before processing. +/// Angular's `ingestSwitchBlock` in `ingest.ts` iterates groups in source order, but the +/// `generateConditionalExpressions` phase later splices `@default` out and uses it as the +/// ternary fallback. Because the Rust pipeline's conditional codegen expects `@default` last, +/// we reorder here so that slot allocation, function naming, and the conditional expression +/// all match Angular's compiled output. /// /// Ported from Angular's `ingestSwitchBlock` in `ingest.ts`. fn ingest_switch_block<'a>( @@ -2904,15 +2906,17 @@ fn ingest_switch_block<'a>( // Convert the main switch expression as the test let test = convert_ast_to_ir(job, switch_block.expression); - // Reorder groups to put @default LAST, matching Angular's behavior. - // Angular always assigns higher slot numbers to @default regardless of template order. - // A group is considered the @default group if ALL its cases have expression: None. + // Reorder groups to put @default LAST, matching Angular's compiled output. + // While Angular's ingestSwitchBlock iterates in source order, the downstream + // generateConditionalExpressions phase (conditionals.ts) splices @default out and + // uses it as the ternary fallback base. Because slot allocation and function naming + // happen after ingest, moving @default last here ensures our xref/slot/function + // ordering matches Angular's final output. let mut groups_vec: std::vec::Vec<_> = switch_block.groups.into_iter().collect(); let default_idx = groups_vec.iter().position(|group| { !group.cases.is_empty() && group.cases.iter().all(|c| c.expression.is_none()) }); if let Some(idx) = default_idx { - // Move the default group to the end let default_group = groups_vec.remove(idx); groups_vec.push(default_group); } @@ -3432,6 +3436,7 @@ fn ingest_defer_triggers<'a>( // Handle viewport trigger if let Some(viewport_trigger) = triggers.viewport { + let options = viewport_trigger.options.map(|opts| convert_ast_to_ir(job, opts)); let op = CreateOp::DeferOn(DeferOnOp { base: CreateOpBase { source_span: Some(viewport_trigger.source_span), @@ -3446,7 +3451,7 @@ fn ingest_defer_triggers<'a>( target_slot_view_steps: None, target_name: viewport_trigger.reference, delay: None, - options: None, + options, }); if let Some(view) = job.view_mut(view_xref) { view.create.push(op); diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/generate_arrow_functions.rs b/crates/oxc_angular_compiler/src/pipeline/phases/generate_arrow_functions.rs index 88966254d..a1b0d6836 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/generate_arrow_functions.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/generate_arrow_functions.rs @@ -15,7 +15,7 @@ use crate::ir::ops::{CreateOp, Op}; use crate::output::ast::{ ArrowFunctionBody, ArrowFunctionExpr as OutputArrowFunctionExpr, FnParam, }; -use crate::pipeline::compilation::ComponentCompilationJob; +use crate::pipeline::compilation::{ComponentCompilationJob, HostBindingCompilationJob}; use oxc_allocator::{Box as AllocBox, Vec as AllocVec}; /// Finds arrow functions written by the user and converts them into @@ -123,7 +123,9 @@ fn convert_output_arrow_to_ir<'a>( // The expression syntax doesn't support multi-line arrow functions, // but the output AST does. We don't need to handle them here if // the user isn't able to write one. - // In TypeScript this throws an error, but we'll just skip it. + // Angular throws an assertion error here; we use debug_assert to + // catch any internal compiler bugs that produce this in debug builds. + debug_assert!(false, "unexpected multi-line arrow function in template expression"); return None; } }; @@ -143,6 +145,39 @@ fn convert_output_arrow_to_ir<'a>( }) } +/// Generate arrow functions for host binding compilation. +/// +/// Angular runs this phase for Kind.Both, meaning it applies to both +/// template and host compilations. Host bindings can contain arrow +/// function expressions (e.g., in @HostBinding values or event handlers). +pub fn generate_arrow_functions_for_host(job: &mut HostBindingCompilationJob<'_>) { + let allocator = job.allocator; + + // Process create operations (skip listeners) + for op in job.root.create.iter_mut() { + if !is_listener_op(op) { + transform_expressions_in_create_op( + op, + &|expr, flags| { + add_arrow_function(expr, allocator, flags); + }, + VisitorContextFlag::NONE, + ); + } + } + + // Process update operations + for op in job.root.update.iter_mut() { + transform_expressions_in_update_op( + op, + &|expr, flags| { + add_arrow_function(expr, allocator, flags); + }, + VisitorContextFlag::NONE, + ); + } +} + /// Collect arrow functions from a view's operations into its functions set. fn collect_arrow_functions_from_view( view: &mut crate::pipeline::compilation::ViewCompilationUnit<'_>, diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/mod.rs b/crates/oxc_angular_compiler/src/pipeline/phases/mod.rs index a9a1e0467..16c8dcb1c 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/mod.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/mod.rs @@ -252,11 +252,11 @@ pub static PHASES: &[Phase] = &[ run_host: None, name: "createVariadicPipes", }, - // Phase 21: generateArrowFunctions (Template only) + // Phase 21: generateArrowFunctions (Both) Phase { - kind: CompilationJobKind::Template, + kind: CompilationJobKind::Both, run: generate_arrow_functions::generate_arrow_functions, - run_host: None, + run_host: Some(generate_arrow_functions::generate_arrow_functions_for_host), name: "generateArrowFunctions", }, // Phase 22: generatePureLiteralStructures (Both) diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/next_context_merging.rs b/crates/oxc_angular_compiler/src/pipeline/phases/next_context_merging.rs index d85847852..60582c521 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/next_context_merging.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/next_context_merging.rs @@ -30,6 +30,13 @@ pub fn merge_next_context_expressions(job: &mut ComponentCompilationJob<'_>) { for xref in view_xrefs { if let Some(view) = job.view_mut(xref) { + // Merge in arrow function op lists (matches Angular's unit.functions traversal) + for fn_ptr in view.functions.iter() { + // SAFETY: These pointers are valid for the duration of the compilation + let arrow_fn = unsafe { &mut **fn_ptr }; + merge_next_contexts_in_handler_ops(&mut arrow_fn.ops); + } + // Merge in create ops for listeners for op in view.create.iter_mut() { match op { diff --git a/crates/oxc_angular_compiler/src/pipeline/phases/reify/statements/defer.rs b/crates/oxc_angular_compiler/src/pipeline/phases/reify/statements/defer.rs index 9f38999d2..9ef1a032f 100644 --- a/crates/oxc_angular_compiler/src/pipeline/phases/reify/statements/defer.rs +++ b/crates/oxc_angular_compiler/src/pipeline/phases/reify/statements/defer.rs @@ -225,12 +225,19 @@ pub fn create_defer_on_stmt<'a>( if let Some(opts) = options { args.push(opts); } - } else if let Some(slot) = target_slot { - // Regular/Prefetch viewport with explicit target: target_slot, target_slot_view_steps?, options? - args.push(OutputExpression::Literal(Box::new_in( - LiteralExpr { value: LiteralValue::Number(slot as f64), source_span: None }, - allocator, - ))); + } else { + // Always emit the first arg: slot number or null if unresolved. + // Angular: o.literal(op.trigger.targetSlot?.slot ?? null) + args.push(match target_slot { + Some(slot) => OutputExpression::Literal(Box::new_in( + LiteralExpr { value: LiteralValue::Number(slot as f64), source_span: None }, + allocator, + )), + None => OutputExpression::Literal(Box::new_in( + LiteralExpr { value: LiteralValue::Null, source_span: None }, + allocator, + )), + }); let view_steps = target_slot_view_steps.unwrap_or(0); if view_steps != 0 { @@ -253,28 +260,32 @@ pub fn create_defer_on_stmt<'a>( args.push(opts); } } - // No arguments when no explicit target is specified } DeferTriggerKind::Interaction | DeferTriggerKind::Hover => { // Hydrate triggers don't support targets if modifier != DeferOpModifierKind::Hydrate { - // Only push arguments if there's an explicit target - if let Some(slot) = target_slot { - args.push(OutputExpression::Literal(Box::new_in( + // Always emit the first arg: slot number or null if unresolved. + // Angular: o.literal(op.trigger.targetSlot?.slot ?? null) + args.push(match target_slot { + Some(slot) => OutputExpression::Literal(Box::new_in( LiteralExpr { value: LiteralValue::Number(slot as f64), source_span: None }, allocator, - ))); + )), + None => OutputExpression::Literal(Box::new_in( + LiteralExpr { value: LiteralValue::Null, source_span: None }, + allocator, + )), + }); - let view_steps = target_slot_view_steps.unwrap_or(0); - if view_steps != 0 { - args.push(OutputExpression::Literal(Box::new_in( - LiteralExpr { - value: LiteralValue::Number(view_steps as f64), - source_span: None, - }, - allocator, - ))); - } + let view_steps = target_slot_view_steps.unwrap_or(0); + if view_steps != 0 { + args.push(OutputExpression::Literal(Box::new_in( + LiteralExpr { + value: LiteralValue::Number(view_steps as f64), + source_span: None, + }, + allocator, + ))); } } } 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 1e80f1b46..61be71e5e 100644 --- a/crates/oxc_angular_compiler/src/transform/html_to_r3.rs +++ b/crates/oxc_angular_compiler/src/transform/html_to_r3.rs @@ -2313,21 +2313,36 @@ impl<'a> HtmlToR3Transform<'a> { let mut unknown_blocks = Vec::new_in(self.allocator); let mut collected_cases: std::vec::Vec> = std::vec::Vec::new(); let mut first_case_start: Option = None; + let mut has_default = false; for child in &block.children { - // Skip non-block nodes (only process block children) + // Skip comments and whitespace-only text nodes (same as Angular) + match child { + HtmlNode::Comment(_) => continue, + HtmlNode::Text(t) if t.value.trim().is_empty() => continue, + _ => {} + } + + // Non-block children (elements, non-whitespace text, etc.) are invalid let child_block = match child { HtmlNode::Block(b) => b, - _ => continue, + _ => { + self.report_error( + "@switch block can only contain @case and @default blocks", + child.span(), + ); + continue; + } }; - // Check if this is a valid case/default block - // Note: @case with no parameters is treated as unknown (same as Angular) - let is_case = - child_block.block_type == BlockType::Case && !child_block.parameters.is_empty(); - let is_default = child_block.block_type == BlockType::Default; - - if !is_case && !is_default { + // Validate: only @case and @default are allowed inside @switch + if child_block.block_type != BlockType::Case + && child_block.block_type != BlockType::Default + { + self.report_error( + "@switch block can only contain @case and @default blocks", + child_block.span, + ); unknown_blocks.push(crate::ast::r3::R3UnknownBlock { name: child_block.name.clone(), source_span: child_block.span, @@ -2336,6 +2351,34 @@ impl<'a> HtmlToR3Transform<'a> { continue; } + let is_default = child_block.block_type == BlockType::Default; + + // Validate @default + if is_default { + if has_default { + self.report_error( + "@switch block can only have one @default block", + child_block.start_span, + ); + } else if !child_block.parameters.is_empty() { + self.report_error( + "@default block cannot have parameters", + child_block.start_span, + ); + } + has_default = true; + } + + // Validate @case: must have exactly one parameter + let is_case = child_block.block_type == BlockType::Case; + if is_case && child_block.parameters.len() != 1 { + self.report_error( + "@case block must have exactly one parameter", + child_block.start_span, + ); + continue; + } + // Parse expression for @case blocks let case_expression = if is_case { let expr_str = child_block.parameters[0].expression.as_str(); diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index f5f92a2f4..2e697ad88 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -435,7 +435,9 @@ fn test_switch_block() { #[test] fn test_switch_block_default_first() { - // Test @switch with @default appearing first - Angular should reorder to put @default last + // Test @switch with @default appearing first - Angular reorders @default last + // Angular's ingestSwitchBlock iterates in source order, but generateConditionalExpressions + // splices @default out as the ternary fallback. We reorder at ingest to match the final output. let js = compile_template_to_js( r"@switch (value) { @default {
Other
} @case (1) {
One
} @case (2) {
Two
} }", "TestComponent", 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 6f8cdf2c1..87450f164 100644 --- a/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs +++ b/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs @@ -749,6 +749,22 @@ fn humanize_ignore_errors(html: &str) -> Vec> { humanizer.humanize(&result.nodes) } +/// Helper to get transform errors from an HTML template. +fn get_transform_errors(html: &str) -> Vec { + let allocator = Box::new(Allocator::default()); + let allocator_ref: &'static Allocator = + unsafe { &*std::ptr::from_ref::(allocator.as_ref()) }; + + let parser = HtmlParser::new(allocator_ref, html, "test.html"); + let html_result = parser.parse(); + + let options = TransformOptions { collect_comment_nodes: false }; + let transformer = HtmlToR3Transform::new(allocator_ref, html, options); + let r3_result = transformer.transform(&html_result.nodes); + + r3_result.errors.iter().map(|e| e.msg.clone()).collect() +} + // ============================================================================ // Tests: Nodes without binding // ============================================================================ @@ -1943,3 +1959,96 @@ mod complex_control_flow { } } } + +// ============================================================================ +// Tests: @switch validation +// ============================================================================ + +mod switch_validation { + use super::*; + + #[test] + fn should_report_error_for_non_block_children_in_switch() { + // Angular reports: "@switch block can only contain @case and @default blocks" + // for non-whitespace text and element children + let errors = get_transform_errors("@switch (expr) {
invalid
@case (1) { a } }"); + assert!( + errors + .iter() + .any(|e| e.contains("@switch block can only contain @case and @default blocks")), + "Expected error about non-block children in @switch, got: {errors:?}" + ); + } + + #[test] + fn should_report_error_for_non_whitespace_text_in_switch() { + let errors = get_transform_errors("@switch (expr) { some text @case (1) { a } }"); + assert!( + errors + .iter() + .any(|e| e.contains("@switch block can only contain @case and @default blocks")), + "Expected error about non-block text in @switch, got: {errors:?}" + ); + } + + #[test] + fn should_allow_whitespace_text_in_switch() { + // Whitespace-only text nodes should be silently skipped (same as Angular) + let errors = get_transform_errors("@switch (expr) { \n @case (1) { a } }"); + assert!( + errors.is_empty(), + "Expected no errors for whitespace-only text in @switch, got: {errors:?}" + ); + } + + #[test] + fn should_allow_comments_in_switch() { + // Comments should be silently skipped (same as Angular) + let errors = get_transform_errors("@switch (expr) { @case (1) { a } }"); + assert!(errors.is_empty(), "Expected no errors for comments in @switch, got: {errors:?}"); + } + + #[test] + fn should_report_error_for_case_with_multiple_parameters() { + // Angular: "@case block must have exactly one parameter" + // Note: the HTML parser may or may not parse this as a valid @case block depending + // on how parameters are tokenized. Either way, an error should be reported. + let errors = get_transform_errors("@switch (expr) { @case (1) (2) { a } }"); + assert!( + !errors.is_empty(), + "Expected errors for @case with multiple parameters, got: {errors:?}" + ); + } + + #[test] + fn should_report_error_for_case_with_no_parameters() { + // Angular: "@case block must have exactly one parameter" + let errors = get_transform_errors("@switch (expr) { @case { a } }"); + assert!( + errors.iter().any(|e| e.contains("@case block must have exactly one parameter")), + "Expected error about @case parameters, got: {errors:?}" + ); + } + + #[test] + fn should_report_error_for_multiple_default_blocks() { + // Angular: "@switch block can only have one @default block" + let errors = get_transform_errors( + "@switch (expr) { @case (1) { a } @default { b } @default { c } }", + ); + assert!( + errors.iter().any(|e| e.contains("@switch block can only have one @default block")), + "Expected error about multiple @default blocks, got: {errors:?}" + ); + } + + #[test] + fn should_report_error_for_default_with_parameters() { + // Angular: "@default block cannot have parameters" + let errors = get_transform_errors("@switch (expr) { @case (1) { a } @default (x) { b } }"); + assert!( + errors.iter().any(|e| e.contains("@default block cannot have parameters")), + "Expected error about @default with parameters, got: {errors:?}" + ); + } +} diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_loading_timer_consts_after_i18n_consts.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_loading_timer_consts_after_i18n_consts.snap index 0491aebeb..30a04153e 100644 --- a/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_loading_timer_consts_after_i18n_consts.snap +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_loading_timer_consts_after_i18n_consts.snap @@ -29,7 +29,7 @@ function TestComponent_Template(rf,ctx) { i0.ɵɵdomTemplate(3,TestComponent_Defer_3_Template,4,0)(4,TestComponent_DeferLoading_4_Template, 4,0); i0.ɵɵdefer(5,3,null,4,null,null,1,null,i0.ɵɵdeferEnableTimerScheduling); - i0.ɵɵdeferOnViewport(); + i0.ɵɵdeferOnViewport(null); i0.ɵɵdeferPrefetchOnIdle(); } } diff --git a/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_on_viewport.snap b/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_on_viewport.snap index 5f6b7a34c..e542d048f 100644 --- a/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_on_viewport.snap +++ b/crates/oxc_angular_compiler/tests/snapshots/integration_test__defer_on_viewport.snap @@ -13,6 +13,6 @@ function TestComponent_Template(rf,ctx) { if ((rf & 1)) { i0.ɵɵdomTemplate(0,TestComponent_Defer_0_Template,3,0); i0.ɵɵdefer(1,0); - i0.ɵɵdeferOnViewport(); + i0.ɵɵdeferOnViewport(null); } }