Skip to content

Commit 088a180

Browse files
varkorvarkor
committed
Add suggestions when encountering chained comparisons
1 parent 2d8d559 commit 088a180

8 files changed

+335
-49
lines changed

‎src/librustc_parse/parser/diagnostics.rs

Lines changed: 70 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use rustc_data_structures::fx::FxHashSet;
44
use rustc_error_codes::*;
55
use rustc_errors::{pluralize, struct_span_err};
66
use rustc_errors::{Applicability, DiagnosticBuilder, Handler, PResult};
7+
use rustc_span::source_map::Spanned;
78
use rustc_span::symbol::kw;
89
use rustc_span::{MultiSpan, Span, SpanSnippetError, DUMMY_SP};
910
use syntax::ast::{
@@ -500,6 +501,58 @@ impl<'a> Parser<'a> {
500501
}
501502
}
502503

504+
/// Check to see if a pair of chained operators looks like an attempt at chained comparison,
505+
/// e.g. `1 < x <= 3`. If so, suggest either splitting the comparison into two, or
506+
/// parenthesising the leftmost comparison.
507+
fn attempt_chained_comparison_suggestion(
508+
&mut self,
509+
err: &mut DiagnosticBuilder<'_>,
510+
inner_op: &Expr,
511+
outer_op: &Spanned<AssocOp>,
512+
) {
513+
if let ExprKind::Binary(op, ref l1, ref r1) = inner_op.kind {
514+
match (op.node, &outer_op.node) {
515+
// `x < y < z` and friends.
516+
(BinOpKind::Lt, AssocOp::Less) | (BinOpKind::Lt, AssocOp::LessEqual) |
517+
(BinOpKind::Le, AssocOp::LessEqual) | (BinOpKind::Le, AssocOp::Less) |
518+
// `x > y > z` and friends.
519+
(BinOpKind::Gt, AssocOp::Greater) | (BinOpKind::Gt, AssocOp::GreaterEqual) |
520+
(BinOpKind::Ge, AssocOp::GreaterEqual) | (BinOpKind::Ge, AssocOp::Greater) => {
521+
let expr_to_str = |e: &Expr| {
522+
self.span_to_snippet(e.span)
523+
.unwrap_or_else(|_| pprust::expr_to_string(&e))
524+
};
525+
err.span_suggestion(
526+
inner_op.span.to(outer_op.span),
527+
"split the comparison into two...",
528+
format!(
529+
"{} {} {} && {} {}",
530+
expr_to_str(&l1),
531+
op.node.to_string(),
532+
expr_to_str(&r1),
533+
expr_to_str(&r1),
534+
outer_op.node.to_ast_binop().unwrap().to_string(),
535+
),
536+
Applicability::MaybeIncorrect,
537+
);
538+
err.span_suggestion(
539+
inner_op.span.to(outer_op.span),
540+
"...or parenthesize one of the comparisons",
541+
format!(
542+
"({} {} {}) {}",
543+
expr_to_str(&l1),
544+
op.node.to_string(),
545+
expr_to_str(&r1),
546+
outer_op.node.to_ast_binop().unwrap().to_string(),
547+
),
548+
Applicability::MaybeIncorrect,
549+
);
550+
}
551+
_ => {}
552+
}
553+
}
554+
}
555+
503556
/// Produces an error if comparison operators are chained (RFC #558).
504557
/// We only need to check the LHS, not the RHS, because all comparison ops have same
505558
/// precedence (see `fn precedence`) and are left-associative (see `fn fixity`).
@@ -515,27 +568,31 @@ impl<'a> Parser<'a> {
515568
/// / \
516569
/// inner_op r2
517570
/// / \
518-
/// l1 r1
571+
/// l1 r1
519572
pub(super) fn check_no_chained_comparison(
520573
&mut self,
521-
lhs: &Expr,
522-
outer_op: &AssocOp,
574+
inner_op: &Expr,
575+
outer_op: &Spanned<AssocOp>,
523576
) -> PResult<'a, Option<P<Expr>>> {
524577
debug_assert!(
525-
outer_op.is_comparison(),
578+
outer_op.node.is_comparison(),
526579
"check_no_chained_comparison: {:?} is not comparison",
527-
outer_op,
580+
outer_op.node,
528581
);
529582

530583
let mk_err_expr =
531584
|this: &Self, span| Ok(Some(this.mk_expr(span, ExprKind::Err, AttrVec::new())));
532585

533-
match lhs.kind {
586+
match inner_op.kind {
534587
ExprKind::Binary(op, _, _) if op.node.is_comparison() => {
535588
// Respan to include both operators.
536589
let op_span = op.span.to(self.prev_span);
537-
let mut err = self
538-
.struct_span_err(op_span, "chained comparison operators require parentheses");
590+
let mut err =
591+
self.struct_span_err(op_span, "comparison operators cannot be chained");
592+
593+
// If it looks like a genuine attempt to chain operators (as opposed to a
594+
// misformatted turbofish, for instance), suggest a correct form.
595+
self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op);
539596

540597
let suggest = |err: &mut DiagnosticBuilder<'_>| {
541598
err.span_suggestion_verbose(
@@ -547,12 +604,12 @@ impl<'a> Parser<'a> {
547604
};
548605

549606
if op.node == BinOpKind::Lt &&
550-
*outer_op == AssocOp::Less || // Include `<` to provide this recommendation
551-
*outer_op == AssocOp::Greater
607+
outer_op.node == AssocOp::Less || // Include `<` to provide this recommendation
608+
outer_op.node == AssocOp::Greater
552609
// even in a case like the following:
553610
{
554611
// Foo<Bar<Baz<Qux, ()>>>
555-
if *outer_op == AssocOp::Less {
612+
if outer_op.node == AssocOp::Less {
556613
let snapshot = self.clone();
557614
self.bump();
558615
// So far we have parsed `foo<bar<`, consume the rest of the type args.
@@ -584,7 +641,7 @@ impl<'a> Parser<'a> {
584641
// FIXME: actually check that the two expressions in the binop are
585642
// paths and resynthesize new fn call expression instead of using
586643
// `ExprKind::Err` placeholder.
587-
mk_err_expr(self, lhs.span.to(self.prev_span))
644+
mk_err_expr(self, inner_op.span.to(self.prev_span))
588645
}
589646
Err(mut expr_err) => {
590647
expr_err.cancel();
@@ -606,7 +663,7 @@ impl<'a> Parser<'a> {
606663
// FIXME: actually check that the two expressions in the binop are
607664
// paths and resynthesize new fn call expression instead of using
608665
// `ExprKind::Err` placeholder.
609-
mk_err_expr(self, lhs.span.to(self.prev_span))
666+
mk_err_expr(self, inner_op.span.to(self.prev_span))
610667
}
611668
}
612669
} else {

‎src/librustc_parse/parser/expr.rs

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use super::{SemiColonMode, SeqSep, TokenExpectType};
55
use crate::maybe_recover_from_interpolated_ty_qpath;
66

77
use rustc_errors::{Applicability, PResult};
8-
use rustc_span::source_map::{self, Span};
8+
use rustc_span::source_map::{self, Span, Spanned};
99
use rustc_span::symbol::{kw, sym, Symbol};
1010
use std::mem;
1111
use syntax::ast::{self, AttrStyle, AttrVec, CaptureBy, Field, Ident, Lit, DUMMY_NODE_ID};
@@ -181,17 +181,17 @@ impl<'a> Parser<'a> {
181181
};
182182

183183
let cur_op_span = self.token.span;
184-
let restrictions = if op.is_assign_like() {
184+
let restrictions = if op.node.is_assign_like() {
185185
self.restrictions & Restrictions::NO_STRUCT_LITERAL
186186
} else {
187187
self.restrictions
188188
};
189-
let prec = op.precedence();
189+
let prec = op.node.precedence();
190190
if prec < min_prec {
191191
break;
192192
}
193193
// Check for deprecated `...` syntax
194-
if self.token == token::DotDotDot && op == AssocOp::DotDotEq {
194+
if self.token == token::DotDotDot && op.node == AssocOp::DotDotEq {
195195
self.err_dotdotdot_syntax(self.token.span);
196196
}
197197

@@ -200,11 +200,12 @@ impl<'a> Parser<'a> {
200200
}
201201

202202
self.bump();
203-
if op.is_comparison() {
203+
if op.node.is_comparison() {
204204
if let Some(expr) = self.check_no_chained_comparison(&lhs, &op)? {
205205
return Ok(expr);
206206
}
207207
}
208+
let op = op.node;
208209
// Special cases:
209210
if op == AssocOp::As {
210211
lhs = self.parse_assoc_op_cast(lhs, lhs_span, ExprKind::Cast)?;
@@ -298,7 +299,7 @@ impl<'a> Parser<'a> {
298299
}
299300

300301
fn should_continue_as_assoc_expr(&mut self, lhs: &Expr) -> bool {
301-
match (self.expr_is_complete(lhs), self.check_assoc_op()) {
302+
match (self.expr_is_complete(lhs), self.check_assoc_op().map(|op| op.node)) {
302303
// Semi-statement forms are odd:
303304
// See https://github.com/rust-lang/rust/issues/29071
304305
(true, None) => false,
@@ -343,19 +344,22 @@ impl<'a> Parser<'a> {
343344
/// The method does not advance the current token.
344345
///
345346
/// Also performs recovery for `and` / `or` which are mistaken for `&&` and `||` respectively.
346-
fn check_assoc_op(&self) -> Option<AssocOp> {
347-
match (AssocOp::from_token(&self.token), &self.token.kind) {
348-
(op @ Some(_), _) => op,
349-
(None, token::Ident(sym::and, false)) => {
350-
self.error_bad_logical_op("and", "&&", "conjunction");
351-
Some(AssocOp::LAnd)
352-
}
353-
(None, token::Ident(sym::or, false)) => {
354-
self.error_bad_logical_op("or", "||", "disjunction");
355-
Some(AssocOp::LOr)
356-
}
357-
_ => None,
358-
}
347+
fn check_assoc_op(&self) -> Option<Spanned<AssocOp>> {
348+
Some(Spanned {
349+
node: match (AssocOp::from_token(&self.token), &self.token.kind) {
350+
(Some(op), _) => op,
351+
(None, token::Ident(sym::and, false)) => {
352+
self.error_bad_logical_op("and", "&&", "conjunction");
353+
AssocOp::LAnd
354+
}
355+
(None, token::Ident(sym::or, false)) => {
356+
self.error_bad_logical_op("or", "||", "disjunction");
357+
AssocOp::LOr
358+
}
359+
_ => return None,
360+
},
361+
span: self.token.span,
362+
})
359363
}
360364

361365
/// Error on `and` and `or` suggesting `&&` and `||` respectively.

‎src/test/ui/did_you_mean/issue-40396.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
fn main() {
22
(0..13).collect<Vec<i32>>();
3-
//~^ ERROR chained comparison
3+
//~^ ERROR comparison operators cannot be chained
44
Vec<i32>::new();
5-
//~^ ERROR chained comparison
5+
//~^ ERROR comparison operators cannot be chained
66
(0..13).collect<Vec<i32>();
7-
//~^ ERROR chained comparison
7+
//~^ ERROR comparison operators cannot be chained
88
}

‎src/test/ui/did_you_mean/issue-40396.stderr

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,23 @@
1-
error: chained comparison operators require parentheses
1+
error: comparison operators cannot be chained
22
--> $DIR/issue-40396.rs:2:20
33
|
44
LL | (0..13).collect<Vec<i32>>();
55
| ^^^^^
66
|
7+
help: split the comparison into two...
8+
|
9+
LL | (0..13).collect < Vec && Vec <i32>>();
10+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11+
help: ...or parenthesize one of the comparisons
12+
|
13+
LL | ((0..13).collect < Vec) <i32>>();
14+
| ^^^^^^^^^^^^^^^^^^^^^^^^^
715
help: use `::<...>` instead of `<...>` to specify type arguments
816
|
917
LL | (0..13).collect::<Vec<i32>>();
1018
| ^^
1119

12-
error: chained comparison operators require parentheses
20+
error: comparison operators cannot be chained
1321
--> $DIR/issue-40396.rs:4:8
1422
|
1523
LL | Vec<i32>::new();
@@ -20,12 +28,20 @@ help: use `::<...>` instead of `<...>` to specify type arguments
2028
LL | Vec::<i32>::new();
2129
| ^^
2230

23-
error: chained comparison operators require parentheses
31+
error: comparison operators cannot be chained
2432
--> $DIR/issue-40396.rs:6:20
2533
|
2634
LL | (0..13).collect<Vec<i32>();
2735
| ^^^^^
2836
|
37+
help: split the comparison into two...
38+
|
39+
LL | (0..13).collect < Vec && Vec <i32>();
40+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
41+
help: ...or parenthesize one of the comparisons
42+
|
43+
LL | ((0..13).collect < Vec) <i32>();
44+
| ^^^^^^^^^^^^^^^^^^^^^^^^^
2945
help: use `::<...>` instead of `<...>` to specify type arguments
3046
|
3147
LL | (0..13).collect::<Vec<i32>();

‎src/test/ui/parser/chained-comparison-suggestion.rs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Check that we get nice suggestions when attempting a chained comparison.
2+
3+
fn comp1() {
4+
1 < 2 <= 3; //~ ERROR comparison operators cannot be chained
5+
//~^ ERROR mismatched types
6+
}
7+
8+
fn comp2() {
9+
1 < 2 < 3; //~ ERROR comparison operators cannot be chained
10+
}
11+
12+
fn comp3() {
13+
1 <= 2 < 3; //~ ERROR comparison operators cannot be chained
14+
//~^ ERROR mismatched types
15+
}
16+
17+
fn comp4() {
18+
1 <= 2 <= 3; //~ ERROR comparison operators cannot be chained
19+
//~^ ERROR mismatched types
20+
}
21+
22+
fn comp5() {
23+
1 > 2 >= 3; //~ ERROR comparison operators cannot be chained
24+
//~^ ERROR mismatched types
25+
}
26+
27+
fn comp6() {
28+
1 > 2 > 3; //~ ERROR comparison operators cannot be chained
29+
}
30+
31+
fn comp7() {
32+
1 >= 2 > 3; //~ ERROR comparison operators cannot be chained
33+
}
34+
35+
fn comp8() {
36+
1 >= 2 >= 3; //~ ERROR comparison operators cannot be chained
37+
//~^ ERROR mismatched types
38+
}
39+
40+
fn main() {}

0 commit comments

Comments
 (0)