Detect unmet bound error caused by lack of perfect derives #143993
Conversation
When encountering an unmet bound E0599 error originating in a derived trait impl, provide additional context:
```
error[E0599]: the method `clone` exists for struct `S<X, X, Y>`, but its trait bounds were not satisfied
--> $DIR/lack-of-perfect-derives.rs:27:16
|
LL | struct S<T, K, V>(Arc<T>, Rc<K>, Arc<V>, ());
| ----------------- method `clone` not found for this struct because it doesn't satisfy `S<X, X, Y>: Clone`
...
LL | struct X;
| -------- doesn't satisfy `X: Clone`
...
LL | let s2 = s.clone();
| ^^^^^ method cannot be called on `S<X, X, Y>` due to unsatisfied trait bounds
|
note: trait bound `X: Clone` was not satisfied
--> $DIR/lack-of-perfect-derives.rs:4:10
|
LL | #[derive(Clone, PartialEq)]
| ^^^^^ unsatisfied trait bound introduced in this `derive` macro
LL | struct S<T, K, V>(Arc<T>, Rc<K>, Arc<V>, ());
| - - implicit unsatisfied bound on this type parameter
| |
| implicit unsatisfied bound on this type parameter
= note: `#[derive(Clone)]` introduces `where`-bounds requiring every type parameter to implement `Clone`, even when not strictly necessary
= help: you can manually implement `Clone` with more targeted bounds: `impl<T, K, V> Clone for S<T, K, V> where Arc<T>: Clone, Rc<K>: Clone, Arc<V>: Clone { /* .. */ }`
help: consider annotating `X` with `#[derive(Clone)]`
|
LL + #[derive(Clone)]
LL | struct X;
|
```
We now add a `note` mentioning that `derive` introduces `where`-bounds on the type parameters.
If the derived bounds would be different under "perfect derives", meaning a relaxation of the impl bounds could make the impl apply, then we mention (an approximate of) the impl that can be manually written.
rustbot
added
S-waiting-on-review
| })) if let ( | ||
| ExpnKind::Macro(MacroKind::Derive, mac), _) | ||
| | (_, Some(ExpnKind::Macro(MacroKind::Derive, mac)) | ||
| ) = ( | ||
| self_ty.span.ctxt().outer_expn_data().kind, | ||
| of_trait.as_ref().map(|t| t.path.span.ctxt().outer_expn_data().kind), | ||
| ) => |
There was a problem hiding this comment.
This is done so that we can get the macro name from the Span of either the implemented trait or the type being implemented. I don't think we strictly need it to be like this, but I didn't try alternatives.
| LL | struct Bar<T: Foo> { | ||
| | - implicit unsatisfied bound on this type parameter | ||
| = note: `#[derive(Clone)]` introduces `where`-bounds requiring every type parameter to implement `Clone`, even when not strictly necessary | ||
| = help: you can manually implement `Clone` with more targeted bounds: `impl<T> Clone for Bar<T> where T::X: Clone { /* .. */ }` |
There was a problem hiding this comment.
The correct version of this would be impl<T: Foo> Clone for Bar<T> where T::X: Clone { /* .. */ }, but that is a where bound on the impl. The naïve approach to dealing with predicates in the suggestion code isn't conductive to properly handling this case :-/
| error[E0369]: binary operation `==` cannot be applied to type `S<X, X, X>` | ||
| --> $DIR/lack-of-perfect-derives.rs:23:15 | ||
| | | ||
| LL | let _ = s == s2; | ||
| | - ^^ -- S<X, X, X> | ||
| | | | ||
| | S<X, X, X> | ||
| | | ||
| note: an implementation of `PartialEq` might be missing for `X` | ||
| --> $DIR/lack-of-perfect-derives.rs:15:1 | ||
| | | ||
| LL | struct X; | ||
| | ^^^^^^^^ must implement `PartialEq` | ||
| help: consider annotating `X` with `#[derive(PartialEq)]` | ||
| | | ||
| LL + #[derive(PartialEq)] | ||
| LL | struct X; | ||
| | |
There was a problem hiding this comment.
This added test is unrelated to the codepath being taken here, but E0369 should eventually use part of the same logic that E0599 does, so that it can provide more suggestions than it does today.
| error[E0599]: the method `clone` exists for struct `S<X, X, Y>`, but its trait bounds were not satisfied | ||
| --> $DIR/lack-of-perfect-derives.rs:27:16 | ||
| | | ||
| LL | struct S<T, K, V>(Arc<T>, Rc<K>, Arc<V>, ()); | ||
| | ----------------- method `clone` not found for this struct because it doesn't satisfy `S<X, X, Y>: Clone` | ||
| ... | ||
| LL | struct X; | ||
| | -------- doesn't satisfy `X: Clone` | ||
| ... | ||
| LL | let s2 = s.clone(); | ||
| | ^^^^^ method cannot be called on `S<X, X, Y>` due to unsatisfied trait bounds | ||
| | | ||
| note: trait bound `X: Clone` was not satisfied | ||
| --> $DIR/lack-of-perfect-derives.rs:4:10 | ||
| | | ||
| LL | #[derive(Clone, PartialEq)] | ||
| | ^^^^^ unsatisfied trait bound introduced in this `derive` macro | ||
| LL | struct S<T, K, V>(Arc<T>, Rc<K>, Arc<V>, ()); | ||
| | - - implicit unsatisfied bound on this type parameter | ||
| | | | ||
| | implicit unsatisfied bound on this type parameter | ||
| = note: `#[derive(Clone)]` introduces `where`-bounds requiring every type parameter to implement `Clone`, even when not strictly necessary | ||
| = help: you can manually implement `Clone` with more targeted bounds: `impl<T, K, V> Clone for S<T, K, V> where V: Clone { /* .. */ }` | ||
| help: consider annotating `X` with `#[derive(Clone)]` | ||
| | | ||
| LL + #[derive(Clone)] | ||
| LL | struct X; | ||
| | |
There was a problem hiding this comment.
This is the case that "perfect derives" would fix, and main impetus behind this change. Note that the help still includes where V: Clone, because that obligation was met (because V ends up being Y, which is Clone. We could change this behavior slightly to not include that bound either, but ran out of time to look into that.
I would like to remove some of the output from this error, like the suggestion to make X cloneable, and maybe the secondary span label pointing at S, as that is already pointed at by the "unsatisfied trait bound X: Clone" note.
| note: trait bound `X: PartialEq` was not satisfied | ||
| --> $DIR/lack-of-perfect-derives.rs:4:17 | ||
| | | ||
| LL | #[derive(Clone, PartialEq)] | ||
| | ^^^^^^^^^ unsatisfied trait bound introduced in this `derive` macro | ||
| LL | struct S<T, K, V>(Arc<T>, Rc<K>, Arc<V>, ()); | ||
| | - - - implicit unsatisfied bound on this type parameter | ||
| | | | | ||
| | | implicit unsatisfied bound on this type parameter | ||
| | implicit unsatisfied bound on this type parameter | ||
| = note: `#[derive(PartialEq)]` introduces `where`-bounds requiring every type parameter to implement `PartialEq`, even when not strictly necessary | ||
| = help: you can manually implement `PartialEq` with more targeted bounds: `impl<T, K, V> PartialEq for S<T, K, V> where Arc<T>: PartialEq, Rc<K>: PartialEq, Arc<V>: PartialEq { /* .. */ }` |
There was a problem hiding this comment.
This is the opposite perfect derive case: instead of the impl having no additional bounds, the bounds added are all not directly on the type parameter but rather on the wrapper types.
| = note: the following trait bounds were not satisfied: | ||
| `S<X, X, X>: Iterator` | ||
| which is required by `&mut S<X, X, X>: Iterator` | ||
| note: the trait `Iterator` must be implemented | ||
| --> $SRC_DIR/core/src/iter/traits/iterator.rs:LL:COL |
There was a problem hiding this comment.
Pre-existing issue. We shouldn't be talking about Iterator at all for this case.
| note: trait bound `X: Clone` was not satisfied | ||
| --> $DIR/lack-of-perfect-derives.rs:8:10 | ||
| | | ||
| LL | #[derive(Clone, PartialEq)] | ||
| | ^^^^^ unsatisfied trait bound introduced in this `derive` macro | ||
| LL | struct N<T, K, V>(Arc<T>, T, Rc<K>, K, Arc<V>, V, ()); | ||
| | - - implicit unsatisfied bound on this type parameter | ||
| | | | ||
| | implicit unsatisfied bound on this type parameter | ||
| = note: `#[derive(Clone)]` introduces `where`-bounds requiring every type parameter to implement `Clone` |
There was a problem hiding this comment.
Note the mixed use of Arc<T> and T. We don't suggest where Arc<T>: Clone, T: Clone, so in the end we suggest no impl.
| @@ -1752,6 +1762,226 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| err.emit() | |||
| } | |||
|
|
|||
| fn extract_derive_bounds( | |||
There was a problem hiding this comment.
The new suggestion logic starting here isn't... the cleanest and would love it if anyone comes up with a neater alternative, but this is what I got to.
| bounds | ||
| .entry(self_ty.to_string()) | ||
| .or_default() | ||
| .insert("placeholder".to_string()); |
There was a problem hiding this comment.
"placeholder" is so that we keep the space of types that can implement the trait without obligating the original type parameter to also implement the trait (like for Arc and unlike PartialEq).
| @@ -29,6 +29,9 @@ note: trait bound `CloneNoCopy: Copy` was not satisfied | |||
| | | |||
| LL | #[derive(Clone, Copy)] | |||
| | ^^^^^ unsatisfied trait bound introduced in this `derive` macro | |||
| LL | union U5<T> { | |||
| | - implicit unsatisfied bound on this type parameter | |||
| = note: `#[derive(Clone)]` introduces `where`-bounds requiring every type parameter to implement `Clone` | |||
There was a problem hiding this comment.
This error message looks misleading. The T: Clone bound of #[derive(Clone)] is required for Clone and the problem is the U5<CloneNoCopy>: Copy, not U5<CloneNoCopy>: Clone
There was a problem hiding this comment.
I've been putting off reviewing this as the existing function/the changes seem really involved and I expected reviewing it to be a lot of effort. I think I should have still spent 15 min on this the previous time I've went through my assigned PRs.
I didn't review it in depth yet, some initial questions/thoughts
Why do we report this for method errors instead of general trait errors? This is also frequently an issue for just normal trait bounds.
Can you do the type system check on the lowered field types/why do you use hir::Ty?
I think the algorithm we want is:
- if we've got a trait bound which doesn't hold where the self type is an ADT, look at all impls for the self type
- check whether one exists has the
#[automatically_derived]attribute - now check whether the impl differs from a perfect derive*
- if so, emit a suggestion to manually impl it
The "check whether impl differs from perfect derive" is hard to do properly. It's similar to auto_traits.rs for rustdoc. We intend to rewrite this using the new solver, but for now it's probably easiest to approximate this check somehow.
I feel like what gets us most of the way there is: given the where-bounds of the ADT definition + bounds for the derived trait on all params expect one, does its field already implement the derived trait? If so, remove this bound. Try all bounds in a loop, minimizing the required bounds.
This doesn't catch cases where the impl has different where-bounds from the derive, but in that case I'd assume the derive itself just errors 🤔 i.e. the only cases where we need to help users is if derives add unnecessary constraints. If the derive adds different constraints, the derive itself will error.
lcnr
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #144740) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot
removed
the
S-waiting-on-author
When encountering an unmet bound E0599 error originating in a derived trait impl, provide additional context:
We now add a
notementioning thatderiveintroduceswhere-bounds on the type parameters.If the derived bounds would be different under "perfect derives", meaning a relaxation of the impl bounds could make the impl apply, then we mention (an approximate of) the impl that can be manually written.
Fix #143714.