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

Tweak chained operators diagnostic #69878

Merged
merged 2 commits into from
Mar 26, 2020
Merged

Tweak chained operators diagnostic #69878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89571a1
Tweak chained operators diagnostic
estebank Mar 10, 2020
4832f3f
review comments
estebank Mar 26, 2020
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
155 changes: 103 additions & 52 deletions src/librustc_parse/parser/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use rustc_span::symbol::kw;
use rustc_span::{MultiSpan, Span, SpanSnippetError, DUMMY_SP};

use log::{debug, trace};
use std::mem;

const TURBOFISH: &str = "use `::<...>` instead of `<...>` to specify type arguments";

Expand Down Expand Up @@ -459,9 +458,28 @@ impl<'a> Parser<'a> {
err: &mut DiagnosticBuilder<'_>,
inner_op: &Expr,
outer_op: &Spanned<AssocOp>,
) {
) -> bool /* advanced the cursor */ {
if let ExprKind::Binary(op, ref l1, ref r1) = inner_op.kind {
match (op.node, &outer_op.node) {
if let ExprKind::Field(_, ident) = l1.kind {
if ident.as_str().parse::<i32>().is_err() && !matches!(r1.kind, ExprKind::Lit(_)) {
// The parser has encountered `foo.bar<baz`, the likelihood of the turbofish
// suggestion being the only one to apply is high.
return false;
}
}
let mut enclose = |left: Span, right: Span| {
err.multipart_suggestion(
"parenthesize the comparison",
vec![
(left.shrink_to_lo(), "(".to_string()),
(right.shrink_to_hi(), ")".to_string()),
],
Applicability::MaybeIncorrect,
);
};
return match (op.node, &outer_op.node) {
// `x == y == z`
(BinOpKind::Eq, AssocOp::Equal) |
// `x < y < z` and friends.
(BinOpKind::Lt, AssocOp::Less) | (BinOpKind::Lt, AssocOp::LessEqual) |
(BinOpKind::Le, AssocOp::LessEqual) | (BinOpKind::Le, AssocOp::Less) |
Expand All @@ -472,35 +490,55 @@ impl<'a> Parser<'a> {
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(),
),
err.span_suggestion_verbose(
inner_op.span.shrink_to_hi(),
"split the comparison into two",
format!(" && {}", expr_to_str(&r1)),
Applicability::MaybeIncorrect,
);
false // Keep the current parse behavior, where the AST is `(x < y) < z`.
}
_ => {}
}
// `x == y < z`
(BinOpKind::Eq, AssocOp::Less) | (BinOpKind::Eq, AssocOp::LessEqual) |
(BinOpKind::Eq, AssocOp::Greater) | (BinOpKind::Eq, AssocOp::GreaterEqual) => {
// Consume `z`/outer-op-rhs.
let snapshot = self.clone();
match self.parse_expr() {
Ok(r2) => {
// We are sure that outer-op-rhs could be consumed, the suggestion is
// likely correct.
enclose(r1.span, r2.span);
true
}
Err(mut expr_err) => {
expr_err.cancel();
*self = snapshot;
false
}
}
}
// `x > y == z`
(BinOpKind::Lt, AssocOp::Equal) | (BinOpKind::Le, AssocOp::Equal) |
(BinOpKind::Gt, AssocOp::Equal) | (BinOpKind::Ge, AssocOp::Equal) => {
let snapshot = self.clone();
// At this point it is always valid to enclose the lhs in parentheses, no
// further checks are necessary.
match self.parse_expr() {
Ok(_) => {
enclose(l1.span, r1.span);
true
}
Err(mut expr_err) => {
expr_err.cancel();
*self = snapshot;
false
}
}
}
_ => false,
};
}
false
}

