rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.46k stars 1.54k forks source link

Catch use of PhantomData that requires T: Sized #2308

Open dtolnay opened 6 years ago

dtolnay commented 6 years ago

In Serde the traits we implement for PhantomData should have worked for T: ?Sized, but as a bug they required T: Sized -- fixed in https://github.com/serde-rs/serde/commit/4751627f1cd14cacdf216188ccbb9ab0831e2b3f. Could Clippy have caught this? I struggle to think of any use of PhantomData\ where T is an unconstrained type parameter where you would not want to allow ?Sized. As a starting point, maybe we could catch impls of the form:

impl<..., T, ...> Trait<...> for PhantomData<T> { /* ... */ }

where there are no trait bounds on T. In this case you pretty much always want T: ?Sized right?

oli-obk commented 6 years ago

The latter I agree with being a clear case where a ?Sized bound is always the right thing.

I can't think of any counter examples for arbitrary bounds, we should probably just run it on a few big crates and see what happens.

dtolnay commented 6 years ago

I found a few instances of this in github search that would all want ?Sized:

Here is one that would be a false positive 😿 -- using the T for an associated type that requires Sized.

dtolnay commented 6 years ago

Oh but the "false positive" has a where-clause T: Deserialize<'de> and we have Deserialize<'de>: Sized, so hopefully we can avoid recommending a ?Sized bound in that situation because it would do nothing.

clarfonthey commented 6 years ago

Perhaps this lint could be generalised to requiring Sized in cases where it's not needed?