-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @michaelwoerister. Use |
This comment has been minimized.
This comment has been minimized.
73ca03d
to
350d3e6
Compare
// 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]); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
☔ The latest upstream changes (presumably #128041) made this pull request unmergeable. Please resolve the merge conflicts. |
…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.
350d3e6
to
7bf75ab
Compare
suggested_derive = self.suggest_derive( | ||
&mut err, | ||
&[( | ||
<_ as ty::UpcastFrom<_, _>>::upcast_from( |
There was a problem hiding this comment.
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
.
ty::TraitRef::identity(tcx, trait_pred.trait_ref.def_id) | ||
.with_self_ty(tcx, ty); |
There was a problem hiding this comment.
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.
☔ The latest upstream changes (presumably #130237) made this pull request unmergeable. Please resolve the merge conflicts. |
With code like the following
the derived trait will have an implicit restriction on
A: Clone
for both types.When referenced as follows:
suggest constraining
Z
:When referenced as follows, with a specific type
S
:suggest
derive
ing the appropriate trait on the local type:Given
we emit
Fix #110446, fix #108712.