Skip to content

Commit

Permalink
Auto merge of rust-lang#94034 - willcrichton:fix-trait-suggestion-for…
Browse files Browse the repository at this point in the history
…-binops, r=estebank

Fix incorrect suggestion for trait bounds involving binary operators

This PR fixes rust-lang#93927, rust-lang#92347, rust-lang#93744 by replacing the bespoke trait-suggestion logic in `op.rs` with a more common code path.

The downside is that this fix causes some suggestions to not include an `Output=` type, reducing their usefulness.

Note that this causes one case in the `missing-bounds.rs` test to fail rustfix. So I would need to move that code into a separate non-fix test if this PR is otherwise acceptable.
  • Loading branch information
bors committed Apr 26, 2022
2 parents 1c988cf + dc41dba commit d6a57d3
Show file tree
Hide file tree
Showing 10 changed files with 109 additions and 161 deletions.
137 changes: 47 additions & 90 deletions compiler/rustc_typeck/src/check/op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,12 @@ use rustc_middle::ty::adjustment::{
};
use rustc_middle::ty::fold::TypeFolder;
use rustc_middle::ty::TyKind::{Adt, Array, Char, FnDef, Never, Ref, Str, Tuple, Uint};
use rustc_middle::ty::{
self, suggest_constraining_type_param, Ty, TyCtxt, TypeFoldable, TypeVisitor,
};
use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeVisitor};
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{sym, Ident};
use rustc_span::Span;
use rustc_trait_selection::infer::InferCtxtExt;
use rustc_trait_selection::traits::error_reporting::suggestions::InferCtxtExt as _;
use rustc_trait_selection::traits::{FulfillmentError, TraitEngine, TraitEngineExt};

use std::ops::ControlFlow;
Expand Down Expand Up @@ -266,7 +265,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Err(_) if lhs_ty.references_error() || rhs_ty.references_error() => self.tcx.ty_error(),
Err(errors) => {
let source_map = self.tcx.sess.source_map();
let (mut err, missing_trait, use_output) = match is_assign {
let (mut err, missing_trait, _use_output) = match is_assign {
IsAssign::Yes => {
let mut err = struct_span_err!(
self.tcx.sess,
Expand Down Expand Up @@ -449,39 +448,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// concatenation (e.g., "Hello " + "World!"). This means
// we don't want the note in the else clause to be emitted
} else if let [ty] = &visitor.0[..] {
if let ty::Param(p) = *ty.kind() {
// Check if the method would be found if the type param wasn't
// involved. If so, it means that adding a trait bound to the param is
// enough. Otherwise we do not give the suggestion.
let mut eraser = TypeParamEraser(self, expr.span);
let needs_bound = self
.lookup_op_method(
eraser.fold_ty(lhs_ty),
Some(eraser.fold_ty(rhs_ty)),
Some(rhs_expr),
Op::Binary(op, is_assign),
)
.is_ok();
if needs_bound {
suggest_constraining_param(
self.tcx,
self.body_id,
// Look for a TraitPredicate in the Fulfillment errors,
// and use it to generate a suggestion.
//
// Note that lookup_op_method must be called again but
// with a specific rhs_ty instead of a placeholder so
// the resulting predicate generates a more specific
// suggestion for the user.
let errors = self
.lookup_op_method(
lhs_ty,
Some(rhs_ty),
Some(rhs_expr),
Op::Binary(op, is_assign),
)
.unwrap_err();
let predicates = errors
.into_iter()
.filter_map(|error| error.obligation.predicate.to_opt_poly_trait_pred())
.collect::<Vec<_>>();
if !predicates.is_empty() {
for pred in predicates {
self.infcx.suggest_restricting_param_bound(
&mut err,
*ty,
rhs_ty,
missing_trait,
p,
use_output,
pred,
self.body_id,
);
} else if *ty != lhs_ty {
// When we know that a missing bound is responsible, we don't show
// this note as it is redundant.
err.note(&format!(
"the trait `{missing_trait}` is not implemented for `{lhs_ty}`"
));
}
} else {
bug!("type param visitor stored a non type param: {:?}", ty.kind());
} else if *ty != lhs_ty {
// When we know that a missing bound is responsible, we don't show
// this note as it is redundant.
err.note(&format!(
"the trait `{missing_trait}` is not implemented for `{lhs_ty}`"
));
}
}
}
Expand Down Expand Up @@ -671,24 +670,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ex.span,
format!("cannot apply unary operator `{}`", op.as_str()),
);
let missing_trait = match op {
hir::UnOp::Deref => unreachable!("check unary op `-` or `!` only"),
hir::UnOp::Not => "std::ops::Not",
hir::UnOp::Neg => "std::ops::Neg",
};

let mut visitor = TypeParamVisitor(vec![]);
visitor.visit_ty(operand_ty);
if let [ty] = &visitor.0[..] && let ty::Param(p) = *operand_ty.kind() {
suggest_constraining_param(
self.tcx,
self.body_id,
&mut err,
*ty,
operand_ty,
missing_trait,
p,
true,
);
if let [_] = &visitor.0[..] && let ty::Param(_) = *operand_ty.kind() {
let predicates = errors
.iter()
.filter_map(|error| {
error.obligation.predicate.clone().to_opt_poly_trait_pred()
});
for pred in predicates {
self.infcx.suggest_restricting_param_bound(
&mut err,
pred,
self.body_id,
);
}
}

let sp = self.tcx.sess.source_map().start_point(ex.span);
Expand Down Expand Up @@ -973,46 +970,6 @@ fn is_builtin_binop<'tcx>(lhs: Ty<'tcx>, rhs: Ty<'tcx>, op: hir::BinOp) -> bool
}
}

fn suggest_constraining_param(
tcx: TyCtxt<'_>,
body_id: hir::HirId,
mut err: &mut Diagnostic,
lhs_ty: Ty<'_>,
rhs_ty: Ty<'_>,
missing_trait: &str,
p: ty::ParamTy,
set_output: bool,
) {
let hir = tcx.hir();
let msg = &format!("`{lhs_ty}` might need a bound for `{missing_trait}`");
// Try to find the def-id and details for the parameter p. We have only the index,
// so we have to find the enclosing function's def-id, then look through its declared
// generic parameters to get the declaration.
let def_id = hir.body_owner_def_id(hir::BodyId { hir_id: body_id });
let generics = tcx.generics_of(def_id);
let param_def_id = generics.type_param(&p, tcx).def_id;
if let Some(generics) = param_def_id
.as_local()
.map(|id| hir.local_def_id_to_hir_id(id))
.and_then(|id| hir.find_by_def_id(hir.get_parent_item(id)))
.as_ref()
.and_then(|node| node.generics())
{
let output = if set_output { format!("<Output = {rhs_ty}>") } else { String::new() };
suggest_constraining_type_param(
tcx,
generics,
&mut err,
&lhs_ty.to_string(),
&format!("{missing_trait}{output}"),
None,
);
} else {
let span = tcx.def_span(param_def_id);
err.span_label(span, msg);
}
}

struct TypeParamVisitor<'tcx>(Vec<Ty<'tcx>>);

impl<'tcx> TypeVisitor<'tcx> for TypeParamVisitor<'tcx> {
Expand Down
20 changes: 20 additions & 0 deletions src/test/ui/binop/issue-93927.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Regression test for #93927: suggested trait bound for T should be Eq, not PartialEq
struct MyType<T>(T);

impl<T> PartialEq for MyType<T>
where
T: Eq,
{
fn eq(&self, other: &Self) -> bool {
true
}
}

fn cond<T: PartialEq>(val: MyType<T>) -> bool {
val == val
//~^ ERROR binary operation `==` cannot be applied to type `MyType<T>`
}

fn main() {
cond(MyType(0));
}
16 changes: 16 additions & 0 deletions src/test/ui/binop/issue-93927.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0369]: binary operation `==` cannot be applied to type `MyType<T>`
--> $DIR/issue-93927.rs:14:9
|
LL | val == val
| --- ^^ --- MyType<T>
| |
| MyType<T>
|
help: consider further restricting this bound
|
LL | fn cond<T: PartialEq + std::cmp::Eq>(val: MyType<T>) -> bool {
| ++++++++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0369`.
46 changes: 0 additions & 46 deletions src/test/ui/generic-associated-types/missing-bounds.fixed

This file was deleted.

2 changes: 0 additions & 2 deletions src/test/ui/generic-associated-types/missing-bounds.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// run-rustfix

use std::ops::Add;

struct A<B>(B);
Expand Down
20 changes: 10 additions & 10 deletions src/test/ui/generic-associated-types/missing-bounds.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: equality constraints are not yet supported in `where` clauses
--> $DIR/missing-bounds.rs:37:33
--> $DIR/missing-bounds.rs:35:33
|
LL | impl<B: Add> Add for E<B> where <B as Add>::Output = B {
| ^^^^^^^^^^^^^^^^^^^^^^ not supported
Expand All @@ -11,7 +11,7 @@ LL | impl<B: Add> Add for E<B> where B: Add<Output = B> {
| ~~~~~~~~~~~~~~~~~~

error[E0308]: mismatched types
--> $DIR/missing-bounds.rs:11:11
--> $DIR/missing-bounds.rs:9:11
|
LL | impl<B> Add for A<B> where B: Add {
| - this type parameter
Expand All @@ -24,7 +24,7 @@ LL | A(self.0 + rhs.0)
= note: expected type parameter `B`
found associated type `<B as Add>::Output`
note: tuple struct defined here
--> $DIR/missing-bounds.rs:5:8
--> $DIR/missing-bounds.rs:3:8
|
LL | struct A<B>(B);
| ^
Expand All @@ -34,7 +34,7 @@ LL | impl<B> Add for A<B> where B: Add + Add<Output = B> {
| +++++++++++++++++

error[E0308]: mismatched types
--> $DIR/missing-bounds.rs:21:14
--> $DIR/missing-bounds.rs:19:14
|
LL | impl<B: Add> Add for C<B> {
| - this type parameter
Expand All @@ -47,7 +47,7 @@ LL | Self(self.0 + rhs.0)
= note: expected type parameter `B`
found associated type `<B as Add>::Output`
note: tuple struct defined here
--> $DIR/missing-bounds.rs:15:8
--> $DIR/missing-bounds.rs:13:8
|
LL | struct C<B>(B);
| ^
Expand All @@ -57,7 +57,7 @@ LL | impl<B: Add + Add<Output = B>> Add for C<B> {
| +++++++++++++++++

error[E0369]: cannot add `B` to `B`
--> $DIR/missing-bounds.rs:31:21
--> $DIR/missing-bounds.rs:29:21
|
LL | Self(self.0 + rhs.0)
| ------ ^ ----- B
Expand All @@ -66,11 +66,11 @@ LL | Self(self.0 + rhs.0)
|
help: consider restricting type parameter `B`
|
LL | impl<B: std::ops::Add<Output = B>> Add for D<B> {
| +++++++++++++++++++++++++++
LL | impl<B: std::ops::Add> Add for D<B> {
| +++++++++++++++

error[E0308]: mismatched types
--> $DIR/missing-bounds.rs:42:14
--> $DIR/missing-bounds.rs:40:14
|
LL | impl<B: Add> Add for E<B> where <B as Add>::Output = B {
| - this type parameter
Expand All @@ -83,7 +83,7 @@ LL | Self(self.0 + rhs.0)
= note: expected type parameter `B`
found associated type `<B as Add>::Output`
note: tuple struct defined here
--> $DIR/missing-bounds.rs:35:8
--> $DIR/missing-bounds.rs:33:8
|
LL | struct E<B>(B);
| ^
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/issues/issue-35668.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ LL | a.iter().map(|a| a*a)
| |
| &T
|
help: consider restricting type parameter `T`
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
|
LL | fn func<'a, T: std::ops::Mul<Output = &T>>(a: &'a [T]) -> impl Iterator<Item=&'a T> {
| ++++++++++++++++++++++++++++
LL | fn func<'a, T>(a: &'a [T]) -> impl Iterator<Item=&'a T> where &T: Mul<&T> {
| +++++++++++++++++

error: aborting due to previous error

Expand Down
5 changes: 4 additions & 1 deletion src/test/ui/suggestions/invalid-bin-op.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@ note: an implementation of `PartialEq<_>` might be missing for `S<T>`
|
LL | struct S<T>(T);
| ^^^^^^^^^^^^^^^ must implement `PartialEq<_>`
= note: the trait `std::cmp::PartialEq` is not implemented for `S<T>`
help: consider annotating `S<T>` with `#[derive(PartialEq)]`
|
LL | #[derive(PartialEq)]
|
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
|
LL | pub fn foo<T>(s: S<T>, t: S<T>) where S<T>: PartialEq {
| +++++++++++++++++++++

error: aborting due to previous error

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/traits/resolution-in-overloaded-op.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ LL | a * b
| |
| &T
|
help: consider further restricting this bound
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
|
LL | fn foo<T: MyMul<f64, f64> + std::ops::Mul<Output = f64>>(a: &T, b: f64) -> f64 {
| +++++++++++++++++++++++++++++
LL | fn foo<T: MyMul<f64, f64>>(a: &T, b: f64) -> f64 where &T: Mul<f64> {
| ++++++++++++++++++

error: aborting due to previous error

Expand Down
Loading

0 comments on commit d6a57d3

Please sign in to comment.