New lint: unbuffered_bytes

Author: JonathanBrouwer

CHANGELOG.md

@@ -6138,6 +6138,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

@@ -479,6 +479,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;
@@ -4392,6 +4393,34 @@ declare_clippy_lint! {
      "slicing a string and immediately calling as_bytes is less efficient and can lead to panics"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `Read::bytes()` on an unbuffered Read type
+    ///
+    /// ### 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::Read;
+    /// use std::fs::File;
+    /// use std::fs::BufReader;
+    /// 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"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4561,6 +4590,7 @@ impl_lint_pass!(Methods => [
     USELESS_NONZERO_NEW_UNCHECKED,
     MANUAL_REPEAT_N,
     SLICED_STRING_AS_BYTES,
+    UNBUFFERED_BYTES,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4834,6 +4864,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,33 @@
+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};
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+/// lint to detect `.bytes()` on an unbuffered type such as a `File` or `TcpStream`
+pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>) {
+    // Retrieve the DefId of the BufRead trait
+    // Sadly BufRead does not have a diagnostic item or a lang item
+    let Some(buf_read) = get_trait_def_id(cx.tcx, &["std", "io", "BufRead"]) else {
+        return;
+    };
+
+    let typ = cx.typeck_results().expr_ty_adjusted(recv);
+    if
+    // The .bytes() call is a call from the given trait
+    is_trait_method(cx, expr, sym::IoRead)
+        // And the implementor of the trait is not buffered
+        && !implements_trait(cx, typ, 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`",
+        );
+    }
+}

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,28 @@
+#![warn(clippy::unbuffered_bytes)]
+
+use std::fs::File;
+use std::io::{BufReader, Cursor, Read};
+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 = todo!();
+    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();
+}
+
+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,28 @@
+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:27:5
+   |
+LL |     r.bytes();
+   |     ^^^^^^^^^
+   |
+   = help: consider using `BufReader`
+
+error: aborting due to 3 previous errors
+