about summary refs log tree commit diff
path: root/tests
diff options
context:
space:
mode:
authorJason Newcomb <jsnewcomb@pm.me>2025-02-12 15:17:52 +0000
committerGitHub <noreply@github.com>2025-02-12 15:17:52 +0000
commit1cb4236a95b7ecd10b01d91e598bfd7f6c5188ad (patch)
tree6912d23d7ffb464dcac3480bdb49aefbcdd1b900 /tests
parenta342340e6deb329b8eb9f2cf57275fd282ad14c8 (diff)
parent8b6de49ef74de5bc5485ccb507178fa72dff9c87 (diff)
downloadrust-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.fixed6
-rw-r--r--tests/ui/bytes_count_to_len.rs6
-rw-r--r--tests/ui/unbuffered_bytes.rs37
-rw-r--r--tests/ui/unbuffered_bytes.stderr36
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
+