New lint: unbuffered_bytes (rust-lang#14089)

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 rust-lang#14087
```
changelog: [`unbuffered_bytes`]: new lint
```

Author: Jarcho

CHANGELOG.md

@@ -6144,6 +6144,7 @@ Released 2018-09-13
 [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
 [`type_id_on_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_id_on_box
 [`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
+[`unbuffered_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#unbuffered_bytes
 [`unchecked_duration_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction
 [`unconditional_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#unconditional_recursion
 [`undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks

clippy_lints/src/declared_lints.rs

@@ -482,6 +482,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::methods::SUSPICIOUS_SPLITN_INFO,
     crate::methods::SUSPICIOUS_TO_OWNED_INFO,
     crate::methods::TYPE_ID_ON_BOX_INFO,
+    crate::methods::UNBUFFERED_BYTES_INFO,
     crate::methods::UNINIT_ASSUMED_INIT_INFO,
     crate::methods::UNIT_HASH_INFO,
     crate::methods::UNNECESSARY_FALLIBLE_CONVERSIONS_INFO,

clippy_lints/src/methods/mod.rs

@@ -113,6 +113,7 @@ mod suspicious_map;
 mod suspicious_splitn;
 mod suspicious_to_owned;
 mod type_id_on_box;
+mod unbuffered_bytes;
 mod uninit_assumed_init;
 mod unit_hash;
 mod unnecessary_fallible_conversions;
@@ -4406,6 +4407,33 @@ declare_clippy_lint! {
     "using `Option::and_then` or `Result::and_then` to chain a computation that returns an `Option` or a `Result`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for calls to `Read::bytes` on types which don't implement `BufRead`.
+    ///
+    /// ### Why is this bad?
+    /// The default implementation calls `read` for each byte, which can be very inefficient for data that’s not in memory, such as `File`.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// use std::io::Read;
+    /// use std::fs::File;
+    /// let file = File::open("./bytes.txt").unwrap();
+    /// file.bytes();
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// use std::io::{BufReader, Read};
+    /// use std::fs::File;
+    /// let file = BufReader::new(std::fs::File::open("./bytes.txt").unwrap());
+    /// file.bytes();
+    /// ```
+    #[clippy::version = "1.86.0"]
+    pub UNBUFFERED_BYTES,
+    perf,
+    "calling .bytes() is very inefficient when data is not in memory"
+}
+
 #[expect(clippy::struct_excessive_bools)]
 pub struct Methods {
     avoid_breaking_exported_api: bool,
@@ -4580,6 +4608,7 @@ impl_lint_pass!(Methods => [
     MANUAL_REPEAT_N,
     SLICED_STRING_AS_BYTES,
     RETURN_AND_THEN,
+    UNBUFFERED_BYTES,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4856,6 +4885,7 @@ impl Methods {
                 ("as_ptr", []) => manual_c_str_literals::check_as_ptr(cx, expr, recv, &self.msrv),
                 ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv),
                 ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv),
+                ("bytes", []) => unbuffered_bytes::check(cx, expr, recv),
                 ("cloned", []) => {
                     cloned_instead_of_copied::check(cx, expr, recv, span, &self.msrv);
                     option_as_ref_cloned::check(cx, recv, span);

clippy_lints/src/methods/unbuffered_bytes.rs

@@ -0,0 +1,31 @@
+use super::UNBUFFERED_BYTES;
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::ty::implements_trait;
+use clippy_utils::{get_trait_def_id, is_trait_method, paths};
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>) {
+    let ty = cx.typeck_results().expr_ty_adjusted(recv);
+
+    // If the .bytes() call is a call from the Read trait
+    if is_trait_method(cx, expr, sym::IoRead) {
+        // Retrieve the DefId of the BufRead trait
+        // FIXME: add a diagnostic item for `BufRead`
+        let Some(buf_read) = get_trait_def_id(cx.tcx, &paths::BUF_READ) else {
+            return;
+        };
+        // And the implementor of the trait is not buffered
+        if !implements_trait(cx, ty, buf_read, &[]) {
+            span_lint_and_help(
+                cx,
+                UNBUFFERED_BYTES,
+                expr.span,
+                "calling .bytes() is very inefficient when data is not in memory",
+                None,
+                "consider using `BufReader`",
+            );
+        }
+    }
+}

clippy_utils/src/paths.rs

@@ -29,6 +29,7 @@ pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]
 
 // Paths in `core`/`alloc`/`std`. This should be avoided and cleaned up by adding diagnostic items.
 pub const ABORT: [&str; 3] = ["std", "process", "abort"];
+pub const BUF_READ: [&str; 3] = ["std", "io", "BufRead"];
 pub const CHILD: [&str; 3] = ["std", "process", "Child"];
 pub const CHILD_ID: [&str; 4] = ["std", "process", "Child", "id"];
 pub const CHILD_KILL: [&str; 4] = ["std", "process", "Child", "kill"];

tests/ui/bytes_count_to_len.fixed

@@ -1,6 +1,6 @@
 #![warn(clippy::bytes_count_to_len)]
 use std::fs::File;
-use std::io::Read;
+use std::io::{BufReader, Read};
 
 fn main() {
     // should fix, because type is String
@@ -26,8 +26,8 @@ fn main() {
     bytes.bytes().count();
 
     // The type is File, so should not fix
-    let _ = File::open("foobar").unwrap().bytes().count();
+    let _ = BufReader::new(File::open("foobar").unwrap()).bytes().count();
 
-    let f = File::open("foobar").unwrap();
+    let f = BufReader::new(File::open("foobar").unwrap());
     let _ = f.bytes().count();
 }

tests/ui/bytes_count_to_len.rs

@@ -1,6 +1,6 @@
 #![warn(clippy::bytes_count_to_len)]
 use std::fs::File;
-use std::io::Read;
+use std::io::{BufReader, Read};
 
 fn main() {
     // should fix, because type is String
@@ -26,8 +26,8 @@ fn main() {
     bytes.bytes().count();
 
     // The type is File, so should not fix
-    let _ = File::open("foobar").unwrap().bytes().count();
+    let _ = BufReader::new(File::open("foobar").unwrap()).bytes().count();
 
-    let f = File::open("foobar").unwrap();
+    let f = BufReader::new(File::open("foobar").unwrap());
     let _ = f.bytes().count();
 }

tests/ui/unbuffered_bytes.rs

@@ -0,0 +1,37 @@
+#![warn(clippy::unbuffered_bytes)]
+
+use std::fs::File;
+use std::io::{BufReader, Cursor, Read, Stdin, stdin};
+use std::net::TcpStream;
+
+fn main() {
+    // File is not buffered, should complain
+    let file = File::open("./bytes.txt").unwrap();
+    file.bytes();
+
+    // TcpStream is not buffered, should complain
+    let tcp_stream: TcpStream = TcpStream::connect("127.0.0.1:80").unwrap();
+    tcp_stream.bytes();
+
+    // BufReader<File> is buffered, should not complain
+    let file = BufReader::new(File::open("./bytes.txt").unwrap());
+    file.bytes();
+
+    // Cursor is buffered, should not complain
+    let cursor = Cursor::new(Vec::new());
+    cursor.bytes();
+
+    // Stdio would acquire the lock for every byte, should complain
+    let s: Stdin = stdin();
+    s.bytes();
+
+    // But when locking stdin, this is fine so should not complain
+    let s: Stdin = stdin();
+    let s = s.lock();
+    s.bytes();
+}
+
+fn use_read<R: Read>(r: R) {
+    // Callers of `use_read` may choose a `R` that is not buffered
+    r.bytes();
+}

tests/ui/unbuffered_bytes.stderr

@@ -0,0 +1,36 @@
+error: calling .bytes() is very inefficient when data is not in memory
+  --> tests/ui/unbuffered_bytes.rs:10:5
+   |
+LL |     file.bytes();
+   |     ^^^^^^^^^^^^
+   |
+   = help: consider using `BufReader`
+   = note: `-D clippy::unbuffered-bytes` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unbuffered_bytes)]`
+
+error: calling .bytes() is very inefficient when data is not in memory
+  --> tests/ui/unbuffered_bytes.rs:14:5
+   |
+LL |     tcp_stream.bytes();
+   |     ^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using `BufReader`
+
+error: calling .bytes() is very inefficient when data is not in memory
+  --> tests/ui/unbuffered_bytes.rs:26:5
+   |
+LL |     s.bytes();
+   |     ^^^^^^^^^
+   |
+   = help: consider using `BufReader`
+
+error: calling .bytes() is very inefficient when data is not in memory
+  --> tests/ui/unbuffered_bytes.rs:36:5
+   |
+LL |     r.bytes();
+   |     ^^^^^^^^^
+   |
+   = help: consider using `BufReader`
+
+error: aborting due to 4 previous errors
+