Skip to content

Commit 8a3a1d5

Browse files
Brooooooklynclaude
andcommitted
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 <noreply@anthropic.com>
1 parent bfaaea9 commit 8a3a1d5

2 files changed

Lines changed: 299 additions & 6 deletions

File tree

crates/oxc_angular_compiler/src/transform/control_flow.rs

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -837,13 +837,16 @@ fn is_hydrate_never_pattern(s: &str) -> bool {
837837
}
838838

839839
/// Pattern to identify a `when` parameter in a block.
840+
/// Matches "when" followed by whitespace, or bare "when" (which can occur after trimming).
840841
fn is_when_pattern(s: &str) -> bool {
841-
s.starts_with("when") && s.len() > 4 && s.as_bytes()[4].is_ascii_whitespace()
842+
s.starts_with("when")
843+
&& (s.len() == 4 || (s.len() > 4 && s.as_bytes()[4].is_ascii_whitespace()))
842844
}
843845

844846
/// Pattern to identify an `on` parameter in a block.
847+
/// Matches "on" followed by whitespace, or bare "on" (which can occur after trimming).
845848
fn is_on_pattern(s: &str) -> bool {
846-
s.starts_with("on") && s.len() > 2 && s.as_bytes()[2].is_ascii_whitespace()
849+
s.starts_with("on") && (s.len() == 2 || (s.len() > 2 && s.as_bytes()[2].is_ascii_whitespace()))
847850
}
848851

849852
/// Gets the index within an expression at which the trigger parameters start.
@@ -1051,7 +1054,21 @@ fn parse_when_trigger_from_expr<'a>(
10511054
errors,
10521055
);
10531056
} else {
1054-
errors.push("@defer 'when' trigger requires a condition expression".to_string());
1057+
// Angular silently creates a when trigger with an empty expression
1058+
// when no condition is provided (e.g., bare "when" after trimming).
1059+
// We parse an empty condition to match.
1060+
parse_when_trigger(
1061+
allocator,
1062+
binding_parser,
1063+
"",
1064+
span,
1065+
span,
1066+
prefetch_span,
1067+
hydrate_span,
1068+
when_source_span,
1069+
triggers,
1070+
errors,
1071+
);
10551072
}
10561073
} else {
10571074
errors.push("Could not find \"when\" keyword in expression".to_string());
@@ -1092,9 +1109,9 @@ fn parse_on_trigger_from_expr<'a>(
10921109
errors,
10931110
binding_parser,
10941111
);
1095-
} else {
1096-
errors.push("@defer 'on' trigger requires trigger types".to_string());
10971112
}
1113+
// If no trigger parameters found (e.g., bare "on" after trimming),
1114+
// Angular silently accepts it with no triggers. Match that behavior.
10981115
} else {
10991116
errors.push("Could not find \"on\" keyword in expression".to_string());
11001117
}
@@ -1282,7 +1299,9 @@ fn parse_single_on_trigger<'a>(
12821299
let name = trigger_str[..paren_start].trim();
12831300
let params_end = trigger_str.rfind(')').unwrap_or(trigger_str.len());
12841301
let params_str = trigger_str[paren_start + 1..params_end].trim();
1285-
(name, Some(params_str))
1302+
// Empty parentheses like `idle()` should be treated as zero parameters,
1303+
// matching Angular's consumeParameters() which returns an empty array for `()`.
1304+
(name, if params_str.is_empty() { None } else { Some(params_str) })
12861305
} else {
12871306
(trigger_str.trim(), None)
12881307
};
@@ -1424,6 +1443,31 @@ fn parse_single_on_trigger<'a>(
14241443
return;
14251444
}
14261445

