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

Extension traits should not be used as trait bounds #9390

Open
Darksonn opened this issue Aug 28, 2022 · 4 comments
Open

Extension traits should not be used as trait bounds #9390

Darksonn opened this issue Aug 28, 2022 · 4 comments
Labels
A-lint Area: New lints

Comments

@Darksonn
Copy link

Darksonn commented Aug 28, 2022

What it does

It detects when an extension trait is used as a trait bound. The base trait should be used instead. For example:

async fn takes_async_read<R: AsyncReadExt>(read: R) { ... }

In this case, it should suggest writing R: AsyncRead instead.

To define "extension trait", one possibility is any trait of the following form:

trait MyTraitExt: MyTrait { .. }

impl<T: MyTrait + ?Sized> MyTraitExt for T {}

Lint Name

extension-trait-as-trait-bound

Category

style

Advantage

  • It is conceptually incorrect.

Drawbacks

Somewhat unclear how "extension trait" should be defined. For example, should + ?Sized be required for it to be an extension trait?

It's also unclear if there are cases where this would be a false positive.

Example

async fn takes_async_read<R: AsyncReadExt>(read: R) { ... }

Could be written as:

async fn takes_async_read<R: AsyncRead>(read: R) { ... }
@Darksonn Darksonn added the A-lint Area: New lints label Aug 28, 2022
@lukaslueg
Copy link
Contributor

It's not clear to me why a trait bound like this should be considered bad practice. AsyncReadExt: AsyncRead, and takes_async_read may require both.

@Darksonn
Copy link
Author

Due to the blanket impl, R: AsyncRead implies R: AsyncReadExt, so they are equivalent mechanically. I suppose "bad style" would be a better category.

Here's another example:

fn takes_iterator<I: itertools::Itertools>(iter: I) { .. }

However, people tend to do this more often with AsyncRead.

@lukaslueg
Copy link
Contributor

lukaslueg commented Aug 28, 2022

Ah, now I see your point. As any R that is R: AsyncReadExt has to be R: AsyncRead (due to pub trait AsyncReadExt: AsyncRead), and as all R: AsyncRead + ?Sized are AsyncReadExt due to impl<R: AsyncRead + ?Sized> AsyncReadExt for R, a trait bound R: AsyncReadExt is never more powerful than a R: AsyncRead.

I guess one could make a list of common(?) traits which have this property. Solving the general case is probably quite hard.

@Darksonn
Copy link
Author

I've only ever seen people make the mistake with traits called StreamExt, AsyncReadExt, and AsyncWriteExt. (Though note that all of those names have multiple instances of them in different crates.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

2 participants