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 derive(Trait) or T: Trait from transitive obligation in some cases #127997

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
Suggest derive(Trait) or T: Trait from transitive obligation in s…
…ome cases

With code like the following

```rust
struct Ctx<A> {
    a_map: HashMap<String, B<A>>,
}

struct B<A> {
    a: A,
}
```

the derived trait will have an implicit restriction on `A: Clone` for both types.

When referenced as follows:

```rust
fn foo<Z>(ctx: &mut Ctx<Z>) {
    let a_map = ctx.a_map.clone(); //~ ERROR E0599
}
```

suggest constraining `Z`:

```
error[E0599]: the method `clone` exists for struct `HashMap<String, B<Z>>`, but its trait bounds were not satisfied
  --> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:16:27
   |
LL | struct B<A> {
   | ----------- doesn't satisfy `B<Z>: Clone`
...
LL |     let a_map = ctx.a_map.clone();
   |                           ^^^^^ method cannot be called on `HashMap<String, B<Z>>` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `B<Z>: Clone`
           which is required by `HashMap<String, B<Z>>: Clone`
help: consider restricting type parameter `Z`
   |
LL | fn foo<Z: std::clone::Clone>(ctx: &mut Ctx<Z>) {
   |         +++++++++++++++++++
```

When referenced as follows, with a specific type `S`:

```rust
struct S;
fn bar(ctx: &mut Ctx<S>) {
    let a_map = ctx.a_map.clone(); //~ ERROR E0599
}
```

suggest `derive`ing the appropriate trait on the local type:

```
error[E0599]: the method `clone` exists for struct `HashMap<String, B<S>>`, but its trait bounds were not satisfied
  --> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:21:27
   |
LL | struct B<A> {
   | ----------- doesn't satisfy `B<S>: Clone`
...
LL |     let a_map = ctx.a_map.clone();
   |                           ^^^^^ method cannot be called on `HashMap<String, B<S>>` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `B<S>: Clone`
           which is required by `HashMap<String, B<S>>: Clone`
help: consider annotating `S` with `#[derive(Clone)]`
   |
LL + #[derive(Clone)]
LL | struct S;
   |
