Skip to content

read_zero_byte_vec refactor for better heuristics #11766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 17, 2024
Merged

read_zero_byte_vec refactor for better heuristics #11766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 71 additions & 47 deletions clippy_lints/src/read_zero_byte_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
use clippy_utils::get_enclosing_block;
use clippy_utils::higher::{get_vec_init_kind, VecInitKind};
use clippy_utils::source::snippet;
use clippy_utils::visitors::for_each_expr;
use core::ops::ControlFlow;
use hir::{Expr, ExprKind, Local, PatKind, PathSegment, QPath, StmtKind};

use hir::{Expr, ExprKind, HirId, Local, PatKind, PathSegment, QPath, StmtKind};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};

Expand Down Expand Up @@ -49,57 +51,40 @@ 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
for stmt in block.stmts {
if stmt.span.from_expansion() {
return;
}

if let StmtKind::Local(local) = stmt.kind
&& let Local {
pat, init: Some(init), ..
} = local
&& let PatKind::Binding(_, id, ident, _) = pat.kind
&& let Some(vec_init_kind) = get_vec_init_kind(cx, init)
{
let visitor = |expr: &Expr<'_>| {
if let ExprKind::MethodCall(path, _, [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
{
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
let mut visitor = ReadVecVisitor {
local_id: id,
read_zero_expr: None,
has_resize: false,
};

let (read_found, next_stmt_span) = if let Some(next_stmt) = block.stmts.get(idx + 1) {
// case { .. stmt; stmt; .. }
(for_each_expr(next_stmt, visitor).is_some(), next_stmt.span)
} else if let Some(e) = block.expr {
// case { .. stmt; expr }
(for_each_expr(e, visitor).is_some(), e.span)
} else {
let Some(enclosing_block) = get_enclosing_block(cx, id) else {
return;
};
visitor.visit_block(enclosing_block);

if read_found && !next_stmt_span.from_expansion() {
if let Some(expr) = visitor.read_zero_expr {
let applicability = Applicability::MaybeIncorrect;
match vec_init_kind {
VecInitKind::WithConstCapacity(len) => {
span_lint_and_sugg(
cx,
READ_ZERO_BYTE_VEC,
next_stmt_span,
expr.span,
"reading zero byte data to `Vec`",
"try",
format!(
"{}.resize({len}, 0); {}",
ident.as_str(),
snippet(cx, next_stmt_span, "..")
),
format!("{}.resize({len}, 0); {}", ident.as_str(), snippet(cx, expr.span, "..")),
applicability,
);
},
Expand All @@ -108,29 +93,68 @@ impl<'tcx> LateLintPass<'tcx> for ReadZeroByteVec {
span_lint_and_sugg(
cx,
READ_ZERO_BYTE_VEC,
next_stmt_span,
expr.span,
"reading zero byte data to `Vec`",
"try",
format!(
"{}.resize({}, 0); {}",
ident.as_str(),
snippet(cx, e.span, ".."),
snippet(cx, next_stmt_span, "..")
snippet(cx, expr.span, "..")
),
applicability,
);
},
_ => {
span_lint(
cx,
READ_ZERO_BYTE_VEC,
next_stmt_span,
"reading zero byte data to `Vec`",
);
span_lint(cx, READ_ZERO_BYTE_VEC, expr.span, "reading zero byte data to `Vec`");
},
}
}
}
}
}
}

struct ReadVecVisitor<'tcx> {
local_id: HirId,
read_zero_expr: Option<&'tcx Expr<'tcx>>,
has_resize: bool,
}

impl<'tcx> Visitor<'tcx> for ReadVecVisitor<'tcx> {
fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
if let ExprKind::MethodCall(path, receiver, args, _) = e.kind {
let PathSegment { ident, .. } = *path;

match ident.as_str() {
"read" | "read_exact" => {
let [arg] = args else { return };
if 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
&& let Res::Local(res_id) = inner_seg.res
&& self.local_id == res_id
{
self.read_zero_expr = Some(e);
return;
}
},
"resize" => {
// If the Vec is resized, then it's a valid read
if let ExprKind::Path(QPath::Resolved(_, inner_path)) = receiver.kind
&& let Res::Local(res_id) = inner_path.res
&& self.local_id == res_id
{
self.has_resize = true;
return;
}
},
_ => {},
}
}

if !self.has_resize && self.read_zero_expr.is_none() {
walk_expr(self, e);
}
}
}
29 changes: 21 additions & 8 deletions tests/ui/read_zero_byte_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@ fn test() -> io::Result<()> {
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);
Comment on lines -58 to -64
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should lint, but the previous implementation can't catch this


// should not lint
let mut data8 = Vec::new();
data8.resize(100, 0);
Expand All @@ -75,6 +67,27 @@ fn test() -> io::Result<()> {
Ok(())
}

fn test_nested() -> io::Result<()> {
let cap = 1000;
let mut f = File::open("foo.txt").unwrap();

// Issue #9274
// Should not lint
let mut v = Vec::new();
{
v.resize(10, 0);
f.read(&mut v)?;
}

let mut v = Vec::new();
{
f.read(&mut v)?;
//~^ ERROR: reading zero byte data to `Vec`
}

Ok(())
}

async fn test_futures<R: AsyncRead + Unpin>(r: &mut R) {
// should lint
let mut data = Vec::new();
Expand Down
34 changes: 20 additions & 14 deletions tests/ui/read_zero_byte_vec.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:21:5
|
LL | f.read_exact(&mut data).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data.resize(20, 0); f.read_exact(&mut data).unwrap();`
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data.resize(20, 0); f.read_exact(&mut data)`
|
= note: `-D clippy::read-zero-byte-vec` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::read_zero_byte_vec)]`
Expand All @@ -11,19 +11,19 @@ error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:27:5
|
LL | f.read_exact(&mut data2)?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data2.resize(cap, 0); 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:32:5
|
LL | f.read_exact(&mut data3)?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:37:5
--> $DIR/read_zero_byte_vec.rs:37:13
|
LL | let _ = f.read(&mut data4)?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:43:9
Expand All @@ -38,28 +38,34 @@ LL | f.read(&mut data6)
| ^^^^^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:81:5
--> $DIR/read_zero_byte_vec.rs:84:9
|
LL | f.read(&mut v)?;
| ^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:94:5
|
LL | r.read(&mut data).await.unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:86:5
--> $DIR/read_zero_byte_vec.rs:99:5
|
LL | r.read_exact(&mut data2).await.unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:93:5
--> $DIR/read_zero_byte_vec.rs:106:5
|
LL | r.read(&mut data).await.unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:98:5
--> $DIR/read_zero_byte_vec.rs:111:5
|
LL | r.read_exact(&mut data2).await.unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 10 previous errors
error: aborting due to 11 previous errors