From 63030acf4ff453da2beb7f43b13cca8fb6cf425a Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Sun, 12 Mar 2023 14:45:22 +0000 Subject: [PATCH 1/3] Refactor --- clippy_lints/src/collection_is_never_read.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs index 15a5b7dd2748..a5feffa31065 100644 --- a/clippy_lints/src/collection_is_never_read.rs +++ b/clippy_lints/src/collection_is_never_read.rs @@ -101,9 +101,9 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc return ControlFlow::Continue(()); } - // Method call on `id` in a statement ignores any return value, so it's not a read access: + // Look for method call with receiver `id`. It might be a non-read access: // - // id.foo(...); // Not reading `id`. + // id.foo(args) // // Only assuming this for "official" methods defined on the type. For methods defined in extension // traits (identified as local, based on the orphan rule), pessimistically assume that they might @@ -111,11 +111,15 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, expr.hir_id) && let ExprKind::MethodCall(_, receiver, _, _) = parent.kind && path_to_local_id(receiver, id) - && let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id) && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(parent.hir_id) && !method_def_id.is_local() { - return ControlFlow::Continue(()); + // The method call is a statement, so the return value is not used. That's not a read access: + // + // id.foo(args); + if let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id) { + return ControlFlow::Continue(()); + } } // Any other access to `id` is a read access. Stop searching. From 008ba7326b315abd1d6ef693d773b88ccb044ba4 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Sun, 12 Mar 2023 14:56:14 +0000 Subject: [PATCH 2/3] Ignore fake read access --- clippy_lints/src/collection_is_never_read.rs | 9 +++++++++ tests/ui/collection_is_never_read.rs | 3 +-- tests/ui/collection_is_never_read.stderr | 20 +++++++++++++------- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs index a5feffa31065..5e2eb5789f62 100644 --- a/clippy_lints/src/collection_is_never_read.rs +++ b/clippy_lints/src/collection_is_never_read.rs @@ -120,6 +120,15 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc if let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id) { return ControlFlow::Continue(()); } + + // The method call is not a statement, so its return value is used somehow but its type is the + // unit type, so this is not a real read access. Examples: + // + // let y = x.clear(); + // println!("{:?}", x.clear()); + if cx.typeck_results().expr_ty(parent).is_unit() { + return ControlFlow::Continue(()); + } } // Any other access to `id` is a read access. Stop searching. diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs index ca20031bfbef..4f9431be72a7 100644 --- a/tests/ui/collection_is_never_read.rs +++ b/tests/ui/collection_is_never_read.rs @@ -85,9 +85,8 @@ fn shadowing_2() { #[allow(clippy::let_unit_value)] fn fake_read() { - let mut x = vec![1, 2, 3]; // Ok + let mut x = vec![1, 2, 3]; // WARNING x.reverse(); - // `collection_is_never_read` gets fooled, but other lints should catch this. let _: () = x.clear(); } diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr index f5dea96116f8..cfb392983391 100644 --- a/tests/ui/collection_is_never_read.stderr +++ b/tests/ui/collection_is_never_read.stderr @@ -25,40 +25,46 @@ LL | let mut x = HashMap::new(); // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:95:5 + --> $DIR/collection_is_never_read.rs:88:5 | LL | let mut x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:102:5 + --> $DIR/collection_is_never_read.rs:94:5 | LL | let mut x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:119:5 + --> $DIR/collection_is_never_read.rs:101:5 + | +LL | let mut x = vec![1, 2, 3]; // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: collection is never read + --> $DIR/collection_is_never_read.rs:118:5 | LL | let mut x = HashSet::new(); // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:133:5 + --> $DIR/collection_is_never_read.rs:132:5 | LL | let x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:169:5 + --> $DIR/collection_is_never_read.rs:168:5 | LL | let mut s = String::new(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:182:5 + --> $DIR/collection_is_never_read.rs:181:5 | LL | let mut s = String::from("Hello, World!"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 10 previous errors +error: aborting due to 11 previous errors From 3d711455c21fca69cad5de32385da90a4902c67c Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Sun, 12 Mar 2023 14:59:10 +0000 Subject: [PATCH 3/3] Add test case from #10488 --- tests/ui/collection_is_never_read.rs | 8 +++++++- tests/ui/collection_is_never_read.stderr | 18 ++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs index 4f9431be72a7..01259a983ab6 100644 --- a/tests/ui/collection_is_never_read.rs +++ b/tests/ui/collection_is_never_read.rs @@ -84,12 +84,18 @@ fn shadowing_2() { } #[allow(clippy::let_unit_value)] -fn fake_read() { +fn fake_read_1() { let mut x = vec![1, 2, 3]; // WARNING x.reverse(); let _: () = x.clear(); } +fn fake_read_2() { + let mut x = vec![1, 2, 3]; // WARNING + x.reverse(); + println!("{:?}", x.push(5)); +} + fn assignment() { let mut x = vec![1, 2, 3]; // WARNING let y = vec![4, 5, 6]; // Ok diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr index cfb392983391..cf51a53686f2 100644 --- a/tests/ui/collection_is_never_read.stderr +++ b/tests/ui/collection_is_never_read.stderr @@ -37,34 +37,40 @@ LL | let mut x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:101:5 + --> $DIR/collection_is_never_read.rs:100:5 | LL | let mut x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:118:5 + --> $DIR/collection_is_never_read.rs:107:5 + | +LL | let mut x = vec![1, 2, 3]; // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: collection is never read + --> $DIR/collection_is_never_read.rs:124:5 | LL | let mut x = HashSet::new(); // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:132:5 + --> $DIR/collection_is_never_read.rs:138:5 | LL | let x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:168:5 + --> $DIR/collection_is_never_read.rs:174:5 | LL | let mut s = String::new(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: collection is never read - --> $DIR/collection_is_never_read.rs:181:5 + --> $DIR/collection_is_never_read.rs:187:5 | LL | let mut s = String::from("Hello, World!"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 11 previous errors +error: aborting due to 12 previous errors