```
  • Loading branch information
estebank committed Jul 20, 2024
commit 350d3e684a5971192a61cfd991f414272bcf8d35
96 changes: 95 additions & 1 deletion compiler/rustc_hir_typeck/src/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use rustc_span::{edit_distance, ErrorGuaranteed, ExpnKind, FileName, MacroKind,
use rustc_span::{Symbol, DUMMY_SP};
use rustc_trait_selection::error_reporting::traits::on_unimplemented::OnUnimplementedNote;
use rustc_trait_selection::error_reporting::traits::on_unimplemented::TypeErrCtxtExt as _;
use rustc_trait_selection::error_reporting::traits::suggestions::TypeErrCtxtExt;
use rustc_trait_selection::infer::InferCtxtExt;
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _;
use rustc_trait_selection::traits::{
Expand Down Expand Up @@ -1368,7 +1369,100 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
"the following trait bounds were not satisfied:\n{bound_list}"
));
}
suggested_derive = self.suggest_derive(&mut err, unsatisfied_predicates);

let mut suggest_derive = true;
for (pred, _, cause) in unsatisfied_predicates {
let Some(ty::PredicateKind::Clause(ty::ClauseKind::Trait(trait_pred))) =
pred.kind().no_bound_vars()
else {
continue;
};
let (adt, params) = match trait_pred.self_ty().kind() {
ty::Adt(adt, params) if adt.did().is_local() => (*adt, params),
_ => continue,
};
if self
.tcx
.all_impls(trait_pred.def_id())
.filter_map(|imp_did| {
self.tcx.impl_trait_header(imp_did).map(|h| (imp_did, h))
})
.filter(|(did, header)| {
let imp = header.trait_ref.instantiate_identity();
let impl_adt = match imp.self_ty().ty_adt_def() {
Some(impl_adt) if adt.did().is_local() => impl_adt,
_ => return false,
};
header.polarity == ty::ImplPolarity::Positive
&& impl_adt == adt
&& self.tcx.is_automatically_derived(*did)
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 check is sufficient -- I can slap automatically_derived onto any impl I want without any problems. There is no guarantee that this is actually a derived impl in a way that makes this logic sufficient.

})
.count()
== 1
{
// We now know that for this predicate, there *was* a `derive(Trait)` for
// the trait at hand, so we don't want to suggest writing that again.
for param in &params[..] {
// Look at the type parameters for the currently obligated type to see
// if a restriciton of `TypeParam: Trait` would help. If the
// instantiated type param is not a type param but instead an actual
// type, see if we can suggest `derive(Trait)` on *that* type.
// See `tests/ui/suggestions/f1000.rs`
let Some(ty) = param.as_type() else {
continue;
};
match ty.kind() {
ty::Adt(adt, _) if adt.did().is_local() => {
// The type param at hand is a local type, try to suggest
// `derive(Trait)`.
let trait_ref =
ty::TraitRef::new(tcx, trait_pred.trait_ref.def_id, [ty]);
Copy link
Member

@compiler-errors compiler-errors Jul 20, 2024

Choose a reason for hiding this comment

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

Regarding that last comment, for example, there is no guarantee that I can create a trait ref out of just this constituent type just by knowing it's an automatically derived trait.

This will almost certainly ICE if you slap automatically_derived onto some impl for a trait that has more parameters than just Self. Also, actually, it may actually just ICE for traits that have >1 type parameter like PartialEq that are automatically derived.

Copy link
Member

Choose a reason for hiding this comment

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

That being said, I don't think that the right solution is to just slap a check for the number of params the trait has onto this logic, since that introduces seemingly arbitrary inconsistencies in how we issue this suggestion (e.g. only for Eq but not PartialEq)

let trait_pred = ty::Binder::dummy(ty::TraitPredicate {
trait_ref,
polarity: ty::PredicatePolarity::Positive,
});
suggested_derive = self.suggest_derive(
&mut err,
&[(
<_ as ty::UpcastFrom<_, _>>::upcast_from(
trait_pred, self.tcx,
),
None,
cause.clone(),
)],
);
}
ty::Param(_) => {
// It was a type param. See if it corresponds to the current
// `fn` and suggest `T: Trait`.
if let Some(obligation) = cause {
let trait_ref = ty::TraitRef::new(
tcx,
trait_pred.trait_ref.def_id,
[ty],
);
let trait_pred = ty::Binder::dummy(ty::TraitPredicate {
trait_ref,
polarity: ty::PredicatePolarity::Positive,
});
suggested_derive =
self.err_ctxt().suggest_restricting_param_bound(
&mut err,
trait_pred,
None,
obligation.body_id,
);
}
}
_ => {}
}
}
suggest_derive = false
}
}
if suggest_derive {
suggested_derive = self.suggest_derive(&mut err, unsatisfied_predicates);
}

unsatisfied_bounds = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,9 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
trait_pred: ty::PolyTraitPredicate<'tcx>,
associated_ty: Option<(&'static str, Ty<'tcx>)>,
mut body_id: LocalDefId,
) {
) -> bool {
if trait_pred.skip_binder().polarity != ty::PredicatePolarity::Positive {
return;
return false;
}

let trait_pred = self.resolve_numeric_literals_with_default(trait_pred);
Expand Down Expand Up @@ -286,7 +286,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
trait_pred,
Some((ident, bounds)),
);
return;
return true;
}

hir::Node::TraitItem(hir::TraitItem {
Expand All @@ -300,7 +300,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
self.tcx, body_id, generics, "`Self`", err, None, projection, trait_pred,
None,
);
return;
return true;
}

hir::Node::TraitItem(hir::TraitItem {
Expand Down Expand Up @@ -328,7 +328,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
trait_pred,
None,
);
return;
return true;
}
hir::Node::Item(hir::Item {
kind:
Expand All @@ -348,7 +348,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
trait_pred,
None,
);
return;
return true;
}

hir::Node::Item(hir::Item {
Expand Down Expand Up @@ -381,7 +381,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
.iter()
.all(|g| g.is_suggestable(self.tcx, false))
{
return;
return false;
}
// Missing generic type parameter bound.
let param_name = self_ty.to_string();
Expand Down Expand Up @@ -413,7 +413,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
Some(trait_pred.def_id()),
None,
) {
return;
return true;
}
}

Expand All @@ -439,10 +439,10 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
trait_pred,
associated_ty,
) {
return;
return true;
}
}
hir::Node::Crate(..) => return,
hir::Node::Crate(..) => return false,

_ => {}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//@ run-rustfix
#![allow(warnings)]
use std::collections::HashMap;

#[derive(Clone)]
struct Ctx<A> {
a_map: HashMap<String, B<A>>,
}

#[derive(Clone)]
struct B<A> {
a: A,
}

fn foo<Z: std::clone::Clone>(ctx: &mut Ctx<Z>) {
let a_map = ctx.a_map.clone(); //~ ERROR E0599
}

#[derive(Clone)]
struct S;
fn bar(ctx: &mut Ctx<S>) {
let a_map = ctx.a_map.clone(); //~ ERROR E0599
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//@ run-rustfix
#![allow(warnings)]
use std::collections::HashMap;

#[derive(Clone)]
struct Ctx<A> {
a_map: HashMap<String, B<A>>,
}

#[derive(Clone)]
struct B<A> {
a: A,
}

fn foo<Z>(ctx: &mut Ctx<Z>) {
let a_map = ctx.a_map.clone(); //~ ERROR E0599
}

struct S;
fn bar(ctx: &mut Ctx<S>) {
let a_map = ctx.a_map.clone(); //~ ERROR E0599
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
error[E0599]: the method `clone` exists for struct `HashMap<String, B<Z>>`, but its trait bounds were not satisfied
--> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:16:27
|
LL | struct B<A> {
| ----------- doesn't satisfy `B<Z>: Clone`
...
LL | let a_map = ctx.a_map.clone();
| ^^^^^ method cannot be called on `HashMap<String, B<Z>>` due to unsatisfied trait bounds
|
= note: the following trait bounds were not satisfied:
`B<Z>: Clone`
which is required by `HashMap<String, B<Z>>: Clone`
help: consider restricting type parameter `Z`
|
LL | fn foo<Z: std::clone::Clone>(ctx: &mut Ctx<Z>) {
| +++++++++++++++++++

error[E0599]: the method `clone` exists for struct `HashMap<String, B<S>>`, but its trait bounds were not satisfied
--> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:21:27
|
LL | struct B<A> {
| ----------- doesn't satisfy `B<S>: Clone`
...
LL | let a_map = ctx.a_map.clone();
| ^^^^^ method cannot be called on `HashMap<String, B<S>>` due to unsatisfied trait bounds
|
= note: the following trait bounds were not satisfied:
`B<S>: Clone`
which is required by `HashMap<String, B<S>>: Clone`
help: consider annotating `S` with `#[derive(Clone)]`
|
LL + #[derive(Clone)]
LL | struct S;
|

error: aborting due to 2 previous errors

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