Skip to content

Catch use of PhantomData that requires T: Sized #2308

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

Open
dtolnay opened this issue Dec 27, 2017 · 4 comments · May be fixed by #8526
Open

Catch use of PhantomData that requires T: Sized #2308

dtolnay opened this issue Dec 27, 2017 · 4 comments · May be fixed by #8526
Labels
good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types

Comments

@dtolnay
Copy link
Member

dtolnay commented Dec 27, 2017

In Serde the traits we implement for PhantomData should have worked for T: ?Sized, but as a bug they required T: Sized -- fixed in serde-rs/serde@4751627. Could Clippy have caught this? I struggle to think of any use of PhantomData<T> 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 oli-obk added good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types labels Dec 27, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Dec 27, 2017

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
Copy link
Member Author

dtolnay commented Dec 27, 2017

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
Copy link

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

@Jarcho Jarcho linked a pull request Mar 18, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants