diff options
| author | Jason Newcomb <jsnewcomb@pm.me> | 2025-02-12 15:17:52 +0000 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2025-02-12 15:17:52 +0000 |
| commit | 1cb4236a95b7ecd10b01d91e598bfd7f6c5188ad (patch) | |
| tree | 6912d23d7ffb464dcac3480bdb49aefbcdd1b900 /tests | |
| parent | a342340e6deb329b8eb9f2cf57275fd282ad14c8 (diff) | |
| parent | 8b6de49ef74de5bc5485ccb507178fa72dff9c87 (diff) | |
| download | rust-1cb4236a95b7ecd10b01d91e598bfd7f6c5188ad.tar.gz rust-1cb4236a95b7ecd10b01d91e598bfd7f6c5188ad.zip | |
New lint: `unbuffered_bytes` (#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 #14087 ``` changelog: [`unbuffered_bytes`]: new lint ```
Diffstat (limited to 'tests')
| -rw-r--r-- | tests/ui/bytes_count_to_len.fixed | 6 | ||||
| -rw-r--r-- | tests/ui/bytes_count_to_len.rs | 6 | ||||
| -rw-r--r-- | tests/ui/unbuffered_bytes.rs | 37 | ||||
| -rw-r--r-- | tests/ui/unbuffered_bytes.stderr | 36 |
4 files changed, 79 insertions, 6 deletions
diff --git a/tests/ui/bytes_count_to_len.fixed b/tests/ui/bytes_count_to_len.fixed index d20af22535a..fb31ceaad7c 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 340e6b41255..0250059afeb 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 00000000000..82c8839e839 --- /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<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(); +} diff --git a/tests/ui/unbuffered_bytes.stderr b/tests/ui/unbuffered_bytes.stderr new file mode 100644 index 00000000000..3303d579fed --- /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 + |
