From 221f499d0dd21b1ceef71020b93dcf87262fdc7e Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 23 Feb 2026 23:10:14 +0800 Subject: [PATCH] fix(angular): fix 3 control flow parity gaps with Angular reference - Fix empty parentheses in on-triggers (e.g. `idle()`, `hover()`) being incorrectly treated as having parameters. Angular's consumeParameters() returns zero parameters for empty parens. - Add viewport trigger parameter arity validation using top-level comma splitting to reject `viewport(a,b)` while accepting object literals like `viewport({trigger: foo, rootMargin: "123px"})`. - Fix `@defer (on )` and `@defer (when )` falling through to "Unrecognized trigger" error after lexer trimming. Now recognizes bare "on"/"when" keywords and matches Angular's silent acceptance. Co-Authored-By: Claude Opus 4.6 --- .../src/transform/control_flow.rs | 44 +++- .../tests/r3_template_transform_test.rs | 242 ++++++++++++++++++ 2 files changed, 279 insertions(+), 7 deletions(-) diff --git a/crates/oxc_angular_compiler/src/transform/control_flow.rs b/crates/oxc_angular_compiler/src/transform/control_flow.rs index d4c66f48d..b616a6e08 100644 --- a/crates/oxc_angular_compiler/src/transform/control_flow.rs +++ b/crates/oxc_angular_compiler/src/transform/control_flow.rs @@ -837,13 +837,16 @@ fn is_hydrate_never_pattern(s: &str) -> bool { } /// Pattern to identify a `when` parameter in a block. +/// Matches "when" followed by whitespace, or bare "when" (which can occur after trimming). fn is_when_pattern(s: &str) -> bool { - s.starts_with("when") && s.len() > 4 && s.as_bytes()[4].is_ascii_whitespace() + s.starts_with("when") + && (s.len() == 4 || (s.len() > 4 && s.as_bytes()[4].is_ascii_whitespace())) } /// Pattern to identify an `on` parameter in a block. +/// Matches "on" followed by whitespace, or bare "on" (which can occur after trimming). fn is_on_pattern(s: &str) -> bool { - s.starts_with("on") && s.len() > 2 && s.as_bytes()[2].is_ascii_whitespace() + s.starts_with("on") && (s.len() == 2 || (s.len() > 2 && s.as_bytes()[2].is_ascii_whitespace())) } /// Gets the index within an expression at which the trigger parameters start. @@ -1050,9 +1053,9 @@ fn parse_when_trigger_from_expr<'a>( triggers, errors, ); - } else { - errors.push("@defer 'when' trigger requires a condition expression".to_string()); } + // If no condition found (e.g., bare "when" after trimming), + // Angular silently accepts it with no triggers. Match that behavior. } else { errors.push("Could not find \"when\" keyword in expression".to_string()); } @@ -1092,9 +1095,9 @@ fn parse_on_trigger_from_expr<'a>( errors, binding_parser, ); - } else { - errors.push("@defer 'on' trigger requires trigger types".to_string()); } + // If no trigger parameters found (e.g., bare "on" after trimming), + // Angular silently accepts it with no triggers. Match that behavior. } else { errors.push("Could not find \"on\" keyword in expression".to_string()); } @@ -1282,7 +1285,9 @@ fn parse_single_on_trigger<'a>( let name = trigger_str[..paren_start].trim(); let params_end = trigger_str.rfind(')').unwrap_or(trigger_str.len()); let params_str = trigger_str[paren_start + 1..params_end].trim(); - (name, Some(params_str)) + // Empty parentheses like `idle()` should be treated as zero parameters, + // matching Angular's consumeParameters() which returns an empty array for `()`. + (name, if params_str.is_empty() { None } else { Some(params_str) }) } else { (trigger_str.trim(), None) }; @@ -1424,6 +1429,31 @@ fn parse_single_on_trigger<'a>( return; } + // Validate parameter count before parsing (matching Angular's validator). + // Non-hydrate: validatePlainReferenceBasedTrigger → max 1 parameter + // Hydrate: validateHydrateReferenceBasedTrigger → max 1 parameter + // Use top-level comma splitting to respect nested {}, [], () in object literals. + if let Some(p) = params { + let param_parts: std::vec::Vec<&str> = split_by_top_level_comma(p) + .into_iter() + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .collect(); + if param_parts.len() > 1 { + if hydrate_span.is_some() { + errors.push( + "Hydration trigger \"viewport\" cannot have more than one parameter" + .to_string(), + ); + } else { + errors.push( + "\"viewport\" trigger can only have zero or one parameters".to_string(), + ); + } + return; + } + } + let (reference, options) = if let Some(param_str) = params { let trimmed = param_str.trim(); if trimmed.starts_with('{') { 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 185da6422..29326f679 100644 --- a/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs +++ b/crates/oxc_angular_compiler/tests/r3_template_transform_test.rs @@ -1417,6 +1417,248 @@ mod defer_blocks_extended { } } +// ============================================================================ +// Tests: Trigger empty parentheses (Finding #1) +// Empty () should be treated as zero parameters, matching Angular's consumeParameters(). +// ============================================================================ + +mod trigger_empty_parens { + use super::*; + + #[test] + fn should_accept_idle_with_empty_parens() { + // Angular's consumeParameters returns zero parameters for `idle()`. + // Oxc should not error on this. + let errors = get_transform_errors("@defer (on idle()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("idle")), + "idle() with empty parens should not produce an error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_hover_with_empty_parens() { + let errors = get_transform_errors("@defer (on hover()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("hover")), + "hover() with empty parens should not produce an error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_immediate_with_empty_parens() { + let errors = get_transform_errors("@defer (on immediate()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("immediate")), + "immediate() with empty parens should not produce an error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_interaction_with_empty_parens() { + let errors = get_transform_errors("@defer (on interaction()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("interaction")), + "interaction() with empty parens should not produce an error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_viewport_with_empty_parens() { + let errors = get_transform_errors("@defer (on viewport()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("viewport")), + "viewport() with empty parens should not produce an error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_hydrate_hover_with_empty_parens() { + let errors = get_transform_errors("@defer (hydrate on hover()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("hover")), + "hydrate hover() with empty parens should not produce an error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_hydrate_viewport_with_empty_parens() { + let errors = get_transform_errors("@defer (hydrate on viewport()) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("viewport")), + "hydrate viewport() with empty parens should not produce an error, got: {:?}", + errors + ); + } +} + +// ============================================================================ +// Tests: @defer (on ) / @defer (when ) classification (Finding #3) +// After trimming, "on" alone should still be recognized as an on-trigger prefix +// (matching Angular which does not trim the parameter text). +// ============================================================================ + +mod defer_on_when_trimmed { + use super::*; + + #[test] + fn should_silently_accept_defer_on_space() { + // Angular accepts `@defer (on )` silently — no errors at all. + // After trimming, "on" is recognized as an on-trigger with no trigger names → no triggers. + let errors = get_transform_errors("@defer (on ) {hello}"); + assert!(errors.is_empty(), "@defer (on ) should produce no errors, got: {:?}", errors); + } + + #[test] + fn should_silently_accept_defer_when_space() { + // Angular accepts `@defer (when )` silently — no errors at all. + // After trimming, "when" is recognized as a when-trigger with no condition → no trigger. + let errors = get_transform_errors("@defer (when ) {hello}"); + assert!(errors.is_empty(), "@defer (when ) should produce no errors, got: {:?}", errors); + } +} + +// ============================================================================ +// Tests: Viewport parameter arity validation (Finding #2) +// Angular validates viewport parameter count before parsing. +// ============================================================================ + +mod viewport_arity_validation { + use super::*; + + #[test] + fn should_error_on_viewport_with_multiple_params() { + // Angular errors: "viewport" trigger can only have zero or one parameters + let errors = get_transform_errors("@defer (on viewport(a,b)) {hello}"); + assert!( + errors.iter().any(|e| e.contains("viewport") && e.contains("parameter")), + "viewport(a,b) should produce a parameter count error, got: {:?}", + errors + ); + } + + #[test] + fn should_error_on_hydrate_viewport_with_multiple_params() { + // Angular errors: Hydration trigger "viewport" cannot have more than one parameter + let errors = get_transform_errors("@defer (hydrate on viewport(a,b)) {hello}"); + assert!( + errors.iter().any(|e| e.contains("viewport") && e.contains("parameter")), + "hydrate viewport(a,b) should produce a parameter count error, got: {:?}", + errors + ); + } + + #[test] + fn should_accept_viewport_with_single_param() { + // viewport(ref) is valid + let errors = get_transform_errors("@defer (on viewport(ref)) {hello}"); + assert!( + !errors.iter().any(|e| e.contains("viewport") && e.contains("parameter")), + "viewport(ref) should not produce a parameter error, got: {:?}", + errors + ); + } +} + +// ============================================================================ +// Tests: @for error cascade (Finding #4) +// Angular only reports the parse-expression error for `@for (x of ) {}`, +// NOT the missing-track error. Oxc should match. +// ============================================================================ + +mod for_error_cascade { + use super::*; + + #[test] + fn should_not_emit_missing_track_when_expression_empty_after_of() { + // `@for (x of ) {}` - empty expression after "of" + // Angular: only "Cannot parse expression" error + // Oxc should NOT also emit "@for loop must have a \"track\" expression" + let errors = get_transform_errors("@for (x of ) {content}"); + let has_parse_error = errors.iter().any(|e| e.contains("Cannot parse expression")); + let has_track_error = errors.iter().any(|e| e.contains("track")); + assert!(has_parse_error, "Should have a parse expression error, got: {:?}", errors); + assert!( + !has_track_error, + "Should NOT have a missing track error when expression parse fails, got: {:?}", + errors + ); + } + + #[test] + fn should_not_emit_missing_track_when_no_expression() { + // `@for () {}` - no expression at all + let errors = get_transform_errors("@for () {content}"); + let has_track_error = errors.iter().any(|e| e.contains("track")); + assert!( + !has_track_error, + "Should NOT have a missing track error when no expression, got: {:?}", + errors + ); + } + + #[test] + fn should_not_emit_missing_track_when_missing_of() { + // `@for (x) {}` - no "of" keyword + let errors = get_transform_errors("@for (x) {content}"); + let has_track_error = errors.iter().any(|e| e.contains("track")); + assert!( + !has_track_error, + "Should NOT have a missing track error when 'of' is missing, got: {:?}", + errors + ); + } + + #[test] + fn should_not_emit_missing_track_for_for_with_invalid_expression() { + // `@for (x of; track x) {}` - missing iterable, has track + // The semicolon makes "x of" the first parameter and "track x" the second + // Angular should only error on the expression parse + let errors = get_transform_errors("@for (x of; track x) {content}"); + let error_count = errors.len(); + // Should have parse error but not missing-track error + // (track is actually provided but expression parse failed) + assert!( + error_count >= 1, + "Should have at least one error for invalid expression, got: {:?}", + errors + ); + } +} + +// ============================================================================ +// Tests: Error-recovery AST shape (Finding #5) +// Angular only adds @if branches / @for nodes when params parse successfully. +// This is a LOW priority structural difference in error recovery. +// ============================================================================ + +mod error_recovery_ast_shape { + use super::*; + + #[test] + fn if_block_with_missing_expression_should_still_produce_ast() { + // `@if {content}` - missing expression + // Oxc produces an IfBlock node for error recovery + 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 + assert!(!result.is_empty()); + } + + #[test] + fn for_block_with_missing_params_should_still_produce_ast() { + // `@for () {content}` - missing parameters + let result = humanize_ignore_errors("@for () {content}"); + assert!(!result.is_empty()); + } +} + // ============================================================================ // Tests: More switch block tests // ============================================================================