Auto merge of #8964 - tamaroning:read_zero_byte_vec, r=dswij

Warn about read into zero-length `Vec`

Closes #8886

- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`

changelog: none

CHANGELOG.md

@@ -3678,6 +3678,7 @@ Released 2018-09-13
 [`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
 [`rc_clone_in_vec_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_clone_in_vec_init
 [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex
+[`read_zero_byte_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#read_zero_byte_vec
 [`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl
 [`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
 [`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone

clippy_lints/src/lib.register_all.rs

@@ -271,6 +271,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(ranges::RANGE_ZIP_WITH_LEN),
     LintId::of(ranges::REVERSED_EMPTY_RANGES),
     LintId::of(rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT),
+    LintId::of(read_zero_byte_vec::READ_ZERO_BYTE_VEC),
     LintId::of(redundant_clone::REDUNDANT_CLONE),
     LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL),
     LintId::of(redundant_field_names::REDUNDANT_FIELD_NAMES),

clippy_lints/src/lib.register_correctness.rs

@@ -55,6 +55,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
     LintId::of(ptr::INVALID_NULL_PTR_USAGE),
     LintId::of(ptr::MUT_FROM_REF),
     LintId::of(ranges::REVERSED_EMPTY_RANGES),
+    LintId::of(read_zero_byte_vec::READ_ZERO_BYTE_VEC),
     LintId::of(regex::INVALID_REGEX),
     LintId::of(self_assignment::SELF_ASSIGNMENT),
     LintId::of(serde_api::SERDE_API_MISUSE),

clippy_lints/src/lib.register_lints.rs

@@ -459,6 +459,7 @@ store.register_lints(&[
     ranges::RANGE_ZIP_WITH_LEN,
     ranges::REVERSED_EMPTY_RANGES,
     rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT,
+    read_zero_byte_vec::READ_ZERO_BYTE_VEC,
     redundant_clone::REDUNDANT_CLONE,
     redundant_closure_call::REDUNDANT_CLOSURE_CALL,
     redundant_else::REDUNDANT_ELSE,

clippy_lints/src/lib.rs

@@ -348,6 +348,7 @@ mod pub_use;
 mod question_mark;
 mod ranges;
 mod rc_clone_in_vec_init;
+mod read_zero_byte_vec;
 mod redundant_clone;
 mod redundant_closure_call;
 mod redundant_else;
@@ -908,6 +909,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(swap_ptr_to_ref::SwapPtrToRef));
     store.register_late_pass(|| Box::new(mismatching_type_param_order::TypeParamMismatch));
     store.register_late_pass(|| Box::new(as_underscore::AsUnderscore));
+    store.register_late_pass(|| Box::new(read_zero_byte_vec::ReadZeroByteVec));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/read_zero_byte_vec.rs

@@ -0,0 +1,142 @@
+use clippy_utils::{
+    diagnostics::{span_lint, span_lint_and_sugg},
+    higher::{get_vec_init_kind, VecInitKind},
+    source::snippet,
+    visitors::expr_visitor_no_bodies,
+};
+use hir::{intravisit::Visitor, ExprKind, Local, PatKind, PathSegment, QPath, StmtKind};
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// This lint catches reads into a zero-length `Vec`.
+    /// Especially in the case of a call to `with_capacity`, this lint warns that read
+    /// gets the number of bytes from the `Vec`'s length, not its capacity.
+    ///
+    /// ### Why is this bad?
+    /// Reading zero bytes is almost certainly not the intended behavior.
+    ///
+    /// ### Known problems
+    /// In theory, a very unusual read implementation could assign some semantic meaning
+    /// to zero-byte reads. But it seems exceptionally unlikely that code intending to do
+    /// a zero-byte read would allocate a `Vec` for it.
+    ///
+    /// ### Example
+    /// ```rust
+    /// use std::io;
+    /// fn foo<F: io::Read>(mut f: F) {
+    ///     let mut data = Vec::with_capacity(100);
+    ///     f.read(&mut data).unwrap();
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::io;
+    /// fn foo<F: io::Read>(mut f: F) {
+    ///     let mut data = Vec::with_capacity(100);
+    ///     data.resize(100, 0);
+    ///     f.read(&mut data).unwrap();
+    /// }
+    /// ```
+    #[clippy::version = "1.63.0"]
+    pub READ_ZERO_BYTE_VEC,
+    correctness,
+    "checks for reads into a zero-length `Vec`"
+}
+declare_lint_pass!(ReadZeroByteVec => [READ_ZERO_BYTE_VEC]);
+
+impl<'tcx> LateLintPass<'tcx> for ReadZeroByteVec {
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &hir::Block<'tcx>) {
+        for (idx, stmt) in block.stmts.iter().enumerate() {
+            if !stmt.span.from_expansion()
+                // matches `let v = Vec::new();`
+                && let StmtKind::Local(local) = stmt.kind
+                && let Local { pat, init: Some(init), .. } = local
+                && let PatKind::Binding(_, _, ident, _) = pat.kind
+                && let Some(vec_init_kind) = get_vec_init_kind(cx, init)
+            {
+                // finds use of `_.read(&mut v)`
+                let mut read_found = false;
+                let mut visitor = expr_visitor_no_bodies(|expr| {
+                    if let ExprKind::MethodCall(path, [_self, arg], _) = expr.kind
+                        && let PathSegment { ident: read_or_read_exact, .. } = *path
+                        && matches!(read_or_read_exact.as_str(), "read" | "read_exact")
+                        && let ExprKind::AddrOf(_, hir::Mutability::Mut, inner) = arg.kind
+                        && let ExprKind::Path(QPath::Resolved(None, inner_path)) = inner.kind
+                        && let [inner_seg] = inner_path.segments
+                        && ident.name == inner_seg.ident.name
+                    {
+                        read_found = true;
+                    }
+                    !read_found
+                });
+
+                let next_stmt_span;
+                if idx == block.stmts.len() - 1 {
+                    // case { .. stmt; expr }
+                    if let Some(e) = block.expr {
+                        visitor.visit_expr(e);
+                        next_stmt_span = e.span;
+                    } else {
+                        return;
+                    }
+                } else {
+                    // case { .. stmt; stmt; .. }
+                    let next_stmt = &block.stmts[idx + 1];
+                    visitor.visit_stmt(next_stmt);
+                    next_stmt_span = next_stmt.span;
+                }
+                drop(visitor);
+
+                if read_found && !next_stmt_span.from_expansion() {
+                    let applicability = Applicability::MaybeIncorrect;
+                    match vec_init_kind {
+                        VecInitKind::WithConstCapacity(len) => {
+                            span_lint_and_sugg(
+                                cx,
+                                READ_ZERO_BYTE_VEC,
+                                next_stmt_span,
+                                "reading zero byte data to `Vec`",
+                                "try",
+                                format!("{}.resize({}, 0); {}",
+                                    ident.as_str(),
+                                    len,
+                                    snippet(cx, next_stmt_span, "..")
+                                ),
+                                applicability,
+                            );
+                        }
+                        VecInitKind::WithExprCapacity(hir_id) => {
+                            let e = cx.tcx.hir().expect_expr(hir_id);
+                            span_lint_and_sugg(
+                                cx,
+                                READ_ZERO_BYTE_VEC,
+                                next_stmt_span,
+                                "reading zero byte data to `Vec`",
+                                "try",
+                                format!("{}.resize({}, 0); {}",
+                                    ident.as_str(),
+                                    snippet(cx, e.span, ".."),
+                                    snippet(cx, next_stmt_span, "..")
+                                ),
+                                applicability,
+                            );
+                        }
+                        _ => {
+                            span_lint(
+                                cx,
+                                READ_ZERO_BYTE_VEC,
+                                next_stmt_span,
+                                "reading zero byte data to `Vec`",
+                            );
+
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

tests/ui/read_zero_byte_vec.rs

@@ -0,0 +1,87 @@
+#![warn(clippy::read_zero_byte_vec)]
+#![allow(clippy::unused_io_amount)]
+use std::fs::File;
+use std::io;
+use std::io::prelude::*;
+
+extern crate futures;
+use futures::io::{AsyncRead, AsyncReadExt};
+use tokio::io::{AsyncRead as TokioAsyncRead, AsyncReadExt as _, AsyncWrite as TokioAsyncWrite, AsyncWriteExt as _};
+
+fn test() -> io::Result<()> {
+    let cap = 1000;
+    let mut f = File::open("foo.txt").unwrap();
+
+    // should lint
+    let mut data = Vec::with_capacity(20);
+    f.read_exact(&mut data).unwrap();
+
+    // should lint
+    let mut data2 = Vec::with_capacity(cap);
+    f.read_exact(&mut data2)?;
+
+    // should lint
+    let mut data3 = Vec::new();
+    f.read_exact(&mut data3)?;
+
+    // should lint
+    let mut data4 = vec![];
+    let _ = f.read(&mut data4)?;
+
+    // should lint
+    let _ = {
+        let mut data5 = Vec::new();
+        f.read(&mut data5)
+    };
+
+    // should lint
+    let _ = {
+        let mut data6: Vec<u8> = Default::default();
+        f.read(&mut data6)
+    };
+
+    // should not lint
+    let mut buf = [0u8; 100];
+    f.read(&mut buf)?;
+
+    // should not lint
+    let mut empty = vec![];
+    let mut data7 = vec![];
+    f.read(&mut empty);
+
+    // should not lint
+    f.read(&mut data7);
+
+    // should not lint
+    let mut data8 = Vec::new();
+    data8.resize(100, 0);
+    f.read_exact(&mut data8)?;
+
+    // should not lint
+    let mut data9 = vec![1, 2, 3];
+    f.read_exact(&mut data9)?;
+
+    Ok(())
+}
+
+async fn test_futures<R: AsyncRead + Unpin>(r: &mut R) {
+    // should lint
+    let mut data = Vec::new();
+    r.read(&mut data).await.unwrap();
+
+    // should lint
+    let mut data2 = Vec::new();
+    r.read_exact(&mut data2).await.unwrap();
+}
+
+async fn test_tokio<R: TokioAsyncRead + Unpin>(r: &mut R) {
+    // should lint
+    let mut data = Vec::new();
+    r.read(&mut data).await.unwrap();
+
+    // should lint
+    let mut data2 = Vec::new();
+    r.read_exact(&mut data2).await.unwrap();
+}
+
+fn main() {}

tests/ui/read_zero_byte_vec.stderr

@@ -0,0 +1,64 @@
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:17:5
+   |
+LL |     f.read_exact(&mut data).unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data.resize(20, 0); f.read_exact(&mut data).unwrap();`
+   |
+   = note: `-D clippy::read-zero-byte-vec` implied by `-D warnings`
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:21:5
+   |
+LL |     f.read_exact(&mut data2)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data2.resize(cap, 0); f.read_exact(&mut data2)?;`
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:25:5
+   |
+LL |     f.read_exact(&mut data3)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:29:5
+   |
+LL |     let _ = f.read(&mut data4)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:34:9
+   |
+LL |         f.read(&mut data5)
+   |         ^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:40:9
+   |
+LL |         f.read(&mut data6)
+   |         ^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:70:5
+   |
+LL |     r.read(&mut data).await.unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:74:5
+   |
+LL |     r.read_exact(&mut data2).await.unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:80:5
+   |
+LL |     r.read(&mut data).await.unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: reading zero byte data to `Vec`
+  --> $DIR/read_zero_byte_vec.rs:84:5
+   |
+LL |     r.read_exact(&mut data2).await.unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 10 previous errors
+