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

Better error reporting for T: ?Sized types when impl Receiver for MyType<T> is implicitly sized #134390

Open
compiler-errors opened this issue Dec 16, 2024 · 7 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-discussion Category: Discussion or questions that doesn't represent real issues. D-confusing Diagnostics: Confusing error or lint that should be reworked. F-arbitrary_self_types `#![feature(arbitrary_self_types)]`

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Dec 16, 2024

Originally from @adetaylor:


I'm looking for advice on the diagnostics around the Sizedness of a receiver, and I'm hoping @estebank can advise (or of course anyone else @wesleywiser @compiler-errors ).

The background (for Esteban):

  • This is the tracking issue for "arbitrary self types v2", which allows methods like this:
    impl Foo {
      fn bar(self: MySmartPtr<Self>) {} // note type of self
    }
  • The key part of this is that types (such as MySmartPtr, above) which wish to act as method receivers must implement a new trait Receiver
  • Before writing the RFC, I happened to make a mistake which I think might be quite common, so in the Diagnostics section of the RFC I proposed adding a diagnostic specifically for this case (the second bullet in that linked section).

The case I want to generate an error for: see this code but, in short, someone implements Receiver for T but not T: ?Sized.

Questions. Even partial answers to some questions might point me in the right direction.

Overall approach

  1. I can quite easily get hold of an unmet obligation for why the type of self doesn't implement Receiver. But how can I determine if some missing ?Sized bound is the problem?
    a. Re-run the resolution process with some simulated fake sized Self type? See if the obligation resolves in that case, and if so, show a diagnostic.
    b. Simulate that some impl<T> Receiver for T block is actually impl <T: ?Sized> Receiver for T, elsewhere in the program or even in another crate. See if the obligation resolves in that case, and if so, show a diagnostic.
    c. Suggest "ensure any Receiver implementations cover !Sized" without actually checking that this is the problem. This might give lots of false positives.

@adetaylor: I've split this out into its own issue. Let's try to avoid discussion on tracking issues. It's really not the purpose of a tracking issue, and we've locked tracking issues in the past for exactly the same reason (it often pings like... 40 people who are subscribed to the issue).

These days tracking issues carry the note:

As with all tracking issues for the language, please file anything unrelated to implementation history, that is: bugs and design questions, as separate issues as opposed to leaving comments here. The status of the feature should also be covered by the feature gate label. Please do not ask about developments here.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 16, 2024
@compiler-errors compiler-errors added A-diagnostics Area: Messages for errors, warnings, and lints F-arbitrary_self_types `#![feature(arbitrary_self_types)]` D-confusing Diagnostics: Confusing error or lint that should be reworked. C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 16, 2024
@compiler-errors
Copy link
Member Author

I can quite easily get hold of an unmet obligation for why the type of self doesn't implement Receiver. But how can I determine if some missing ?Sized bound is the problem?

Well, if you put that unmet obligation into an ObligationCtxt::new_with_diagnostics, then you should get a FulfillmentError out whose predicate will be a Sized goal.

@adetaylor
Copy link
Contributor

Well, if you put that unmet obligation into an ObligationCtxt::new_with_diagnostics, then you should get a FulfillmentError out whose predicate will be a Sized goal.

Unfortunately it doesn't seem to have figured out its way through to the Sized problem. I get one of these:

