New lint: unbuffered_bytes

[JonathanBrouwer]

Checks for Read::bytes() on an unbuffered Read type.
The default implementation calls read for each byte, which can be very inefficient for data that’s not in memory, such as File.

Considerations which I'd like to have feedback on:

• Currently this lint triggers when .bytes() is called on any type that implements std::io::Read but not std::io::BufRead. This is quite aggressive and in and may result in false positives. Alternatives:
  • Only trigger on concrete types, not generic types. This does mean that cases where a function is generic over a R: Read and calls .bytes() are not caught by the lint, which could be quite a nasty case of this bug.
  • Only trigger on an allowlist of stdlib types
  • Compromise: Is it possible to make this lint pedantic on types that are not on a allowlist?
• Theoretically, a trait implementation of Read could override .bytes() with an efficient implementation. I'm not sure how to add this check to the lint, and I can't find any cases of this being done in practice.
• I don't think an automatic fix for this lint is possible, but I'd love to be proven wrong
• This is my first time contributing to clippy, please let me know if I did anything wrong

Fixes #14087

changelog: [`unbuffered_bytes`]: new lint

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[JonathanBrouwer]

I made a few more changes, should be ready for review now

[Jarcho]

Looks fine minus a few nits.

Theoretically, a trait implementation of Read could override .bytes() with an efficient implementation. I'm not sure how to add this check to the lint, and I can't find any cases of this being done in practice.

This is not possible given the return type of bytes. The only potential false positive would be:

• A type that generates the data. No way to detect this, but generating one byte at a time isn't necessarily efficient either.
• A type which reads from memory. These really should just implement BufRead, but they may not.
• Opaque types which hide a BufRead impl. This can be worked around, but it's probably not very common.

I don't think an automatic fix for this lint is possible, but I'd love to be proven wrong

You can wrap the receiver in a call to std::io::BufRead::new. You'll need to use walk_span_to_context(recv.span, expr.span) and then a multipart suggestion inserting to the start and end of the span.

Compromise: Is it possible to make this lint pedantic on types that are not on a allowlist?

The closest thing is to have a configuration value for the lint to make it more or less aggressive. We might at some point get different predefined configs, but that's not going to be anytime soon.

[JonathanBrouwer]

@Jarcho I've resolved your comments, thanks for the feedback!

[Jarcho]

Looks good. Can you just squash the commits please.

If you want to add the suggestion to the lint feel free, but it's not needed.

[JonathanBrouwer]

I've squashed my commits @Jarcho

I may try to do the suggestion later (in a future PR), when I have a bit more time on my hands.

[Jarcho]

FCP started: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.3A.20unbuffered_bytes