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 3 commits into
base: master
Choose a base branch
from

Conversation

estebank
Copy link
Contributor

With code like the following

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

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

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

When referenced as follows:

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:

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

suggest deriveing 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;
   |

Given

use std::fmt::Debug;
use std::hash::Hash;
pub trait Ctx: Clone + Eq + Debug {
type Loc: Loc;
}

pub trait Accessible {
    type Context<'a>: Ctx;
    fn can_access<'a>(&self, ctx: &Self::Context<'a>) -> bool;
}

pub trait Id: Copy + Clone + Debug + Eq + Hash + Ord + PartialOrd {}

pub trait Loc: Accessible {
    type LocId: Id;
}

// error
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub enum History<T>
where T: Ctx
{
    Visit(<<T as Ctx>::Loc as Loc>::LocId),
    Action(<<T as Ctx>::Loc as Loc>::LocId),
}

#[derive(Copy, Clone, Default, Eq, PartialEq)]
pub struct HSlice<'a, T>
where T: Ctx {
    slice: &'a [History<T>],
}
impl<'a, T> Hash for HSlice<'a, T> where T: Ctx {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        (*self.slice).hash(state);
    }
}

we emit

error[E0599]: the method `hash` exists for slice `[History<T>]`, but its trait bounds were not satisfied
  --> f1000.rs:34:23
   |
20 | pub enum History<T>
   | ------------------- doesn't satisfy `History<T>: Hash`
...
34 |         (*self.slice).hash(state);
   |                       ^^^^ method cannot be called on `[History<T>]` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `History<T>: Hash`
           which is required by `[History<T>]: Hash`
help: consider further restricting this bound
   |
32 | impl<'a, T> Hash for HSlice<'a, T> where T: Ctx + std::hash::Hash {
   |                                                 +++++++++++++++++

Fix #110446, fix #108712.

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2024

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2024
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_hir_typeck/src/method/suggest.rs Outdated Show resolved Hide resolved
// 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, doing ty::TraitRef::identity(tcx, trait_pred.trait_ref.def_id).with_self_ty(ty); would be the right thing to do here, right?

Copy link
Member

Choose a reason for hiding this comment

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

No, that wouldn't be substituted correctly still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funnily enough, the error for lack of Eq triggers in E0277, so it isn't affected by this check:

error[E0277]: the trait bound `Z: Eq` is not satisfied
  --> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:26:6
   |
LL |     <_ as Eq>::assert_receiver_is_total_eq(&ctx.a_map["a"]);
   |      ^ the trait `Eq` is not implemented for `Z`, which is required by `B<Z>: Eq`
   |
note: required for `B<Z>` to implement `Eq`
  --> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:10:28
   |
LL | #[derive(Clone, PartialEq, Eq)]
   |                            ^^ unsatisfied trait bound introduced in this `derive` macro
   = note: this error originates in the derive macro `Eq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting type parameter `Z`
   |
LL | fn qux<Z: std::cmp::Eq>(ctx: &mut Ctx<Z>) {
   |         ++++++++++++++

error[E0277]: the trait bound `S: Eq` is not satisfied
  --> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:31:6
   |
LL |     <_ as Eq>::assert_receiver_is_total_eq(&ctx.a_map["a"]);
   |      ^ the trait `Eq` is not implemented for `S`, which is required by `B<S>: Eq`
   |
   = help: the trait `Eq` is implemented for `B<A>`
note: required for `B<S>` to implement `Eq`
  --> $DIR/type-or-type-param-missing-transitive-trait-contraint.rs:10:28
   |
LL | #[derive(Clone, PartialEq, Eq)]
   |                            ^^ unsatisfied trait bound introduced in this `derive` macro
   = note: this error originates in the derive macro `Eq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `S` with `#[derive(Eq)]`
   |
LL + #[derive(Eq)]
LL | struct S;
   |

I think that with the specific allow-list of traits then we'll be fine.

@michaelwoerister
Copy link
Member

I'm not a good reviewer for this. r? @compiler-errors, since you're already looking at it -- but please re-assign if you prefer that.

@bors
Copy link
Contributor

bors commented Jul 22, 2024

☔ The latest upstream changes (presumably #128041) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2024
…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;
   |
```
Address review comments and rebase. Add more tests.
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 8, 2024
suggested_derive = self.suggest_derive(
&mut err,
&[(
<_ as ty::UpcastFrom<_, _>>::upcast_from(
Copy link
Member

Choose a reason for hiding this comment

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

Don't use UpcastFrom::upcast_from, just call upcast.

Comment on lines +1423 to +1424
ty::TraitRef::identity(tcx, trait_pred.trait_ref.def_id)
.with_self_ty(tcx, ty);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right :( PartialEq has two type parameters -- Self and Rhs. This only substitutes the first one.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2024
@bors
Copy link
Contributor

bors commented Sep 11, 2024

☔ The latest upstream changes (presumably #130237) made this pull request unmergeable. Please resolve the merge conflicts.

@alex-semenyuk alex-semenyuk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Clone bound suggestion Invalid suggestion already set
7 participants