FulfillmentError(Obligation(predicate=Binder { value: TraitPredicate(<SmartPtr<'_, Self> as std::ops::Receiver>, polarity:Positive), bound_vars: [] }, depth=0),Unimplemented)

for this test case.

I'll poke at why that's the case.

@cramertj cramertj self-assigned this Dec 17, 2024
@compiler-errors
Copy link
Member Author

On second though, you probably won't have the compiler choose the a specific Receiver impl since there's also a blanket impl for Deref types.

@cramertj
Copy link
Member

cramertj commented Jan 3, 2025

On second though, you probably won't have the compiler choose the a specific Receiver impl since there's also a blanket impl for Deref types.

This does actually appear to be the real issue here. The error message still doesn't provide the hint even when it doesn't originate from an autoderef:

#![feature(arbitrary_self_types)]

struct SmartPtr<'a, T: ?Sized>(&'a T);

impl<T> std::ops::Receiver for SmartPtr<'_, T> {
    type Target = T;
}

fn takes_receiver(_: impl std::ops::Receiver) {}

fn main() {
    let x: SmartPtr<str> = SmartPtr("fooey");
    takes_receiver(x)
}

gives

error[E0277]: the trait bound `SmartPtr<'_, str>: std::ops::Receiver` is not satisfied
  --> src/main.rs:13:20
   |
13 |     takes_receiver(x)
   |     -------------- ^ the trait `std::ops::Receiver` is not implemented for `SmartPtr<'_, str>`
   |     |
   |     required by a bound introduced by this call
   |
note: required by a bound in `takes_receiver`
  --> src/main.rs:9:27
   |
9  | fn takes_receiver(_: impl std::ops::Receiver) {}
   |                           ^^^^^^^^^^^^^^^^^^ required by this bound in `takes_receiver`
help: consider borrowing here
   |
13 |     takes_receiver(&x)
   |                    +
13 |     takes_receiver(&mut x)
   |                    ++++

For more information about this error, try `rustc --explain E0277`.

Note the lack of a Sized hint.

@compiler-errors
Copy link
Member Author

Yeah, unfortunately to the compiler it's not obviously clear that we should prefer a more specific impl over a blanket. For the trait solver, if neither impl applies they're basically just... equally invalid. For diagnostics, it seems like a special case that's motivated for this example but could also be misleading in other places, especially when the blanket impls and concrete impls get more complex and "close" to overlapping.

@cramertj
Copy link
Member

cramertj commented Jan 3, 2025

In that case, I think this issue generalizes to examples like this which have nothing to do with Receiver (or with Sized):

trait MyTrait {}
trait BaseTraitForBlanketImpl {}

struct Wrapper<T>(T);

impl<T: MyTrait> MyTrait for Wrapper<T> {}
impl<T: BaseTraitForBlanketImpl> MyTrait for T {}

fn take_mytrait(_: impl MyTrait) {}

fn main() {
    take_mytrait(Wrapper(5));
}

generates

error[E0277]: the trait bound `Wrapper<{integer}>: MyTrait` is not satisfied
  --> src/main.rs:12:18
   |
12 |     take_mytrait(Wrapper(5));
   |     ------------ ^^^^^^^^^^ the trait `MyTrait` is not implemented for `Wrapper<{integer}>`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the trait `MyTrait` is implemented for `Wrapper<T>`
note: required by a bound in `take_mytrait`
  --> src/main.rs:9:25
   |
9  | fn take_mytrait(_: impl MyTrait) {}
   |                         ^^^^^^^ required by this bound in `take_mytrait`

For more information about this error, try `rustc --explain E0277`.

but with the non-applicable blanket impl removed, it gives a more specific error:

error[E0277]: the trait bound `{integer}: MyTrait` is not satisfied
  --> src/main.rs:12:26
   |
12 |     take_mytrait(Wrapper(5));
   |     ------------         ^ the trait `MyTrait` is not implemented for `{integer}`, which is required by `Wrapper<{integer}>: MyTrait`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the trait `MyTrait` is implemented for `Wrapper<T>`
note: required for `Wrapper<{integer}>` to implement `MyTrait`
  --> src/main.rs:6:18
   |
6  | impl<T: MyTrait> MyTrait for Wrapper<T> {}
   |         -------  ^^^^^^^     ^^^^^^^^^^
   |         |
   |         unsatisfied trait bound introduced here
note: required by a bound in `take_mytrait`
  --> src/main.rs:9:25
   |
9  | fn take_mytrait(_: impl MyTrait) {}
   |                         ^^^^^^^ required by this bound in `take_mytrait`

For more information about this error, try `rustc --explain E0277`.

edit: jinx

@cramertj
Copy link
Member

For the trait solver, if neither impl applies they're basically just... equally invalid. For diagnostics, it seems like a special case that's motivated for this example but could also be misleading in other places, especially when the blanket impls and concrete impls get more complex and "close" to overlapping.

One option would be to prefer impls whose type-without params (e.g. Wrapper<T> for some Wrapper<usize>) matches. If such an impl exists, it must be where the impl would come from-- the type can't plausibly be covered by the blanket impl without a conflict (well, until a hypothetical future with intersection impls).

This would also only work for generic record types, not type aliases (because e.g. type Wrapper<T> = <T as Trait>::Assoc;), but we already have to distinguish these two things for the purposes of e.g. coherence, where impl<T> Trait for SomeLocalType<T> is valid but impl<T> Trait for SomeLocalTypeAliasThatMayPointToANonLocalType<T> is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-discussion Category: Discussion or questions that doesn't represent real issues. D-confusing Diagnostics: Confusing error or lint that should be reworked. F-arbitrary_self_types `#![feature(arbitrary_self_types)]`
Projects
None yet
Development

No branches or pull requests

4 participants