Improve E0277 for imperfect derives

[raushan728]

View all comments

Relates to #157117

[rustbot]

r? @khyperia

rustbot has assigned @khyperia.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 22 candidates

[mejrs]

I don't think we should be dropping this "perfect derive" prose on every trait bound error. Certainly not for the linked example.

Some examples where it would be appropriate are:

#[derive(Debug)]
struct S<T>{
    spooky: PhantomData<T>,
}

or

#[derive(Clone)]
struct S<T>{
   stufff: Arc<T>,
}

View changes since this review

[mejrs]

I'm not sure about recommending all these crates. If I'm honest it makes me a bit uncomfortable.

[raushan728]

I agree that avoiding specific crate endorsements aligns better with general rustc diagnostic practices. I'll remove them once we confirm the final wording with @estebank.

[estebank]

Not mentioning specific crates might be best... But there's precedent in mentioning some (async-trait comes to mind).

[mejrs]

Same for these links. I'd lean towards just briefly explaining why there's a T: Trait bound introduced by the derive, name drop "perfect derive" and let the user google it for themselves.

[raushan728]

Briefly explaining the bound and name dropping "perfect derive" for users to search sounds like a much cleaner approach. I'll update the diagnostic to do this once we have consensus on the scope.

[estebank]

At the very least, lets link to the issue (the ideal situation would be for the dev guide or some other "official" page to have literally the same info as niko's blog).

Edit: I do feel comfortable with linking to niko's blog, at least for now, though. He himself might not :)

[mejrs]

"perfect derive" is like "turbofish" in that it's very google-able, so there is limited value in providing a link to one particular resource. And while Niko's blog is very well written and fun to read it doesn't actually help with making your code compile.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[raushan728]

I don't think we should be dropping this "perfect derive" prose on every trait bound error. Certainly not for the linked example.

Some examples where it would be appropriate are:

#[derive(Debug)]
struct S<T>{
    spooky: PhantomData<T>,
}

or

#[derive(Clone)]
struct S<T>{
   stufff: Arc<T>,
}

View changes since this review

I understand ur concerns about restricting this proce to actual imperfect cases, and avoiding specific crate endorsement or external URLs in the compiler output.

I implemented it this way beacouse I strictly followed the S<T>(T) example and the exact desired output in that issue. Since there is a design difference here regarding filtering logic and diagnostic, I'd love to both of ur thoughts to reach a consensus before I update. @estebank

[raushan728]

@rustbot ready

[khyperia]

thanks for looking into this! ✨

(just chiming in as the randomly-assigned reviewer, mostly to acknowledge that I've seen this - someone like estebank might be a more appropriate reviewer) - I agree with @mejrs that it would be unfortunate to talk about the difference between perfect and imperfect derives in error messages in situations where there is no difference between the two (e.g. struct S<T>(T)). Happy to wait for estebank to chime in though!

[mejrs]

I understand ur concerns about restricting this proce to actual imperfect cases

@estebank I'd like to build some consensus on this. I would only like to see this diagnostic if "perfect derive" would actually solve their problem. In impl terms, we would only mention it if for each field existed an impl regardless of T, like impl <T> Clone for Arc<T> or impl <T> Debug for Phantomdata<T>

[khyperia]

In impl terms, we would only mention it if for each field existed an impl regardless of T

I'm guessing folks are already aware, but just saying it out loud as an fyi: it's not just blanket impls regardless of T, associated types also are relevant here:

trait Trait {
    type Assoc: Clone;
}

#[derive(Clone)]
struct S<T: Trait> {
    field: T::Assoc,
}

struct NonClone;

impl Trait for NonClone {
    type Assoc = u32;
}

fn main() {
    let x: S<NonClone> = S { field: 2 };
    let _y = x.clone();
    //~^ ERROR the method `clone` exists for struct `S<NonClone>`, but its trait bounds were not satisfied
}

// however, manually writing it works fine:
// impl<T: Trait> Clone for S<T> {
//     fn clone(&self) -> Self {
//         Self {
//             field: self.field.clone(),
//         }
//     }
// }

edit: I suppose you could view this as "an impl existing regardless of T", the field of type T::Assoc does indeed have an impl of Clone regardless of T (via the bounds of T::Assoc: Clone)

[raushan728]

I thing requirement is clear: we should only emit this prose if all fields of the struct actually implement the trait independent of T itself. Implementing this filtering logic inside diagnostic reporting would require Inspecting the struct definition, iterating over its fields, and evaluating trait obligation for each field's type without strictly binding T.

@estebank, given that this expands the scope to more involved traint evaluation check, do u agree with this ? if yes, should i move ahead?

[khyperia]

apologies, I've fallen ill and probably won't get around to reviewing this for a while, so I'm just assigning this to estebank

r? estebank

[estebank]

Apologies for the delay.

Let's change the wording slightly and not mention explicit crates, nor niko's blogpost (but keep the perfect derive info and tracking issue).

given that this expands the scope to more involved traint evaluation check, do u agree with this ? if yes, should i move ahead?

Only if you feel up to it, experience and time wise. Bruno's concerns around showing the wording only when strictly relevant are well founded, we don't want to lead people astray. On the other hand, I think we could land the wording changes only (we already tell people to manually implement the trait from the bound without really checking if that would solve the problem)...

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[estebank]

@bors r+

Let's keep the ticket open to address the more accurate targeting, but in the meantime lets merge the wording changes.

[rust-bors]

📌 Commit c02f727 has been approved by estebank

It is now in the queue for this repository.

[raushan728]

Let's keep the ticket open to address the more accurate targeting, but in the meantime lets merge the wording changes.

I updated it to Relates to since Addresses makes it seem like all the work is done, which isn't the case yet.🙂

[rust-bors]

⌛ Testing commit c02f727 with merge 84d065e...

Workflow: https://github.com/rust-lang/rust/actions/runs/29442115239

[JonathanBrouwer]

@bors yield
Yielding to enclosing rollup

[rust-bors]

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/29442115239

The next pull request likely to be tested is #159350.

[rust-bors]

⌛ Testing commit c02f727 with merge c827f63...

Workflow: https://github.com/rust-lang/rust/actions/runs/29442932447

[JonathanBrouwer]

@bors yield
Yielding to enclosing rollup

[rust-bors]

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/29442932447

The next pull request likely to be tested is #159350.