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.

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

Suggest using matches or adding == on x == a || b || c #128159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 11 additions & 6 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1882,12 +1882,17 @@ impl Expr<'_> {
/// To a first-order approximation, is this a pattern?
pub fn is_approximately_pattern(&self) -> bool {
match &self.kind {
ExprKind::Array(_)
| ExprKind::Call(..)
| ExprKind::Tup(_)
| ExprKind::Lit(_)
| ExprKind::Path(_)
| ExprKind::Struct(..) => true,
ExprKind::Array(exprs) | ExprKind::Tup(exprs) => {
exprs.iter().all(|expr| expr.is_approximately_pattern())
}
ExprKind::Struct(_, fields, None) => {
fields.iter().all(|field| field.expr.is_approximately_pattern())
}
ExprKind::Call(callee, exprs) => {
callee.is_approximately_pattern()
&& exprs.iter().all(|expr| expr.is_approximately_pattern())
}
ExprKind::Lit(_) | ExprKind::Path(_) => true,
_ => false,
}
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_hir_typeck/src/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pub fn emit_coerce_suggestions(
&self,
err: &mut Diag<'_>,
expr: &hir::Expr<'tcx>,
expr: &'tcx hir::Expr<'tcx>,
expr_ty: Ty<'tcx>,
expected: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
Expand All @@ -86,6 +86,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

self.annotate_expected_due_to_let_ty(err, expr, error);
self.annotate_loop_expected_due_to_inference(err, expr, error);
self.annotate_incorrect_or_expr(err, expr, expr_ty, expected);

// FIXME(#73154): For now, we do leak check when coercing function
// pointers in typeck, instead of only during borrowck. This can lead
Expand Down
109 changes: 109 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3365,4 +3365,113 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err.span_label(block.span, "this block is missing a tail expression");
}
}

