Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add suggestions when encountering chained comparisons #68108

Merged
merged 1 commit into from
Jan 12, 2020
Merged

Add suggestions when encountering chained comparisons #68108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 70 additions & 13 deletions src/librustc_parse/parser/diagnostics.rs
View file
Open in desktop
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 @@ -500,6 +501,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 @@ -515,27 +568,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 @@ -547,12 +604,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 @@ -584,7 +641,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 @@ -606,7 +663,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,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 @@ -181,17 +181,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() {
varkor marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -200,11 +200,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 @@ -298,7 +299,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 @@ -343,19 +344,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
Open in desktop
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
Open in desktop
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
Open in desktop
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