Skip to content

Commit

Permalink
Rollup merge of #68108 - varkor:chained-comparison-suggestions, r=Cen…
Browse files Browse the repository at this point in the history 
…tril

Add suggestions when encountering chained comparisons

Ideally, we'd also prevent the type error, which is just extra noise, but that will require moving the error from the parser, and I think the suggestion makes things clear enough for now.

Fixes #65659.
  • Loading branch information
@Centril
Centril authored Jan 12, 2020
2 parents bbd210e + 088a180 commit 82c19b4
Show file tree
Hide file tree
Showing 8 changed files with 335 additions and 49 deletions.
83 changes: 70 additions & 13 deletions src/librustc_parse/parser/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use rustc_data_structures::fx::FxHashSet;
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::{
Expand Down Expand Up @@ -491,6 +492,58 @@ impl<'a> Parser<'a> {
}
}

/// 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`).
Expand All @@ -506,27 +559,31 @@ impl<'a> Parser<'a> {
/// / \
/// 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(
Expand All @@ -538,12 +595,12 @@ impl<'a> Parser<'a> {
};

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.
Expand Down Expand Up @@ -575,7 +632,7 @@ impl<'a> Parser<'a> {
// 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();
Expand All @@ -597,7 +654,7 @@ impl<'a> Parser<'a> {
// 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 {
Expand Down
42 changes: 23 additions & 19 deletions src/librustc_parse/parser/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::{SemiColonMode, SeqSep, TokenExpectType};
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};
Expand Down Expand Up @@ -180,17 +180,17 @@ impl<'a> Parser<'a> {
};

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);
}

Expand All @@ -199,11 +199,12 @@ impl<'a> Parser<'a> {
}

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)?;
Expand Down Expand Up @@ -297,7 +298,7 @@ impl<'a> Parser<'a> {
}

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,
Expand Down Expand Up @@ -342,19 +343,22 @@ impl<'a> Parser<'a> {
/// 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.
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/did_you_mean/issue-40396.rs
View file
Original file line number Diff line number Diff line change
@@ -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
}
22 changes: 19 additions & 3 deletions src/test/ui/did_you_mean/issue-40396.stderr
View file
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -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>();
Expand Down
40 changes: 40 additions & 0 deletions src/test/ui/parser/chained-comparison-suggestion.rs
View file
Original file line number Diff line number Diff line change
@@ -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() {}
Loading

0 comments on commit 82c19b4

Please sign in to comment.