1446+
// Validate parameter count before parsing (matching Angular's validator).
1447+
// Non-hydrate: validatePlainReferenceBasedTrigger → max 1 parameter
1448+
// Hydrate: validateHydrateReferenceBasedTrigger → max 1 parameter
1449+
// Use top-level comma splitting to respect nested {}, [], () in object literals.
1450+
if let Some(p) = params {
1451+
let param_parts: std::vec::Vec<&str> = split_by_top_level_comma(p)
1452+
.into_iter()
1453+
.map(|s| s.trim())
1454+
.filter(|s| !s.is_empty())
1455+
.collect();
1456+
if param_parts.len() > 1 {
1457+
if hydrate_span.is_some() {
1458+
errors.push(
1459+
"Hydration trigger \"viewport\" cannot have more than one parameter"
1460+
.to_string(),
1461+
);
1462+
} else {
1463+
errors.push(
1464+
"\"viewport\" trigger can only have zero or one parameters".to_string(),
1465+
);
1466+
}
1467+
return;
1468+
}
1469+
}
1470+
14271471
let (reference, options) = if let Some(param_str) = params {
14281472
let trimmed = param_str.trim();
14291473
if trimmed.starts_with('{') {

crates/oxc_angular_compiler/tests/r3_template_transform_test.rs

Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1417,6 +1417,255 @@ mod defer_blocks_extended {
14171417
}
14181418
}
14191419

1420+
// ============================================================================
1421+
// Tests: Trigger empty parentheses (Finding #1)
1422+
// Empty () should be treated as zero parameters, matching Angular's consumeParameters().
1423+
// ============================================================================
1424+
1425+
mod trigger_empty_parens {
1426+
use super::*;
1427+
1428+
#[test]
1429+
fn should_accept_idle_with_empty_parens() {
1430+
// Angular's consumeParameters returns zero parameters for `idle()`.
1431+
// Oxc should not error on this.
1432+
let errors = get_transform_errors("@defer (on idle()) {hello}");
1433+
assert!(
1434+
!errors.iter().any(|e| e.contains("idle")),
1435+
"idle() with empty parens should not produce an error, got: {:?}",
1436+
errors
1437+
);
1438+
}
1439+
1440+
#[test]
1441+
fn should_accept_hover_with_empty_parens() {
1442+
let errors = get_transform_errors("@defer (on hover()) {hello}");
1443+
assert!(
1444+
!errors.iter().any(|e| e.contains("hover")),
1445+
"hover() with empty parens should not produce an error, got: {:?}",
1446+
errors
1447+
);
1448+
}
1449+
1450+
#[test]
1451+
fn should_accept_immediate_with_empty_parens() {
1452+
let errors = get_transform_errors("@defer (on immediate()) {hello}");
1453+
assert!(
1454+
!errors.iter().any(|e| e.contains("immediate")),
1455+
"immediate() with empty parens should not produce an error, got: {:?}",
1456+
errors
1457+
);
1458+
}
1459+
1460+
#[test]
1461+
fn should_accept_interaction_with_empty_parens() {
1462+
let errors = get_transform_errors("@defer (on interaction()) {hello}");
1463+
assert!(
1464+
!errors.iter().any(|e| e.contains("interaction")),
1465+
"interaction() with empty parens should not produce an error, got: {:?}",
1466+
errors
1467+
);
1468+
}
1469+
1470+
#[test]
1471+
fn should_accept_viewport_with_empty_parens() {
1472+
let errors = get_transform_errors("@defer (on viewport()) {hello}");
1473+
assert!(
1474+
!errors.iter().any(|e| e.contains("viewport")),
1475+
"viewport() with empty parens should not produce an error, got: {:?}",
1476+
errors
1477+
);
1478+
}
1479+
1480+
#[test]
1481+
fn should_accept_hydrate_hover_with_empty_parens() {
1482+
let errors = get_transform_errors("@defer (hydrate on hover()) {hello}");
1483+
assert!(
1484+
!errors.iter().any(|e| e.contains("hover")),
1485+
"hydrate hover() with empty parens should not produce an error, got: {:?}",
1486+
errors
1487+
);
1488+
}
1489+
1490+
#[test]
1491+
fn should_accept_hydrate_viewport_with_empty_parens() {
1492+
let errors = get_transform_errors("@defer (hydrate on viewport()) {hello}");
1493+
assert!(
1494+
!errors.iter().any(|e| e.contains("viewport")),
1495+
"hydrate viewport() with empty parens should not produce an error, got: {:?}",
1496+
errors
1497+
);
1498+
}
1499+
}
1500+
1501+
// ============================================================================
1502+
// Tests: @defer (on ) / @defer (when ) classification (Finding #3)
1503+
// After trimming, "on" alone should still be recognized as an on-trigger prefix
1504+
// (matching Angular which does not trim the parameter text).
1505+
// ============================================================================
1506+
1507+
mod defer_on_when_trimmed {
1508+
use super::*;
1509+
1510+
#[test]
1511+
fn should_not_error_unrecognized_trigger_for_defer_on_space() {
1512+
// Angular accepts `@defer (on )` without an "Unrecognized trigger" error.
1513+
// It classifies it as an on-trigger with no trigger names → no triggers.
1514+
let errors = get_transform_errors("@defer (on ) {hello}");
1515+
assert!(
1516+
!errors.iter().any(|e| e.contains("Unrecognized trigger")),
1517+
"@defer (on ) should not produce 'Unrecognized trigger', got: {:?}",
1518+
errors
1519+
);
1520+
}
1521+
1522+
#[test]
1523+
fn should_not_error_unrecognized_trigger_for_defer_when_space() {
1524+
// Angular accepts `@defer (when )` without an "Unrecognized trigger" error.
1525+
let errors = get_transform_errors("@defer (when ) {hello}");
1526+
assert!(
1527+
!errors.iter().any(|e| e.contains("Unrecognized trigger")),
1528+
"@defer (when ) should not produce 'Unrecognized trigger', got: {:?}",
1529+
errors
1530+
);
1531+
}
1532+
}
1533+
1534+
// ============================================================================
1535+
// Tests: Viewport parameter arity validation (Finding #2)
1536+
// Angular validates viewport parameter count before parsing.
1537+
// ============================================================================
1538+
1539+
mod viewport_arity_validation {
1540+
use super::*;
1541+
1542+
#[test]
1543+
fn should_error_on_viewport_with_multiple_params() {
1544+
// Angular errors: "viewport" trigger can only have zero or one parameters
1545+
let errors = get_transform_errors("@defer (on viewport(a,b)) {hello}");
1546+
assert!(
1547+
errors.iter().any(|e| e.contains("viewport") && e.contains("parameter")),
1548+
"viewport(a,b) should produce a parameter count error, got: {:?}",
1549+
errors
1550+
);
1551+
}
1552+
1553+
#[test]
1554+
fn should_error_on_hydrate_viewport_with_multiple_params() {
1555+
// Angular errors: Hydration trigger "viewport" cannot have more than one parameter
1556+
let errors = get_transform_errors("@defer (hydrate on viewport(a,b)) {hello}");
1557+
assert!(
1558+
errors.iter().any(|e| e.contains("viewport") || e.contains("parameter")),
1559+
"hydrate viewport(a,b) should produce an error, got: {:?}",
1560+
errors
1561+
);
1562+
}
1563+
1564+
#[test]
1565+
fn should_accept_viewport_with_single_param() {
1566+
// viewport(ref) is valid
1567+
let errors = get_transform_errors("@defer (on viewport(ref)) {hello}");
1568+
assert!(
1569+
!errors.iter().any(|e| e.contains("viewport") && e.contains("parameter")),
1570+
"viewport(ref) should not produce a parameter error, got: {:?}",
1571+
errors
1572+
);
1573+
}
1574+
}
1575+
1576+
// ============================================================================
1577+
// Tests: @for error cascade (Finding #4)
1578+
// Angular only reports the parse-expression error for `@for (x of ) {}`,
1579+
// NOT the missing-track error. Oxc should match.
1580+
// ============================================================================
1581+
1582+
mod for_error_cascade {
1583+
use super::*;
1584+
1585+
#[test]
1586+
fn should_not_emit_missing_track_when_expression_empty_after_of() {
1587+
// `@for (x of ) {}` - empty expression after "of"
1588+
// Angular: only "Cannot parse expression" error
1589+
// Oxc should NOT also emit "@for loop must have a \"track\" expression"
1590+
let errors = get_transform_errors("@for (x of ) {content}");
1591+
let has_parse_error = errors.iter().any(|e| e.contains("Cannot parse expression"));
1592+
let has_track_error = errors.iter().any(|e| e.contains("track"));
1593+
assert!(has_parse_error, "Should have a parse expression error, got: {:?}", errors);
1594+
assert!(
1595+
!has_track_error,
1596+
"Should NOT have a missing track error when expression parse fails, got: {:?}",
1597+
errors
1598+
);
1599+
}
1600+
1601+
#[test]
1602+
fn should_not_emit_missing_track_when_no_expression() {
1603+
// `@for () {}` - no expression at all
1604+
let errors = get_transform_errors("@for () {content}");
1605+
let has_track_error = errors.iter().any(|e| e.contains("track"));
1606+
assert!(
1607+
!has_track_error,
1608+
"Should NOT have a missing track error when no expression, got: {:?}",
1609+
errors
1610+
);
1611+
}
1612+
1613+
#[test]
1614+
fn should_not_emit_missing_track_when_missing_of() {
1615+
// `@for (x) {}` - no "of" keyword
1616+
let errors = get_transform_errors("@for (x) {content}");
1617+
let has_track_error = errors.iter().any(|e| e.contains("track"));
1618+
assert!(
1619+
!has_track_error,
1620+
"Should NOT have a missing track error when 'of' is missing, got: {:?}",
1621+
errors
1622+
);
1623+
}
1624+
1625+
#[test]
1626+
fn should_not_emit_missing_track_for_for_with_invalid_expression() {
1627+
// `@for (x of; track x) {}` - missing iterable, has track
1628+
// The semicolon makes "x of" the first parameter and "track x" the second
1629+
// Angular should only error on the expression parse
1630+
let errors = get_transform_errors("@for (x of; track x) {content}");
1631+
let error_count = errors.len();
1632+
// Should have parse error but not missing-track error
1633+
// (track is actually provided but expression parse failed)
1634+
assert!(
1635+
error_count >= 1,
1636+
"Should have at least one error for invalid expression, got: {:?}",
1637+
errors
1638+
);
1639+
}
1640+
}
1641+
1642+
// ============================================================================
1643+
// Tests: Error-recovery AST shape (Finding #5)
1644+
// Angular only adds @if branches / @for nodes when params parse successfully.
1645+
// This is a LOW priority structural difference in error recovery.
1646+
// ============================================================================
1647+
1648+
mod error_recovery_ast_shape {
1649+
use super::*;
1650+
1651+
#[test]
1652+
fn if_block_with_missing_expression_should_still_produce_ast() {
1653+
// `@if {content}` - missing expression
1654+
// Oxc produces an IfBlock node for error recovery
1655+
let result = humanize_ignore_errors("@if {content}");
1656+
// We don't need to match Angular exactly here - just verify no crash
1657+
// and that the AST is reasonable for error recovery
1658+
assert!(!result.is_empty());
1659+
}
1660+
1661+
#[test]
1662+
fn for_block_with_missing_params_should_still_produce_ast() {
1663+
// `@for () {content}` - missing parameters
1664+
let result = humanize_ignore_errors("@for () {content}");
1665+
assert!(!result.is_empty());
1666+
}
1667+
}
1668+
14201669
// ============================================================================
14211670
// Tests: More switch block tests
14221671
// ============================================================================

0 commit comments

Comments
 (0)