From b4f29140757c0307d66a9e0e40baaf5ddab67c3f Mon Sep 17 00:00:00 2001 From: LongYinan Date: Sun, 22 Feb 2026 13:09:02 +0800 Subject: [PATCH] fix(angular): fix 5 compiler divergences from Angular reference 1. Standalone connected blocks (@else, @empty, @placeholder, @loading, @error) now emit error diagnostics and produce UnknownBlock nodes instead of being silently dropped. 2. @defer connected block validation now rejects duplicate @placeholder/ @loading/@error, @error with parameters, and unrecognized params in @placeholder/@loading blocks. 3. @switch case groups now propagate i18n metadata via create_block_placeholder() instead of hardcoding None. 4. parse_track_expression now returns empty string for bare "track " so the downstream EmptyExpr check produces the correct diagnostic. 5. Comments between connected blocks are no longer marked as processed, matching Angular's findConnectedBlocks behavior. Co-Authored-By: Claude Opus 4.6 --- .../src/transform/control_flow.rs | 9 +- .../src/transform/html_to_r3.rs | 316 ++++++++++++++---- .../src/util/deferred_time.rs | 81 ----- .../tests/r3_template_transform_test.rs | 179 ++++++++++ 4 files changed, 429 insertions(+), 156 deletions(-) diff --git a/crates/oxc_angular_compiler/src/transform/control_flow.rs b/crates/oxc_angular_compiler/src/transform/control_flow.rs index 3883565a8d..ffe522248f 100644 --- a/crates/oxc_angular_compiler/src/transform/control_flow.rs +++ b/crates/oxc_angular_compiler/src/transform/control_flow.rs @@ -99,8 +99,10 @@ fn parse_for_of_expression(s: &str) -> Option<(&str, &str)> { Some((var_name, expression)) } -/// Parse "track" expression pattern: `^track\s+([\S\s]+)` -/// Returns the expression after "track " if matched. +/// Parse "track" expression pattern: `^track\s+([\S\s]*)` +/// Returns the expression after "track " if matched, which may be empty. +/// An empty expression is valid as a match (Angular handles this by checking EmptyExpr). +/// Reference: r3_control_flow.ts FOR_LOOP_TRACK_PATTERN = /^track\s+([\S\s]*)/ fn parse_track_expression(s: &str) -> Option<&str> { if !s.starts_with("track") { return None; @@ -110,9 +112,6 @@ fn parse_track_expression(s: &str) -> Option<&str> { return None; } let expr = after_track.trim_start(); - if expr.is_empty() { - return None; - } Some(expr) } 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 d63493c0a9..b3b0456af8 100644 --- a/crates/oxc_angular_compiler/src/transform/html_to_r3.rs +++ b/crates/oxc_angular_compiler/src/transform/html_to_r3.rs @@ -30,7 +30,7 @@ use crate::parser::expression::{BindingParser, find_comment_start}; use crate::parser::html::decode_entities_in_string; use crate::schema::get_security_context; use crate::transform::control_flow::{parse_conditional_params, parse_defer_triggers}; -use crate::util::{ParseError, parse_loading_parameters, parse_placeholder_parameters}; +use crate::util::ParseError; /// Regex pattern for binding prefixes. /// Matches: bind-, let-, ref-/#, on-, bindon-, @ @@ -1721,8 +1721,23 @@ impl<'a> HtmlToR3Transform<'a> { self.visit_if_block(block, &connected) } BlockType::Else | BlockType::ElseIf => { - // These are handled as part of @if - None + // Standalone @else/@else if without a preceding @if - emit error and create UnknownBlock + // Reference: r3_template_transform.ts:515-517 + self.report_error( + &format!( + "@{} block can only be used after an @if or @else if block.", + block.name + ), + block.span, + ); + Some(R3Node::UnknownBlock(Box::new_in( + crate::ast::r3::R3UnknownBlock { + name: block.name.clone(), + source_span: block.span, + name_span: block.name_span, + }, + self.allocator, + ))) } BlockType::For => { // First, find and collect connected block indices and whitespace @@ -1754,8 +1769,17 @@ impl<'a> HtmlToR3Transform<'a> { self.visit_for_block(block, &connected) } BlockType::Empty => { - // Handled as part of @for - None + // Standalone @empty without a preceding @for - emit error and create UnknownBlock + // Reference: r3_template_transform.ts:512-514 + self.report_error("@empty block can only be used after an @for block.", block.span); + Some(R3Node::UnknownBlock(Box::new_in( + crate::ast::r3::R3UnknownBlock { + name: block.name.clone(), + source_span: block.span, + name_span: block.name_span, + }, + self.allocator, + ))) } BlockType::Switch => self.visit_switch_block(block), BlockType::Case | BlockType::Default => { @@ -1792,8 +1816,20 @@ impl<'a> HtmlToR3Transform<'a> { self.visit_defer_block(block, &connected) } BlockType::Placeholder | BlockType::Loading | BlockType::Error => { - // Handled as part of @defer - None + // Standalone @placeholder/@loading/@error without a preceding @defer - emit error and create UnknownBlock + // Reference: r3_template_transform.ts:509-511 + self.report_error( + &format!("@{} block can only be used after an @defer block.", block.name), + block.span, + ); + Some(R3Node::UnknownBlock(Box::new_in( + crate::ast::r3::R3UnknownBlock { + name: block.name.clone(), + source_span: block.span, + name_span: block.name_span, + }, + self.allocator, + ))) } } } @@ -1814,9 +1850,10 @@ impl<'a> HtmlToR3Transform<'a> { for i in (primary_index + 1)..siblings.len() { let node = &siblings[i]; - // Skip over comments and mark them as consumed + // Skip over comments without marking them as processed + // Reference: r3_template_transform.ts:543-546 - Angular skips comments + // but does NOT add them to processedNodes if matches!(node, HtmlNode::Comment(_)) { - whitespace_indices.push(i); continue; } @@ -2442,6 +2479,16 @@ impl<'a> HtmlToR3Transform<'a> { // Visit children of the final case (which has the body) let children = self.visit_children(&child_block.children); + // Propagate i18n metadata for the case group + // Reference: r3_control_flow.ts:293 - Angular passes node.i18n to SwitchBlockCaseGroup + let i18n = self.create_block_placeholder( + &child_block.name, + &[], + group_source_span, + group_start_source_span, + child_block.end_span, + ); + let group = R3SwitchBlockCaseGroup { cases, children, @@ -2449,7 +2496,7 @@ impl<'a> HtmlToR3Transform<'a> { start_source_span: group_start_source_span, end_source_span: child_block.end_span, name_span: child_block.name_span, - i18n: None, + i18n, }; groups.push(group); @@ -2513,7 +2560,8 @@ impl<'a> HtmlToR3Transform<'a> { }); } - // Process connected blocks + // Process connected blocks with validation + // Reference: r3_deferred_blocks.ts parseConnectedBlocks (lines 106-164) let mut placeholder = None; let mut loading = None; let mut error = None; @@ -2523,74 +2571,202 @@ impl<'a> HtmlToR3Transform<'a> { match connected.block_type { BlockType::Placeholder => { - // Parse minimum time from parameters like "minimum 500ms" - let params: std::vec::Vec<&str> = - connected.parameters.iter().map(|p| p.expression.as_str()).collect(); - let minimum_time = parse_placeholder_parameters(¶ms); + if placeholder.is_some() { + // Reference: r3_deferred_blocks.ts:124-131 + self.report_error( + "@defer block can only have one @placeholder block", + connected.start_span, + ); + } else { + // Parse and validate placeholder parameters + // Reference: r3_deferred_blocks.ts parsePlaceholderBlock (lines 167-198) + let mut minimum_time = None; + let mut has_minimum = false; + for param in &connected.parameters { + let expr = param.expression.as_str(); + if expr.trim_start().starts_with("minimum") + && expr.trim_start().get(7..8).is_none_or(|c| { + c.chars().next().is_none_or(char::is_whitespace) + }) + { + if has_minimum { + self.report_error( + "@placeholder block can only have one \"minimum\" parameter", + connected.start_span, + ); + } else { + has_minimum = true; + let time_str = + &expr[crate::util::get_trigger_parameters_start(expr)..]; + let parsed = crate::util::parse_deferred_time(time_str); + if parsed.is_none() { + self.report_error( + "Could not parse time value of parameter \"minimum\"", + param.span, + ); + } + minimum_time = parsed; + } + } else { + self.report_error( + &format!( + "Unrecognized parameter in @placeholder block: \"{}\"", + expr + ), + param.span, + ); + } + } - // Create i18n placeholder if inside an i18n context - let i18n = self.create_block_placeholder( - "placeholder", - &[], - connected.span, - connected.start_span, - connected.end_span, - ); + // Create i18n placeholder if inside an i18n context + let i18n = self.create_block_placeholder( + "placeholder", + &[], + connected.span, + connected.start_span, + connected.end_span, + ); - placeholder = Some(R3DeferredBlockPlaceholder { - children: connected_children, - minimum_time, - source_span: connected.span, - name_span: connected.name_span, - start_source_span: connected.start_span, - end_source_span: connected.end_span, - i18n, - }); + placeholder = Some(R3DeferredBlockPlaceholder { + children: connected_children, + minimum_time, + source_span: connected.span, + name_span: connected.name_span, + start_source_span: connected.start_span, + end_source_span: connected.end_span, + i18n, + }); + } } BlockType::Loading => { - // Parse after and minimum time from parameters like "after 100ms; minimum 1s" - let params: std::vec::Vec<&str> = - connected.parameters.iter().map(|p| p.expression.as_str()).collect(); - let (after_time, minimum_time) = parse_loading_parameters(¶ms); + if loading.is_some() { + // Reference: r3_deferred_blocks.ts:137-143 + self.report_error( + "@defer block can only have one @loading block", + connected.start_span, + ); + } else { + // Parse and validate loading parameters + // Reference: r3_deferred_blocks.ts parseLoadingBlock (lines 201-248) + let mut after_time = None; + let mut has_after = false; + let mut minimum_time = None; + let mut has_minimum = false; + for param in &connected.parameters { + let expr = param.expression.as_str(); + let trimmed = expr.trim_start(); + if trimmed.starts_with("after") + && trimmed.get(5..6).is_none_or(|c| { + c.chars().next().is_none_or(char::is_whitespace) + }) + { + if has_after { + self.report_error( + "@loading block can only have one \"after\" parameter", + connected.start_span, + ); + } else { + has_after = true; + let time_str = + &expr[crate::util::get_trigger_parameters_start(expr)..]; + let parsed = crate::util::parse_deferred_time(time_str); + if parsed.is_none() { + self.report_error( + "Could not parse time value of parameter \"after\"", + param.span, + ); + } + after_time = parsed; + } + } else if trimmed.starts_with("minimum") + && trimmed.get(7..8).is_none_or(|c| { + c.chars().next().is_none_or(char::is_whitespace) + }) + { + if has_minimum { + self.report_error( + "@loading block can only have one \"minimum\" parameter", + connected.start_span, + ); + } else { + has_minimum = true; + let time_str = + &expr[crate::util::get_trigger_parameters_start(expr)..]; + let parsed = crate::util::parse_deferred_time(time_str); + if parsed.is_none() { + self.report_error( + "Could not parse time value of parameter \"minimum\"", + param.span, + ); + } + minimum_time = parsed; + } + } else { + self.report_error( + &format!( + "Unrecognized parameter in @loading block: \"{}\"", + expr + ), + param.span, + ); + } + } - // Create i18n placeholder if inside an i18n context - let i18n = self.create_block_placeholder( - "loading", - &[], - connected.span, - connected.start_span, - connected.end_span, - ); + // Create i18n placeholder if inside an i18n context + let i18n = self.create_block_placeholder( + "loading", + &[], + connected.span, + connected.start_span, + connected.end_span, + ); - loading = Some(R3DeferredBlockLoading { - children: connected_children, - after_time, - minimum_time, - source_span: connected.span, - name_span: connected.name_span, - start_source_span: connected.start_span, - end_source_span: connected.end_span, - i18n, - }); + loading = Some(R3DeferredBlockLoading { + children: connected_children, + after_time, + minimum_time, + source_span: connected.span, + name_span: connected.name_span, + start_source_span: connected.start_span, + end_source_span: connected.end_span, + i18n, + }); + } } BlockType::Error => { - // Create i18n placeholder if inside an i18n context - let i18n = self.create_block_placeholder( - "error", - &[], - connected.span, - connected.start_span, - connected.end_span, - ); + if error.is_some() { + // Reference: r3_deferred_blocks.ts:149-152 + self.report_error( + "@defer block can only have one @error block", + connected.start_span, + ); + } else { + // Reference: r3_deferred_blocks.ts:252-253 + if !connected.parameters.is_empty() { + self.report_error( + "@error block cannot have parameters", + connected.start_span, + ); + } - error = Some(R3DeferredBlockError { - children: connected_children, - source_span: connected.span, - name_span: connected.name_span, - start_source_span: connected.start_span, - end_source_span: connected.end_span, - i18n, - }); + // Create i18n placeholder if inside an i18n context + let i18n = self.create_block_placeholder( + "error", + &[], + connected.span, + connected.start_span, + connected.end_span, + ); + + error = Some(R3DeferredBlockError { + children: connected_children, + source_span: connected.span, + name_span: connected.name_span, + start_source_span: connected.start_span, + end_source_span: connected.end_span, + i18n, + }); + } } _ => {} } diff --git a/crates/oxc_angular_compiler/src/util/deferred_time.rs b/crates/oxc_angular_compiler/src/util/deferred_time.rs index b4a63855a4..8ec3c99060 100644 --- a/crates/oxc_angular_compiler/src/util/deferred_time.rs +++ b/crates/oxc_angular_compiler/src/util/deferred_time.rs @@ -80,53 +80,6 @@ pub fn get_trigger_parameters_start(expression: &str) -> usize { expression.len() } -/// Checks if a parameter string starts with "minimum". -fn starts_with_minimum(s: &str) -> bool { - let s = s.trim_start(); - s.starts_with("minimum") - && s.get(7..8).is_none_or(|c| c.chars().next().is_none_or(char::is_whitespace)) -} - -/// Checks if a parameter string starts with "after". -fn starts_with_after(s: &str) -> bool { - let s = s.trim_start(); - s.starts_with("after") - && s.get(5..6).is_none_or(|c| c.chars().next().is_none_or(char::is_whitespace)) -} - -/// Parses timing parameters from a @placeholder block. -/// -/// Returns the minimum time in milliseconds if found. -pub fn parse_placeholder_parameters(params: &[&str]) -> Option { - for param in params { - if starts_with_minimum(param) { - let time_str = ¶m[get_trigger_parameters_start(param)..]; - return parse_deferred_time(time_str); - } - } - None -} - -/// Parses timing parameters from a @loading block. -/// -/// Returns (after_time, minimum_time) in milliseconds. -pub fn parse_loading_parameters(params: &[&str]) -> (Option, Option) { - let mut after_time = None; - let mut minimum_time = None; - - for param in params { - if starts_with_after(param) { - let time_str = ¶m[get_trigger_parameters_start(param)..]; - after_time = parse_deferred_time(time_str); - } else if starts_with_minimum(param) { - let time_str = ¶m[get_trigger_parameters_start(param)..]; - minimum_time = parse_deferred_time(time_str); - } - } - - (after_time, minimum_time) -} - #[cfg(test)] mod tests { use super::*; @@ -164,38 +117,4 @@ mod tests { assert_eq!(parse_deferred_time(" 500ms "), Some(500)); assert_eq!(parse_deferred_time(" 1s "), Some(1000)); } - - #[test] - fn test_parse_placeholder_parameters() { - assert_eq!(parse_placeholder_parameters(&["minimum 500ms"]), Some(500)); - assert_eq!(parse_placeholder_parameters(&["minimum 1s"]), Some(1000)); - assert_eq!(parse_placeholder_parameters(&[]), None); - } - - #[test] - fn test_parse_loading_parameters() { - assert_eq!( - parse_loading_parameters(&["after 100ms", "minimum 500ms"]), - (Some(100), Some(500)) - ); - assert_eq!(parse_loading_parameters(&["after 1s"]), (Some(1000), None)); - assert_eq!(parse_loading_parameters(&["minimum 500ms"]), (None, Some(500))); - assert_eq!(parse_loading_parameters(&[]), (None, None)); - } - - #[test] - fn test_starts_with_minimum() { - assert!(starts_with_minimum("minimum 500ms")); - assert!(starts_with_minimum(" minimum 500ms")); - assert!(!starts_with_minimum("after 500ms")); - assert!(!starts_with_minimum("minimumValue")); // not followed by whitespace - } - - #[test] - fn test_starts_with_after() { - assert!(starts_with_after("after 500ms")); - assert!(starts_with_after(" after 500ms")); - assert!(!starts_with_after("minimum 500ms")); - assert!(!starts_with_after("afterTime")); // not followed by whitespace - } } 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 8b3672aa0f..d66edd6075 100644 --- a/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs +++ b/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs @@ -2126,3 +2126,182 @@ mod for_loop_duplicate_let_validation { ); } } + +// ============================================================================ +// Tests: Standalone connected blocks emit errors and UnknownBlock +// ============================================================================ + +mod standalone_connected_blocks { + use super::*; + + #[test] + fn standalone_else_should_produce_error_and_unknown_block() { + // Reference: r3_template_transform.ts:515-517 + let errors = get_transform_errors("@else { content }"); + assert!( + errors.iter().any(|e| e.contains("@else block can only be used after an @if")), + "Expected error for standalone @else, got: {errors:?}" + ); + let result = parse_with_options("@else { content }", true, false); + assert!( + result + .nodes + .iter() + .any(|n| matches!(n, R3NodeRef::UnknownBlock { name } if name == "else")), + "Expected UnknownBlock for standalone @else" + ); + } + + #[test] + fn standalone_else_if_should_produce_error_and_unknown_block() { + let errors = get_transform_errors("@else if (cond) { content }"); + assert!( + errors.iter().any(|e| e.contains("@else if block can only be used after an @if")), + "Expected error for standalone @else if, got: {errors:?}" + ); + } + + #[test] + fn standalone_empty_should_produce_error_and_unknown_block() { + // Reference: r3_template_transform.ts:512-514 + let errors = get_transform_errors("@empty { content }"); + assert!( + errors.iter().any(|e| e.contains("@empty block can only be used after an @for")), + "Expected error for standalone @empty, got: {errors:?}" + ); + let result = parse_with_options("@empty { content }", true, false); + assert!( + result + .nodes + .iter() + .any(|n| matches!(n, R3NodeRef::UnknownBlock { name } if name == "empty")), + "Expected UnknownBlock for standalone @empty" + ); + } + + #[test] + fn standalone_placeholder_should_produce_error_and_unknown_block() { + // Reference: r3_template_transform.ts:509-511 + let errors = get_transform_errors("@placeholder { content }"); + assert!( + errors + .iter() + .any(|e| e.contains("@placeholder block can only be used after an @defer")), + "Expected error for standalone @placeholder, got: {errors:?}" + ); + let result = parse_with_options("@placeholder { content }", true, false); + assert!( + result + .nodes + .iter() + .any(|n| matches!(n, R3NodeRef::UnknownBlock { name } if name == "placeholder")), + "Expected UnknownBlock for standalone @placeholder" + ); + } + + #[test] + fn standalone_loading_should_produce_error_and_unknown_block() { + let errors = get_transform_errors("@loading { content }"); + assert!( + errors.iter().any(|e| e.contains("@loading block can only be used after an @defer")), + "Expected error for standalone @loading, got: {errors:?}" + ); + } + + #[test] + fn standalone_error_should_produce_error_and_unknown_block() { + let errors = get_transform_errors("@error { content }"); + assert!( + errors.iter().any(|e| e.contains("@error block can only be used after an @defer")), + "Expected error for standalone @error, got: {errors:?}" + ); + } +} + +// ============================================================================ +// Tests: @defer connected block validation (duplicates / bad params) +// ============================================================================ + +mod defer_connected_block_validation { + use super::*; + + #[test] + fn should_report_duplicate_placeholder() { + // Reference: r3_deferred_blocks.ts:124-131 + let errors = + get_transform_errors("@defer { main } @placeholder { first } @placeholder { second }"); + assert!( + errors.iter().any(|e| e.contains("@defer block can only have one @placeholder block")), + "Expected duplicate placeholder error, got: {errors:?}" + ); + } + + #[test] + fn should_report_duplicate_loading() { + // Reference: r3_deferred_blocks.ts:137-143 + let errors = get_transform_errors("@defer { main } @loading { first } @loading { second }"); + assert!( + errors.iter().any(|e| e.contains("@defer block can only have one @loading block")), + "Expected duplicate loading error, got: {errors:?}" + ); + } + + #[test] + fn should_report_duplicate_error() { + // Reference: r3_deferred_blocks.ts:149-152 + let errors = get_transform_errors("@defer { main } @error { first } @error { second }"); + assert!( + errors.iter().any(|e| e.contains("@defer block can only have one @error block")), + "Expected duplicate error block error, got: {errors:?}" + ); + } + + #[test] + fn should_report_error_block_with_parameters() { + // Reference: r3_deferred_blocks.ts:252-253 + let errors = get_transform_errors("@defer { main } @error (something) { err }"); + assert!( + errors.iter().any(|e| e.contains("@error block cannot have parameters")), + "Expected error block parameter error, got: {errors:?}" + ); + } + + #[test] + fn should_report_unrecognized_placeholder_parameter() { + // Reference: r3_deferred_blocks.ts:186 + let errors = get_transform_errors("@defer { main } @placeholder (unknown 100ms) { ph }"); + assert!( + errors.iter().any(|e| e.contains("Unrecognized parameter in @placeholder block")), + "Expected unrecognized parameter error, got: {errors:?}" + ); + } + + #[test] + fn should_report_unrecognized_loading_parameter() { + // Reference: r3_deferred_blocks.ts:234-235 + let errors = get_transform_errors("@defer { main } @loading (unknown 100ms) { loading }"); + assert!( + errors.iter().any(|e| e.contains("Unrecognized parameter in @loading block")), + "Expected unrecognized parameter error, got: {errors:?}" + ); + } +} + +// ============================================================================ +// Tests: @for track expression parsing +// ============================================================================ + +mod for_track_expression_parsing { + use super::*; + + #[test] + fn empty_track_expression_should_report_missing_track() { + // Reference: r3_control_flow.ts:406-409 + // Angular matches "track" keyword but then reports missing expression when empty + let errors = get_transform_errors("@for (item of items; track ) {}"); + assert!( + errors.iter().any(|e| e.contains("\"track\" expression")), + "Expected missing track expression error, got: {errors:?}" + ); + } +}