Skip to content

Commit ea8bc3b

Browse files
authored
Rollup merge of #134600 - dtolnay:chainedcomparison, r=oli-obk
Fix parenthesization of chained comparisons by pretty-printer Example: ```rust macro_rules! repro { () => { 1 < 2 }; } fn main() { let _ = repro!() == false; } ``` Previously `-Zunpretty=expanded` would pretty-print this syntactically invalid output: `fn main() { let _ = 1 < 2 == false; }` ```console error: comparison operators cannot be chained --> <anon>:8:23 | 8 | fn main() { let _ = 1 < 2 == false; } | ^ ^^ | help: parenthesize the comparison | 8 | fn main() { let _ = (1 < 2) == false; } | + + ``` With the fix, it will print `fn main() { let _ = (1 < 2) == false; }`. Making `-Zunpretty=expanded` consistently produce syntactically valid Rust output is important because that is what makes it possible for `cargo expand` to format and perform filtering on the expanded code. ## Review notes According to `rg '\.fixity\(\)' compiler/` the `fixity` function is called only 3 places: - https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/compiler/rustc_ast_pretty/src/pprust/state/expr.rs#L283-L287 - https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/compiler/rustc_hir_pretty/src/lib.rs#L1295-L1299 - https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/compiler/rustc_parse/src/parser/expr.rs#L282-L289 The 2 pretty printers definitely want to treat comparisons using `Fixity::None`. That's the whole bug being fixed. Meanwhile, the parser's `Fixity::None` codepath is previously unreachable as indicated by the comment, so as long as `Fixity::None` here behaves exactly the way that `Fixity::Left` used to behave, you can tell that this PR definitely does not constitute any behavior change for the parser. My guess for why comparison operators were set to `Fixity::Left` instead of `Fixity::None` is that it's a very old workaround for giving a good chained comparisons diagnostic (like what I pasted above). Nowadays that is handled by a different dedicated codepath.
2 parents cc27e3f + fe65e88 commit ea8bc3b

File tree

3 files changed

+16
-17
lines changed

3 files changed

+16
-17
lines changed

Diff for: compiler/rustc_ast/src/util/parser.rs

+4-3
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,10 @@ impl AssocOp {
153153
match *self {
154154
Assign | AssignOp(_) => Fixity::Right,
155155
As | Multiply | Divide | Modulus | Add | Subtract | ShiftLeft | ShiftRight | BitAnd
156-
| BitXor | BitOr | Less | Greater | LessEqual | GreaterEqual | Equal | NotEqual
157-
| LAnd | LOr => Fixity::Left,
158-
DotDot | DotDotEq => Fixity::None,
156+
| BitXor | BitOr | LAnd | LOr => Fixity::Left,
157+
Less | Greater | LessEqual | GreaterEqual | Equal | NotEqual | DotDot | DotDotEq => {
158+
Fixity::None
159+
}
159160
}
160161
}
161162

Diff for: compiler/rustc_parse/src/parser/expr.rs

+2-10
Original file line numberDiff line numberDiff line change
@@ -279,13 +279,9 @@ impl<'a> Parser<'a> {
279279
break;
280280
}
281281

282-
let fixity = op.fixity();
283-
let min_prec = match fixity {
282+
let min_prec = match op.fixity() {
284283
Fixity::Right => Bound::Included(prec),
285-
Fixity::Left => Bound::Excluded(prec),
286-
// We currently have no non-associative operators that are not handled above by
287-
// the special cases. The code is here only for future convenience.
288-
Fixity::None => Bound::Excluded(prec),
284+
Fixity::Left | Fixity::None => Bound::Excluded(prec),
289285
};
290286
let (rhs, _) = self.with_res(restrictions - Restrictions::STMT_EXPR, |this| {
291287
let attrs = this.parse_outer_attributes()?;
@@ -337,10 +333,6 @@ impl<'a> Parser<'a> {
337333
self.dcx().span_bug(span, "AssocOp should have been handled by special case")
338334
}
339335
};
340-
341-
if let Fixity::None = fixity {
342-
break;
343-
}
344336
}
345337

346338
Ok((lhs, parsed_something))

Diff for: tests/ui-fulldeps/pprust-parenthesis-insertion.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ static EXPRS: &[&str] = &[
6161
"(2 + 2) * 2",
6262
"2 * (2 + 2)",
6363
"2 + 2 + 2",
64+
// Right-associative operator.
65+
"2 += 2 += 2",
66+
"(2 += 2) += 2",
6467
// Return has lower precedence than a binary operator.
6568
"(return 2) + 2",
6669
"2 + (return 2)", // FIXME: no parenthesis needed.
@@ -89,6 +92,13 @@ static EXPRS: &[&str] = &[
8992
// allowed, except if the break is also labeled.
9093
"break 'outer 'inner: loop {} + 2",
9194
"break ('inner: loop {} + 2)",
95+
// Grammar restriction: ranges cannot be the endpoint of another range.
96+
"(2..2)..2",
97+
"2..(2..2)",
98+
"(2..2)..",
99+
"..(2..2)",
100+
// Grammar restriction: comparison operators cannot be chained (1 < 2 == false).
101+
"((1 < 2) == false) as usize",
92102
// Grammar restriction: the value in let-else is not allowed to end in a
93103
// curly brace.
94104
"{ let _ = 1 + 1 else {}; }",
@@ -113,10 +123,6 @@ static EXPRS: &[&str] = &[
113123
"if let _ = () && (Struct {}).x {}",
114124
*/
115125
/*
116-
// FIXME: pretty-printer produces invalid syntax. `(1 < 2 == false) as usize`
117-
"((1 < 2) == false) as usize",
118-
*/
119-
/*
120126
// FIXME: pretty-printer produces invalid syntax. `for _ in 1..{ 2 } {}`
121127
"for _ in (1..{ 2 }) {}",
122128
*/

0 commit comments

Comments
 (0)