pub(crate) fn annotate_incorrect_or_expr(
&self,
diag: &mut Diag<'_>,
expr: &'tcx hir::Expr<'tcx>,
expr_ty: Ty<'tcx>,
expected_ty: Ty<'tcx>,
) {
if expected_ty != self.tcx.types.bool {
return;
}
let hir::Node::Expr(&hir::Expr {
kind:
hir::ExprKind::Binary(
hir::BinOp { node: hir::BinOpKind::Or, span: binop_span },
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
lhs,
rhs,
),
hir_id: parent_hir_id,
span: full_span,
..
}) = self.tcx.parent_hir_node(expr.hir_id)
else {
return;
};
Comment on lines +3383 to +3392
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best if we documented this code. It took some time for me to fully realize what we are doing here. Consider documenting that:

  • We start to see if a suggestion is applicable when b in x == a || b isn't a bool. (type mismatch)
  • We don't try to suggest when b cannot be coerced to x.

if rhs.hir_id != expr.hir_id {
return;
}
let hir::Expr {
kind:
hir::ExprKind::Binary(hir::BinOp { node: hir::BinOpKind::Eq, span: eq_span }, lhs, _),
..
} = *lhs
else {
return;
};
let Some(lhs_ty) = self.typeck_results.borrow().expr_ty_opt(lhs) else {
return;
};
// Coercion here is not totally right, but w/e.
if !self.can_coerce(expr_ty, lhs_ty) {
Comment on lines +3407 to +3408
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks one of the types of the exprs can be coerced. I can think of a weird way where the user wanted to write x == a | b | c in place of x == a || b || c where c has a different type to a or b or x but its BitOr::Output is the same type so the equality would work. I think you might want to bullet-proof this by checking that everything in the binops have the same types.

return;
}

// Track whether all of the exprs to the right, i.e. `|| a || b || c` are all pattern-like.
let mut is_literal = rhs.is_approximately_pattern();
// Track the span of the outermost `||` expr.
let mut full_span = full_span;

// Walk up the expr tree gathering up the binop spans of any subsequent `|| a || b || c`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to move this comment about walking to before the comment for is_literal so the reader knows what we would be doing better

let mut expr_hir_id = parent_hir_id;
let mut binop_spans = vec![binop_span];
while let hir::Node::Expr(&hir::Expr {
kind:
hir::ExprKind::Binary(
hir::BinOp { node: hir::BinOpKind::Or, span: binop_span },
lhs,
rhs,
),
hir_id: parent_hir_id,
span,
..
}) = self.tcx.parent_hir_node(expr_hir_id)
&& lhs.hir_id == expr_hir_id
{
binop_spans.push(binop_span);
expr_hir_id = parent_hir_id;
full_span = span;
is_literal |= rhs.is_approximately_pattern();
}

// If the type is structural peq, then suggest `matches!(x, a | b | c)`.
// Otherwise, suggest adding `x == ` to every `||`.
if is_literal
// I know this logic may look a bit sketchy, but int vars don't implement
// `StructuralPeq` b/c they're unconstrained, so just check for those manually.
&& (self.tcx.lang_items().structural_peq_trait().is_some_and(|structural_peq_def_id| {
self.type_implements_trait(structural_peq_def_id, [lhs_ty], self.param_env)
.must_apply_modulo_regions()
}) || lhs_ty.is_integral())
{
let eq_span = lhs.span.shrink_to_hi().to(eq_span);
diag.multipart_suggestion_verbose(
"use `matches!()` to match against multiple values",
[
(full_span.shrink_to_lo(), "matches!(".to_string()),
(full_span.shrink_to_hi(), ")".to_string()),
(eq_span, ",".to_string()),
]
.into_iter()
.chain(binop_spans.into_iter().map(|span| (span, "|".to_string())))
.collect(),
Applicability::MachineApplicable,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be MachineApplicable. We've only approximated that the exprs might be valid patterns but we aren't sure.

);
} else if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = lhs.kind
&& let Res::Local(local) = path.res
{
let local = self.tcx.hir().name(local);
let local_name = format!(" {local} ==");
diag.multipart_suggestion_verbose(
format!("use `{local} == ...` to match against several different values"),
binop_spans
.into_iter()
.map(|span| (span.shrink_to_hi(), local_name.clone()))
.collect(),
Applicability::MachineApplicable,
);
}
}
}
15 changes: 15 additions & 0 deletions tests/ui/binop/nested-or-of-eq.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
fn main() {
let x = 1;
if x == 1 || 2 || 3 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for x == 1 || 2 || a where let a = 3;

//~^ ERROR mismatched types
//~| ERROR mismatched types
println!("Was 1 or 2 or 3");
}

let x = 1.0;
if x == 1.0 || 2.0 || 3.0 {
//~^ ERROR mismatched types
//~| ERROR mismatched types
println!("Was 1.0 or 2.0 or 3.0");
}
}
45 changes: 45 additions & 0 deletions tests/ui/binop/nested-or-of-eq.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
error[E0308]: mismatched types
--> $DIR/nested-or-of-eq.rs:3:18
|
LL | if x == 1 || 2 || 3 {
| ------ ^ expected `bool`, found integer
| |
| expected because this is `bool`
|
help: use `matches!()` to match against multiple values
|
LL | if matches!(x, 1 | 2 | 3) {
| +++++++++ ~ ~ ~ +

error[E0308]: mismatched types
--> $DIR/nested-or-of-eq.rs:3:23
|
LL | if x == 1 || 2 || 3 {
| ----------- ^ expected `bool`, found integer
| |
| expected because this is `bool`

error[E0308]: mismatched types
--> $DIR/nested-or-of-eq.rs:10:20
|
LL | if x == 1.0 || 2.0 || 3.0 {
| -------- ^^^ expected `bool`, found floating-point number
| |
| expected because this is `bool`
|
help: use `x == ...` to match against several different values
|
LL | if x == 1.0 || x == 2.0 || x == 3.0 {
| ++++ ++++

error[E0308]: mismatched types
--> $DIR/nested-or-of-eq.rs:10:27
|
LL | if x == 1.0 || 2.0 || 3.0 {
| --------------- ^^^ expected `bool`, found floating-point number
| |
| expected because this is `bool`

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0308`.
Loading