Skip to content

Commit

Permalink
Rollup merge of rust-lang#68108 - varkor:chained-comparison-suggestio…
Browse files Browse the repository at this point in the history 
…ns, r=Centril

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 rust-lang#65659.
  • Loading branch information
@JohnTitor
JohnTitor authored Jan 11, 2020
2 parents 7dcb9eb + 088a180 commit a727425
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 a727425

Please sign in to comment.