diff --git a/CHANGELOG.md b/CHANGELOG.md index 2757597fc511..adb68438f086 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ec223381aec6..28a1d8045f14 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index eaccaa824cef..d2b1a9431d2f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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,33 @@ 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::{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" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -4561,6 +4589,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 +4863,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); diff --git a/clippy_lints/src/methods/unbuffered_bytes.rs b/clippy_lints/src/methods/unbuffered_bytes.rs new file mode 100644 index 000000000000..7be37cd92b79 --- /dev/null +++ b/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`", + ); + } +} diff --git a/tests/ui/bytes_count_to_len.fixed b/tests/ui/bytes_count_to_len.fixed index d20af22535a0..fb31ceaad7c4 100644 --- a/tests/ui/bytes_count_to_len.fixed +++ b/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(); } diff --git a/tests/ui/bytes_count_to_len.rs b/tests/ui/bytes_count_to_len.rs index 340e6b412556..0250059afeb2 100644 --- a/tests/ui/bytes_count_to_len.rs +++ b/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(); } diff --git a/tests/ui/unbuffered_bytes.rs b/tests/ui/unbuffered_bytes.rs new file mode 100644 index 000000000000..ca0fa55f52a7 --- /dev/null +++ b/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 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: R) { + // Callers of `use_read` may choose a `R` that is not buffered + r.bytes(); +} diff --git a/tests/ui/unbuffered_bytes.stderr b/tests/ui/unbuffered_bytes.stderr new file mode 100644 index 000000000000..716a5c78e86a --- /dev/null +++ b/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 +