Add suggestions when encountering chained comparisons

This commit is contained in:
varkor 2020-01-11 00:19:09 +00:00
parent 2d8d559bbe
commit 088a1808d2
8 changed files with 335 additions and 49 deletions

View File

@ -4,6 +4,7 @@
use rustc_error_codes::*;
use rustc_errors::{pluralize, struct_span_err};
use rustc_errors::{Applicability, DiagnosticBuilder, Handler, PResult};
use rustc_span::source_map::Spanned;
use rustc_span::symbol::kw;
use rustc_span::{MultiSpan, Span, SpanSnippetError, DUMMY_SP};
use syntax::ast::{
@ -500,6 +501,58 @@ pub(super) fn check_trailing_angle_brackets(&mut self, segment: &PathSegment, en
}
}
/// Check to see if a pair of chained operators looks like an attempt at chained comparison,
/// e.g. `1 < x <= 3`. If so, suggest either splitting the comparison into two, or
/// parenthesising the leftmost comparison.
fn attempt_chained_comparison_suggestion(
&mut self,
err: &mut DiagnosticBuilder<'_>,
inner_op: &Expr,
outer_op: &Spanned<AssocOp>,
) {
if let ExprKind::Binary(op, ref l1, ref r1) = inner_op.kind {
match (op.node, &outer_op.node) {
// `x < y < z` and friends.
(BinOpKind::Lt, AssocOp::Less) | (BinOpKind::Lt, AssocOp::LessEqual) |
(BinOpKind::Le, AssocOp::LessEqual) | (BinOpKind::Le, AssocOp::Less) |
// `x > y > z` and friends.
(BinOpKind::Gt, AssocOp::Greater) | (BinOpKind::Gt, AssocOp::GreaterEqual) |
(BinOpKind::Ge, AssocOp::GreaterEqual) | (BinOpKind::Ge, AssocOp::Greater) => {
let expr_to_str = |e: &Expr| {
self.span_to_snippet(e.span)
.unwrap_or_else(|_| pprust::expr_to_string(&e))
};
err.span_suggestion(
inner_op.span.to(outer_op.span),
"split the comparison into two...",
format!(
"{} {} {} && {} {}",
expr_to_str(&l1),
op.node.to_string(),
expr_to_str(&r1),
expr_to_str(&r1),
outer_op.node.to_ast_binop().unwrap().to_string(),
),
Applicability::MaybeIncorrect,
);
err.span_suggestion(
inner_op.span.to(outer_op.span),
"...or parenthesize one of the comparisons",
format!(
"({} {} {}) {}",
expr_to_str(&l1),
op.node.to_string(),
expr_to_str(&r1),
outer_op.node.to_ast_binop().unwrap().to_string(),
),
Applicability::MaybeIncorrect,
);
}
_ => {}
}
}
}
/// Produces an error if comparison operators are chained (RFC #558).
/// We only need to check the LHS, not the RHS, because all comparison ops have same
/// precedence (see `fn precedence`) and are left-associative (see `fn fixity`).
@ -515,27 +568,31 @@ pub(super) fn check_trailing_angle_brackets(&mut self, segment: &PathSegment, en
/// / \
/// inner_op r2
/// / \
/// l1 r1
/// l1 r1
pub(super) fn check_no_chained_comparison(
&mut self,
lhs: &Expr,
outer_op: &AssocOp,
inner_op: &Expr,
outer_op: &Spanned<AssocOp>,
) -> PResult<'a, Option<P<Expr>>> {
debug_assert!(
outer_op.is_comparison(),
outer_op.node.is_comparison(),
"check_no_chained_comparison: {:?} is not comparison",
outer_op,
outer_op.node,
);
let mk_err_expr =
|this: &Self, span| Ok(Some(this.mk_expr(span, ExprKind::Err, AttrVec::new())));
match lhs.kind {
match inner_op.kind {
ExprKind::Binary(op, _, _) if op.node.is_comparison() => {
// Respan to include both operators.
let op_span = op.span.to(self.prev_span);
let mut err = self
.struct_span_err(op_span, "chained comparison operators require parentheses");
let mut err =
self.struct_span_err(op_span, "comparison operators cannot be chained");
// If it looks like a genuine attempt to chain operators (as opposed to a
// misformatted turbofish, for instance), suggest a correct form.
self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op);
let suggest = |err: &mut DiagnosticBuilder<'_>| {
err.span_suggestion_verbose(
@ -547,12 +604,12 @@ pub(super) fn check_no_chained_comparison(
};
if op.node == BinOpKind::Lt &&
*outer_op == AssocOp::Less || // Include `<` to provide this recommendation
*outer_op == AssocOp::Greater
outer_op.node == AssocOp::Less || // Include `<` to provide this recommendation
outer_op.node == AssocOp::Greater
// even in a case like the following:
{
// Foo<Bar<Baz<Qux, ()>>>
if *outer_op == AssocOp::Less {
if outer_op.node == AssocOp::Less {
let snapshot = self.clone();
self.bump();
// So far we have parsed `foo<bar<`, consume the rest of the type args.
@ -584,7 +641,7 @@ pub(super) fn check_no_chained_comparison(
// FIXME: actually check that the two expressions in the binop are
// paths and resynthesize new fn call expression instead of using
// `ExprKind::Err` placeholder.
mk_err_expr(self, lhs.span.to(self.prev_span))
mk_err_expr(self, inner_op.span.to(self.prev_span))
}
Err(mut expr_err) => {
expr_err.cancel();
@ -606,7 +663,7 @@ pub(super) fn check_no_chained_comparison(
// FIXME: actually check that the two expressions in the binop are
// paths and resynthesize new fn call expression instead of using
// `ExprKind::Err` placeholder.
mk_err_expr(self, lhs.span.to(self.prev_span))
mk_err_expr(self, inner_op.span.to(self.prev_span))
}
}
} else {

View File

@ -5,7 +5,7 @@
use crate::maybe_recover_from_interpolated_ty_qpath;
use rustc_errors::{Applicability, PResult};
use rustc_span::source_map::{self, Span};
use rustc_span::source_map::{self, Span, Spanned};
use rustc_span::symbol::{kw, sym, Symbol};
use std::mem;
use syntax::ast::{self, AttrStyle, AttrVec, CaptureBy, Field, Ident, Lit, DUMMY_NODE_ID};
@ -181,17 +181,17 @@ pub(super) fn parse_assoc_expr_with(
};
let cur_op_span = self.token.span;
let restrictions = if op.is_assign_like() {
let restrictions = if op.node.is_assign_like() {
self.restrictions & Restrictions::NO_STRUCT_LITERAL
} else {
self.restrictions
};
let prec = op.precedence();
let prec = op.node.precedence();
if prec < min_prec {
break;
}
// Check for deprecated `...` syntax
if self.token == token::DotDotDot && op == AssocOp::DotDotEq {
if self.token == token::DotDotDot && op.node == AssocOp::DotDotEq {
self.err_dotdotdot_syntax(self.token.span);
}
@ -200,11 +200,12 @@ pub(super) fn parse_assoc_expr_with(
}
self.bump();
if op.is_comparison() {
if op.node.is_comparison() {
if let Some(expr) = self.check_no_chained_comparison(&lhs, &op)? {
return Ok(expr);
}
}
let op = op.node;
// Special cases:
if op == AssocOp::As {
lhs = self.parse_assoc_op_cast(lhs, lhs_span, ExprKind::Cast)?;
@ -298,7 +299,7 @@ pub(super) fn parse_assoc_expr_with(
}
fn should_continue_as_assoc_expr(&mut self, lhs: &Expr) -> bool {
match (self.expr_is_complete(lhs), self.check_assoc_op()) {
match (self.expr_is_complete(lhs), self.check_assoc_op().map(|op| op.node)) {
// Semi-statement forms are odd:
// See https://github.com/rust-lang/rust/issues/29071
(true, None) => false,
@ -343,19 +344,22 @@ fn error_found_expr_would_be_stmt(&self, lhs: &Expr) {
/// The method does not advance the current token.
///
/// Also performs recovery for `and` / `or` which are mistaken for `&&` and `||` respectively.
fn check_assoc_op(&self) -> Option<AssocOp> {
match (AssocOp::from_token(&self.token), &self.token.kind) {
(op @ Some(_), _) => op,
(None, token::Ident(sym::and, false)) => {
self.error_bad_logical_op("and", "&&", "conjunction");
Some(AssocOp::LAnd)
}
(None, token::Ident(sym::or, false)) => {
self.error_bad_logical_op("or", "||", "disjunction");
Some(AssocOp::LOr)
}
_ => None,
}
fn check_assoc_op(&self) -> Option<Spanned<AssocOp>> {
Some(Spanned {
node: match (AssocOp::from_token(&self.token), &self.token.kind) {
(Some(op), _) => op,
(None, token::Ident(sym::and, false)) => {
self.error_bad_logical_op("and", "&&", "conjunction");
AssocOp::LAnd
}
(None, token::Ident(sym::or, false)) => {
self.error_bad_logical_op("or", "||", "disjunction");
AssocOp::LOr
}
_ => return None,
},
span: self.token.span,
})
}
/// Error on `and` and `or` suggesting `&&` and `||` respectively.

View File

@ -1,8 +1,8 @@
fn main() {
(0..13).collect<Vec<i32>>();
//~^ ERROR chained comparison
//~^ ERROR comparison operators cannot be chained
Vec<i32>::new();
//~^ ERROR chained comparison
//~^ ERROR comparison operators cannot be chained
(0..13).collect<Vec<i32>();
//~^ ERROR chained comparison
//~^ ERROR comparison operators cannot be chained
}

View File

@ -1,15 +1,23 @@
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/issue-40396.rs:2:20
|
LL | (0..13).collect<Vec<i32>>();
| ^^^^^
|
help: split the comparison into two...
|
LL | (0..13).collect < Vec && Vec <i32>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | ((0..13).collect < Vec) <i32>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
help: use `::<...>` instead of `<...>` to specify type arguments
|
LL | (0..13).collect::<Vec<i32>>();
| ^^
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/issue-40396.rs:4:8
|
LL | Vec<i32>::new();
@ -20,12 +28,20 @@ help: use `::<...>` instead of `<...>` to specify type arguments
LL | Vec::<i32>::new();
| ^^
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/issue-40396.rs:6:20
|
LL | (0..13).collect<Vec<i32>();
| ^^^^^
|
help: split the comparison into two...
|
LL | (0..13).collect < Vec && Vec <i32>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | ((0..13).collect < Vec) <i32>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
help: use `::<...>` instead of `<...>` to specify type arguments
|
LL | (0..13).collect::<Vec<i32>();

View File

@ -0,0 +1,40 @@
// Check that we get nice suggestions when attempting a chained comparison.
fn comp1() {
1 < 2 <= 3; //~ ERROR comparison operators cannot be chained
//~^ ERROR mismatched types
}
fn comp2() {
1 < 2 < 3; //~ ERROR comparison operators cannot be chained
}
fn comp3() {
1 <= 2 < 3; //~ ERROR comparison operators cannot be chained
//~^ ERROR mismatched types
}
fn comp4() {
1 <= 2 <= 3; //~ ERROR comparison operators cannot be chained
//~^ ERROR mismatched types
}
fn comp5() {
1 > 2 >= 3; //~ ERROR comparison operators cannot be chained
//~^ ERROR mismatched types
}
fn comp6() {
1 > 2 > 3; //~ ERROR comparison operators cannot be chained
}
fn comp7() {
1 >= 2 > 3; //~ ERROR comparison operators cannot be chained
}
fn comp8() {
1 >= 2 >= 3; //~ ERROR comparison operators cannot be chained
//~^ ERROR mismatched types
}
fn main() {}

View File

@ -0,0 +1,159 @@
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:4:7
|
LL | 1 < 2 <= 3;
| ^^^^^^
|
help: split the comparison into two...
|
LL | 1 < 2 && 2 <= 3;
| ^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 < 2) <= 3;
| ^^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:9:7
|
LL | 1 < 2 < 3;
| ^^^^^
|
= help: use `::<...>` instead of `<...>` to specify type arguments
= help: or use `(...)` if you meant to specify fn arguments
help: split the comparison into two...
|
LL | 1 < 2 && 2 < 3;
| ^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 < 2) < 3;
| ^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:13:7
|
LL | 1 <= 2 < 3;
| ^^^^^^
|
help: split the comparison into two...
|
LL | 1 <= 2 && 2 < 3;
| ^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 <= 2) < 3;
| ^^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:18:7
|
LL | 1 <= 2 <= 3;
| ^^^^^^^
|
help: split the comparison into two...
|
LL | 1 <= 2 && 2 <= 3;
| ^^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 <= 2) <= 3;
| ^^^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:23:7
|
LL | 1 > 2 >= 3;
| ^^^^^^
|
help: split the comparison into two...
|
LL | 1 > 2 && 2 >= 3;
| ^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 > 2) >= 3;
| ^^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:28:7
|
LL | 1 > 2 > 3;
| ^^^^^
|
= help: use `::<...>` instead of `<...>` to specify type arguments
= help: or use `(...)` if you meant to specify fn arguments
help: split the comparison into two...
|
LL | 1 > 2 && 2 > 3;
| ^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 > 2) > 3;
| ^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:32:7
|
LL | 1 >= 2 > 3;
| ^^^^^^
|
= help: use `::<...>` instead of `<...>` to specify type arguments
= help: or use `(...)` if you meant to specify fn arguments
help: split the comparison into two...
|
LL | 1 >= 2 && 2 > 3;
| ^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 >= 2) > 3;
| ^^^^^^^^^^
error: comparison operators cannot be chained
--> $DIR/chained-comparison-suggestion.rs:36:7
|
LL | 1 >= 2 >= 3;
| ^^^^^^^
|
help: split the comparison into two...
|
LL | 1 >= 2 && 2 >= 3;
| ^^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (1 >= 2) >= 3;
| ^^^^^^^^^^^
error[E0308]: mismatched types
--> $DIR/chained-comparison-suggestion.rs:4:14
|
LL | 1 < 2 <= 3;
| ^ expected `bool`, found integer
error[E0308]: mismatched types
--> $DIR/chained-comparison-suggestion.rs:13:14
|
LL | 1 <= 2 < 3;
| ^ expected `bool`, found integer
error[E0308]: mismatched types
--> $DIR/chained-comparison-suggestion.rs:18:15
|
LL | 1 <= 2 <= 3;
| ^ expected `bool`, found integer
error[E0308]: mismatched types
--> $DIR/chained-comparison-suggestion.rs:23:14
|
LL | 1 > 2 >= 3;
| ^ expected `bool`, found integer
error[E0308]: mismatched types
--> $DIR/chained-comparison-suggestion.rs:36:15
|
LL | 1 >= 2 >= 3;
| ^ expected `bool`, found integer
error: aborting due to 13 previous errors
For more information about this error, try `rustc --explain E0308`.

View File

@ -3,24 +3,26 @@ fn f<T>() {}
fn main() {
false == false == false;
//~^ ERROR chained comparison operators require parentheses
//~^ ERROR comparison operators cannot be chained
false == 0 < 2;
//~^ ERROR chained comparison operators require parentheses
//~^ ERROR comparison operators cannot be chained
//~| ERROR mismatched types
//~| ERROR mismatched types
f<X>();
//~^ ERROR chained comparison operators require parentheses
//~^ ERROR comparison operators cannot be chained
//~| HELP use `::<...>` instead of `<...>` to specify type arguments
f<Result<Option<X>, Option<Option<X>>>(1, 2);
//~^ ERROR chained comparison operators require parentheses
//~^ ERROR comparison operators cannot be chained
//~| HELP split the comparison into two...
//~| ...or parenthesize one of the comparisons
//~| HELP use `::<...>` instead of `<...>` to specify type arguments
use std::convert::identity;
let _ = identity<u8>;
//~^ ERROR chained comparison operators require parentheses
//~^ ERROR comparison operators cannot be chained
//~| HELP use `::<...>` instead of `<...>` to specify type arguments
//~| HELP or use `(...)` if you meant to specify fn arguments
}

View File

@ -1,16 +1,16 @@
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/require-parens-for-chained-comparison.rs:5:11
|
LL | false == false == false;
| ^^^^^^^^^^^
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/require-parens-for-chained-comparison.rs:8:11
|
LL | false == 0 < 2;
| ^^^^^^
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/require-parens-for-chained-comparison.rs:13:6
|
LL | f<X>();
@ -21,19 +21,27 @@ help: use `::<...>` instead of `<...>` to specify type arguments
LL | f::<X>();
| ^^
error: chained comparison operators require parentheses
error: comparison operators cannot be chained
--> $DIR/require-parens-for-chained-comparison.rs:17:6
|
LL | f<Result<Option<X>, Option<Option<X>>>(1, 2);
| ^^^^^^^^
|
help: split the comparison into two...
|
LL | f < Result && Result <Option<X>, Option<Option<X>>>(1, 2);
| ^^^^^^^^^^^^^^^^^^^^^^
help: ...or parenthesize one of the comparisons
|
LL | (f < Result) <Option<X>, Option<Option<X>>>(1, 2);
| ^^^^^^^^^^^^^^
help: use `::<...>` instead of `<...>` to specify type arguments
|
LL | f::<Result<Option<X>, Option<Option<X>>>(1, 2);
| ^^
error: chained comparison operators require parentheses
--> $DIR/require-parens-for-chained-comparison.rs:22:21
error: comparison operators cannot be chained
--> $DIR/require-parens-for-chained-comparison.rs:24:21
|
LL | let _ = identity<u8>;
| ^^^^