Make impls apply for trait objects

[dtolnay]

It looks like there may be some ?Sized bounds missing? I would expect the following auto impls to apply to <T: ?Sized> &T and <T: ?Sized> Box<T> but they don't.

use auto_impl::auto_impl;

#[auto_impl(&, Box)]
trait Trait {}

fn assert_impl<T: Trait>() {}

fn main() {
    assert_impl::<&dyn Trait>();
    assert_impl::<Box<dyn Trait>>();
}

error[E0277]: the size for values of type `dyn Trait` cannot be known at compilation time
 --> src/main.rs:9:5
  |
9 |     assert_impl::<&dyn Trait>();
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
  |
  = help: the trait `std::marker::Sized` is not implemented for `dyn Trait`
  = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
  = note: required because of the requirements on the impl of `Trait` for `&dyn Trait`
note: required by `assert_impl`
 --> src/main.rs:6:1
  |
6 | fn assert_impl<T: Trait>() {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the size for values of type `dyn Trait` cannot be known at compilation time
  --> src/main.rs:10:5
   |
10 |     assert_impl::<Box<dyn Trait>>();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `dyn Trait`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required because of the requirements on the impl of `Trait` for `std::boxed::Box<dyn Trait>`
note: required by `assert_impl`
  --> src/main.rs:6:1
   |
6  | fn assert_impl<T: Trait>() {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^

[LukasKalbertodt]

Nice idea! Implementing this is fairly easy. But I noticed a problem: what should happen if the trait has a method with self receiver? Then the generated impl block with ?Sized bounds causes an "the size for values of type T cannot be known at compilation time" error.

So should we just add ?Sized bounds whenever possible? And just don't add them if it's impossible due to a method or so? Or let the user configure that somehow? But configuration requires new syntax and makes the crate more complicated to use. Any ideas?

[dtolnay]

Good catch -- when there are self-by-value methods then the impl shouldn't have a ?Sized bound.

[LukasKalbertodt]

My question was rather about: if we conditionally add the : ?Sized bound, that might be surprising for the user of the library. As in: they use a trait with trait objects, add a "self by value" method to the trait and suddenly auto_impl does something different and their stuff doesn't work anymore. Or is such a situation unlikely?

Also we should probably ignore "self by value" methods which are marked with where Self: Sized.

[KodrAus]

So I guess there are a few different directions we could take:

1. Silently add ?Sized bounds when they're possible
2. Add another container type, like &dyn that implies & and will fail if there are by-value self receivers
3. Allow custom bounds like T: ?Sized to be added by consumers

Personally, I think 1 (and maybe 2 as well?) would be ok. The argument about it being surprising is totally reasonable, but I think the fail wagon would ride like this: you define your trait as object safe and #[auto_impl(&)], you depend on it being object-safe and it works, you change your trait so that it's no longer object safe and you start getting compile errors. In that case I don't think the cause and effect are too far apart. But that's where 2 might help if you don't just want it to be a coincidence that your trait is auto-implemented for dyn Trait.

[KodrAus]

(I'm kind of conflating object safety with ?Sized bounds, which isn't totally accurate but I think you get what I mean 🙂)

[dtolnay]

+1 for @KodrAus's option 1. If my code asks for auto_impl(&, Box) then I would expect those impls to apply whenever possible; for ?Sized if possible, and not for ?Sized if that wouldn't be possible.

[LukasKalbertodt]

Ok, I'm convinced! Will probably implement option 1 in the next days.

[LukasKalbertodt]

@dtolnay In #54 I basically implemented what we discussed here. Did you have a particular use case in mind? If so, could you test it with that patch? If it works, we can close this issue. If you didn't have any particular thing in mind, I would probably close it without further testing.

[dtolnay]

My use case looks like https://github.com/auto-impl-rs/auto_impl/blob/90f17b69350204689730de3a5c4cc17d038d8d94/tests/compile-pass/trait_obj_immutable_self.rs so I think we can close this out. Thanks!