diff --git a/CHANGELOG.md b/CHANGELOG.md index 14f61fb1cd91..2d37e0da37f0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index d5a8f875e472..ac3e4bd90dc1 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7d33f3de57d6..7e9db66ff864 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; @@ -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); diff --git a/clippy_lints/src/methods/unbuffered_bytes.rs b/clippy_lints/src/methods/unbuffered_bytes.rs new file mode 100644 index 000000000000..71c23d256ac6 --- /dev/null +++ b/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`", + ); + } + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index f15fffc09e8d..74a392354894 100644 --- a/clippy_utils/src/paths.rs +++ b/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"]; 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..82c8839e8391 --- /dev/null +++ b/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 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: 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..3303d579fedc --- /dev/null +++ b/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 +