Skip to content

Commit 6b395f4

Browse files
committed
fix paren stripping in let-chain conditions
In `if let PAT = EXPR && (a && b)`, the formatter was emitting `if let PAT = EXPR && a && b`, which re-parses as a 3-element let-chain instead of 2-element. This violated round-trip AST equivalence. The fix: when emitting a ConditionElement::Expr inside a LetChain, wrap it in parens if it contains && or || to prevent the expression from merging with the chain's && separators. https://claude.ai/code/session_0185DQbZrR8XjistfKkPwAki
1 parent 44f70ca commit 6b395f4

File tree

2 files changed

+57
-18
lines changed

2 files changed

+57
-18
lines changed

wado-compiler/src/unparse.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1383,7 +1383,22 @@ impl<'a> Unparser<'a> {
13831383
self.unparse_expr(expr);
13841384
}
13851385
ConditionElement::Expr(expr) => {
1386+
// In a let-chain, elements are joined by `&&`.
1387+
// If this element is itself a `&&` or `||` expression,
1388+
// we must wrap it in parens to preserve the AST structure.
1389+
// Without parens, `let PAT = E && (a && b)` would be
1390+
// re-parsed as three chain elements instead of two.
1391+
let needs_parens = matches!(
1392+
expr,
1393+
Expr::Binary(b) if matches!(b.op, BinaryOp::And | BinaryOp::Or)
1394+
);
1395+
if needs_parens {
1396+
self.output.push('(');
1397+
}
13861398
self.unparse_expr(expr);
1399+
if needs_parens {
1400+
self.output.push(')');
1401+
}
13871402
}
13881403
}
13891404
}
@@ -3202,7 +3217,17 @@ fn unparse_condition_into(cond: &Condition, output: &mut String) {
32023217
unparse_expr_into(expr, output, false);
32033218
}
32043219
ConditionElement::Expr(expr) => {
3220+
let needs_parens = matches!(
3221+
expr,
3222+
Expr::Binary(b) if matches!(b.op, BinaryOp::And | BinaryOp::Or)
3223+
);
3224+
if needs_parens {
3225+
output.push('(');
3226+
}
32053227
unparse_expr_into(expr, output, false);
3228+
if needs_parens {
3229+
output.push(')');
3230+
}
32063231
}
32073232
}
32083233
}

wado-compiler/tests/format.rs

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1695,12 +1695,6 @@ fn test_roundtrip_ast_all_fixtures() {
16951695
continue;
16961696
}
16971697

1698-
// Known bug: formatter strips parens in `if let ... && (expr)` / `while let ... && (expr)`
1699-
// See test_roundtrip_ast_known_paren_stripping_bug for the regression test.
1700-
if filename == "if_merged.wado" || filename == "while_merged.wado" {
1701-
continue;
1702-
}
1703-
17041698
let formatted = match wado_compiler::format(&source) {
17051699
Ok(f) => f,
17061700
Err(_) => continue,
@@ -1750,19 +1744,13 @@ fn test_roundtrip_ast_all_fixtures() {
17501744
}
17511745
}
17521746

1753-
/// Known bug: the formatter strips parentheses in `if let ... && (expr)` and
1754-
/// `while let ... && (expr)` conditions, changing the AST structure.
1755-
///
1756-
/// Example: `if let Some(x) = opt && (flag1 && flag2)` becomes
1757-
/// `if let Some(x) = opt && flag1 && flag2`
1758-
///
1759-
/// While semantically equivalent in this case (due to && associativity),
1760-
/// this changes the AST from `Binary(LetAnd, Paren(Binary(...)))` to
1761-
/// `Binary(LetAnd, Binary(LetAnd, ...))`, which violates round-trip AST
1762-
/// equivalence. The formatter should preserve explicit parens in these positions.
1747+
/// Regression test: the formatter must preserve parentheses in `if let ... && (expr)`
1748+
/// and `while let ... && (expr)` conditions. Without parens, the let-chain element
1749+
/// count changes on re-parse (e.g., 2 elements become 3), violating round-trip
1750+
/// AST equivalence.
17631751
#[test]
1764-
#[should_panic(expected = "Formatter changed AST semantics")]
1765-
fn test_roundtrip_ast_known_paren_stripping_bug() {
1752+
fn test_roundtrip_ast_let_chain_paren_preservation() {
1753+
// if let with && boolean guard containing &&
17661754
assert_format_preserves_ast(
17671755
r#"
17681756
fn test() {
@@ -1773,6 +1761,32 @@ fn test() {
17731761
println(`{x}`);
17741762
}
17751763
}
1764+
"#,
1765+
);
1766+
1767+
// while let with && guard containing &&
1768+
assert_format_preserves_ast(
1769+
r#"
1770+
fn test() {
1771+
let mut iter = [1, 2, 3].iter();
1772+
let min = 0;
1773+
let max = 10;
1774+
while let Some(v) = iter.next() && (v > min && v < max) {
1775+
println(`{v}`);
1776+
}
1777+
}
1778+
"#,
1779+
);
1780+
1781+
// if let with || inside guard
1782+
assert_format_preserves_ast(
1783+
r#"
1784+
fn test() {
1785+
let opt = Option::<i32>::Some(5);
1786+
if let Some(x) = opt && (x > 10 || x < 0) {
1787+
println(`{x}`);
1788+
}
1789+
}
17761790
"#,
17771791
);
17781792
}

0 commit comments

Comments
 (0)