/// Produces an error if comparison operators are chained (RFC #558).
Expand Down Expand Up @@ -534,31 +572,26 @@ impl<'a> Parser<'a> {
|this: &Self, span| Ok(Some(this.mk_expr(span, ExprKind::Err, AttrVec::new())));

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_token.span);
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);
ExprKind::Binary(op, ref l1, ref r1) if op.node.is_comparison() => {
let mut err = self.struct_span_err(
vec![op.span, self.prev_token.span],
"comparison operators cannot be chained",
);

let suggest = |err: &mut DiagnosticBuilder<'_>| {
err.span_suggestion_verbose(
op_span.shrink_to_lo(),
op.span.shrink_to_lo(),
TURBOFISH,
"::".to_string(),
Applicability::MaybeIncorrect,
);
};

if op.node == BinOpKind::Lt &&
outer_op.node == AssocOp::Less || // Include `<` to provide this recommendation
outer_op.node == AssocOp::Greater
// even in a case like the following:
// Include `<` to provide this recommendation even in a case like
// `Foo<Bar<Baz<Qux, ()>>>`
if op.node == BinOpKind::Lt && outer_op.node == AssocOp::Less
|| outer_op.node == AssocOp::Greater
{
// Foo<Bar<Baz<Qux, ()>>>
if outer_op.node == AssocOp::Less {
let snapshot = self.clone();
self.bump();
Expand All @@ -572,7 +605,7 @@ impl<'a> Parser<'a> {
{
// We don't have `foo< bar >(` or `foo< bar >::`, so we rewind the
// parser and bail out.
mem::replace(self, snapshot.clone());
*self = snapshot.clone();
}
}
return if token::ModSep == self.token.kind {
Expand All @@ -597,7 +630,7 @@ impl<'a> Parser<'a> {
expr_err.cancel();
// Not entirely sure now, but we bubble the error up with the
// suggestion.
mem::replace(self, snapshot);
*self = snapshot;
Err(err)
}
}
Expand All @@ -617,15 +650,33 @@ impl<'a> Parser<'a> {
}
}
} else {
// All we know is that this is `foo < bar >` and *nothing* else. Try to
// be helpful, but don't attempt to recover.
err.help(TURBOFISH);
err.help("or use `(...)` if you meant to specify fn arguments");
// These cases cause too many knock-down errors, bail out (#61329).
Err(err)
if !matches!(l1.kind, ExprKind::Lit(_))
&& !matches!(r1.kind, ExprKind::Lit(_))
{
// All we know is that this is `foo < bar >` and *nothing* else. Try to
// be helpful, but don't attempt to recover.
err.help(TURBOFISH);
err.help("or use `(...)` if you meant to specify fn arguments");
}

// If it looks like a genuine attempt to chain operators (as opposed to a
// misformatted turbofish, for instance), suggest a correct form.
if self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op)
{
err.emit();
mk_err_expr(self, inner_op.span.to(self.prev_token.span))
} else {
// These cases cause too many knock-down errors, bail out (#61329).
Err(err)
}
};
}
let recover =
self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op);
err.emit();
if recover {
return mk_err_expr(self, inner_op.span.to(self.prev_token.span));
}
}
_ => {}
}
Expand All @@ -643,7 +694,7 @@ impl<'a> Parser<'a> {

if self.token.kind == token::Eof {
// Not entirely sure that what we consumed were fn arguments, rollback.
mem::replace(self, snapshot);
*self = snapshot;
Err(())
} else {
// 99% certain that the suggestion is correct, continue parsing.
Expand Down
22 changes: 3 additions & 19 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
Expand Up @@ -2,16 +2,8 @@ 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>>();
Expand All @@ -21,7 +13,7 @@ error: comparison operators cannot be chained
--> $DIR/issue-40396.rs:4:8
|
LL | Vec<i32>::new();
| ^^^^^
| ^ ^
|
help: use `::<...>` instead of `<...>` to specify type arguments
|
Expand All @@ -32,16 +24,8 @@ 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
13 changes: 13 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
Expand Up @@ -37,4 +37,17 @@ fn comp8() {
//~^ ERROR mismatched types
}

fn comp9() {
1 == 2 < 3; //~ ERROR comparison operators cannot be chained
}

fn comp10() {
1 > 2 == false; //~ ERROR comparison operators cannot be chained
}

fn comp11() {
1 == 2 == 3; //~ ERROR comparison operators cannot be chained
//~^ ERROR mismatched types
}

fn main() {}
Loading