-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Provide more context on trait bounds being unmet due to imperfect derive #151278
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
base: main
Are you sure you want to change the base?
Conversation
When encountering a value that has a borrow checker error where the type was previously moved, when suggesting cloning verify that it is not already being derived. If it is, explain why the `derive(Clone)` doesn't apply:
```
note: if `TypedAddress<T>` implemented `Clone`, you could clone the value
--> $DIR/derive-clone-implicit-bound.rs:6:1
|
LL | #[derive(Clone, Copy)]
| ----- derived `Clone` adds implicit bounds on type parameters
LL | pub struct TypedAddress<T>{
| ^^^^^^^^^^^^^^^^^^^^^^^^-^
| | |
| | introduces an implicit `T: Clone` bound
| consider manually implementing `Clone` for this type
...
LL | let old = self.return_value(offset);
| ------ you could clone this value
```
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
023f134 to
167ae23
Compare
This comment has been minimized.
This comment has been minimized.
When encountering a bound coming from a derive macro, suggest manual impl of the trait.
Use the span for the specific param when adding bounds in builtin derive macros, so the diagnostic will point at them as well as the derive macro itself.
```
error[E0277]: can't compare `SomeNode` with `SomeNode`
--> f29.rs:24:15
|
24 | accept_eq(&node);
| --------- ^^^^^ no implementation for `SomeNode == SomeNode`
| |
| required by a bound introduced by this call
|
= note: -Ztrack-diagnostics: created at compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs:279:39
= help: the trait `PartialEq` is not implemented for `SomeNode`
note: required for `Id<SomeNode>` to implement `PartialEq`
--> f29.rs:3:10
|
3 | #[derive(PartialEq, Eq)]
| ^^^^^^^^^ unsatisfied trait bound introduced in this `derive` macro
4 | pub struct Id<T>(PhantomData<T>);
| -
= help: consider manually implementing `PartialEq` to avoid undesired bounds
note: required by a bound in `accept_eq`
--> f29.rs:15:23
|
15 | fn accept_eq(_: &impl PartialEq) { }
| ^^^^^^^^^ required by this bound in `accept_eq`
help: consider annotating `SomeNode` with `#[derive(PartialEq)]`
|
13 + #[derive(PartialEq)]
14 | struct SomeNode();
|
```
167ae23 to
dcc907d
Compare
```
note: required for `B<C>` to implement `Copy`
--> $DIR/deriving-copyclone.rs:9:10
|
LL | #[derive(Copy, Clone)]
| ^^^^ unsatisfied trait bound introduced in this `derive` macro
LL | struct B<T> {
| - would need to be `Copy`
```
|
CC another related issue: #146515. This one won't be fixed by your PR IINM but we might be able to leverage & reuse some of your added logic to do so (disclaimer: I've only skimmed your PR, so I might be wrong). There's a PR open to fix said issue, namely #150720, but your approach might be more general and could supersede it. |
|
@fmease for that case, what do you think of output like |
… for derive
On type errors where the difference is expecting an owned type and getting a reference, if the expression is a `.clone()` call and the type is annotated with `#[derive(Clone)]`, we now explain implicit bounds and suggest manually implementing `Clone`.
```
error[E0308]: mismatched types
--> $DIR/derive-implicit-bound-on-clone.rs:10:5
|
LL | fn clone_me<T, K>(x: &ContainsRc<T, K>) -> ContainsRc<T, K> {
| ---------------- expected `ContainsRc<T, K>` because of return type
LL | x.clone()
| ^^^^^^^^^ expected `ContainsRc<T, K>`, found `&ContainsRc<T, K>`
|
= note: expected struct `ContainsRc<_, _>`
found reference `&ContainsRc<_, _>`
note: `ContainsRc<T, K>` does not implement `Clone`, so `&ContainsRc<T, K>` was cloned instead
--> $DIR/derive-implicit-bound-on-clone.rs:10:5
|
LL | x.clone()
| ^
help: `Clone` is not implemented because the some trait bounds could not be satisfied
--> $DIR/derive-implicit-bound-on-clone.rs:5:19
|
LL | #[derive(Clone)]
| ----- in this derive macro expansion
LL | struct ContainsRc<T, K> {
| ^ ^ derive introduces an implicit unsatisfied trait bound `K: Clone`
| |
| derive introduces an implicit unsatisfied trait bound `T: Clone`
= help: consider manually implementing `Clone` to avoid the implict type parameter bounds
```
This comment has been minimized.
This comment has been minimized.
|
David: I can split the PR into smaller ones as needed (it's just they touch the same stderr files so it was easier to keep it as a single patchset). Each commit is logically grouped. |
This comment has been minimized.
This comment has been minimized.
|
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Implementation largely looks good to me, some thoughts on the new suggestions more generally
| | ^^^^^^^^^^^^^^^^^^^^^^^^-^ | ||
| | | | | ||
| | | introduces an implicit `T: Clone` bound | ||
| | consider manually implementing `Clone` for this type |
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.
"manually implementing Clone for this type" is an unexpected suggestion for me, I'd assume we'd instead say "consider adding a Clone bound for the T you instantiate TypedAddress with", rather than a manual impl, which could work for this test case, but probably not in general?
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.
I think both solutions are equally likely a-priori, but somewhat surprisingly the people I've seen getting confused the most predominantly leaned towards the former. I guess that imperfect derives only makes people reach for help when imperfect derives get in the way of what they were trying to accomplish. We should likely suggest (or mention) both.
Edit: we are already suggesting both, with the constraining of the type param being machine applicable already :)
| ... | ||
| LL | let old = self.return_value(offset); | ||
| | ------ you could clone this value | ||
| help: consider further restricting type parameter `T` with trait `Copy` |
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.
Pre-existing, but that this is a "help" while the other two alternatives are "note" seems inconsistent
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.
I think I've addressed this now.
| data.span, | ||
| "unsatisfied trait bound introduced in this `derive` macro", | ||
| if data.span.in_derive_expansion() { | ||
| format!("would need to be `{trait_name}`") |
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.
"would need to implement" maybe?
Do we normally start these messages mid-sentence like this? It reads a little funnily to me. I'm used to "this parameter" or something, which acknowledges that the message is alongside something being highlighted, but not with an implicit "this thing"
| | ---- in this derive macro expansion | ||
| LL | struct Foo<T>(T); | ||
| | ^^^ - type parameter would need to implement `Copy` | ||
| = help: consider manually implementing `Copy` to avoid undesired bounds |
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 one suggestion is nonsensical (can't manually impl Copy).
When encountering a value that has a borrow checker error where the type was previously moved, when suggesting cloning verify that it is not already being derived. If it is, explain why the
derive(Clone)doesn't apply:When encountering a bound coming from a derive macro, suggest manual impl of the trait.
Use the span for the specific param when adding bounds in builtin derive macros, so the diagnostic will point at them as well as the derive macro itself.
Mention that the trait could be manually implemented in E0599.
Fix #108894. Address #143714. Address ##146515 (but ideally would also suggest constraining the fn bound correctly as well).