From a8c35cbbda8bc274cd41146a44956fcb691e6277 Mon Sep 17 00:00:00 2001 From: cocodery Date: Tue, 7 May 2024 16:07:13 +0800 Subject: [PATCH] Check inner caller for clone and judge whether they are mutable if immutbale -> lint delete redudant clone if mutable -> lint check whether clone is needed --- .../src/methods/unnecessary_iter_cloned.rs | 53 ++++++++++++++++++- tests/ui/unnecessary_iter_cloned.fixed | 32 +++++++++++ tests/ui/unnecessary_iter_cloned.rs | 32 +++++++++++ tests/ui/unnecessary_iter_cloned.stderr | 4 +- 4 files changed, 117 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs index 520dcb2d5..7431dc1cf 100644 --- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs +++ b/clippy_lints/src/methods/unnecessary_iter_cloned.rs @@ -3,10 +3,12 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::ForLoop; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{get_iterator_item_ty, implements_trait}; -use clippy_utils::{fn_def_id, get_parent_expr}; +use clippy_utils::visitors::for_each_expr; +use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local}; +use core::ops::ControlFlow; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind}; use rustc_lint::LateContext; use rustc_span::{sym, Symbol}; @@ -40,6 +42,53 @@ pub fn check_for_loop_iter( && !clone_or_copy_needed && let Some(receiver_snippet) = snippet_opt(cx, receiver.span) { + // Issue 12098 + // https://github.com/rust-lang/rust-clippy/issues/12098 + // if the assignee have `mut borrow` conflict with the iteratee + // the lint should not execute, former didn't consider the mut case + + // check whether `expr` is mutable + fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if let Some(hir_id) = path_to_local(expr) + && let Node::Pat(pat) = cx.tcx.hir_node(hir_id) + { + matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..)) + } else { + true + } + } + + fn is_caller_or_fields_change(cx: &LateContext<'_>, body: &Expr<'_>, caller: &Expr<'_>) -> bool { + let mut change = false; + if let ExprKind::Block(block, ..) = body.kind { + for_each_expr(block, |e| { + match e.kind { + ExprKind::Assign(assignee, _, _) | ExprKind::AssignOp(_, assignee, _) => { + change |= !can_mut_borrow_both(cx, caller, assignee); + }, + _ => {}, + } + // the return value has no effect but the function need one return value + ControlFlow::<()>::Continue(()) + }); + } + change + } + + if let ExprKind::Call(_, [child, ..]) = expr.kind { + // filter first layer of iterator + let mut child = child; + // get inner real caller requests for clone + while let ExprKind::MethodCall(_, caller, _, _) = child.kind { + child = caller; + } + if is_mutable(cx, child) && is_caller_or_fields_change(cx, body, child) { + // skip lint + return true; + } + }; + + // the lint should not be executed if no violation happens let snippet = if let ExprKind::MethodCall(maybe_iter_method_name, collection, [], _) = receiver.kind && maybe_iter_method_name.ident.name == sym::iter && let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator) diff --git a/tests/ui/unnecessary_iter_cloned.fixed b/tests/ui/unnecessary_iter_cloned.fixed index ad0e5fab0..2c582c90b 100644 --- a/tests/ui/unnecessary_iter_cloned.fixed +++ b/tests/ui/unnecessary_iter_cloned.fixed @@ -21,6 +21,8 @@ fn main() { let _ = check_files_ref_mut(&[(FileType::Account, path)]); let _ = check_files_self_and_arg(&[(FileType::Account, path)]); let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]); + + check_mut_iteratee_and_modify_inner_variable(); } // `check_files` and its variants are based on: @@ -138,3 +140,33 @@ fn check_files_mut_path_buf(files: &[(FileType, std::path::PathBuf)]) -> bool { fn get_file_path(_file_type: &FileType) -> Result { Ok(std::path::PathBuf::new()) } + +// Issue 12098 +// https://github.com/rust-lang/rust-clippy/issues/12098 +// no message emits +fn check_mut_iteratee_and_modify_inner_variable() { + struct Test { + list: Vec, + mut_this: bool, + } + + impl Test { + fn list(&self) -> &[String] { + &self.list + } + } + + let mut test = Test { + list: vec![String::from("foo"), String::from("bar")], + mut_this: false, + }; + + for _item in test.list().to_vec() { + println!("{}", _item); + + test.mut_this = true; + { + test.mut_this = true; + } + } +} diff --git a/tests/ui/unnecessary_iter_cloned.rs b/tests/ui/unnecessary_iter_cloned.rs index d3d59c4c7..a28ccd1ef 100644 --- a/tests/ui/unnecessary_iter_cloned.rs +++ b/tests/ui/unnecessary_iter_cloned.rs @@ -21,6 +21,8 @@ fn main() { let _ = check_files_ref_mut(&[(FileType::Account, path)]); let _ = check_files_self_and_arg(&[(FileType::Account, path)]); let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]); + + check_mut_iteratee_and_modify_inner_variable(); } // `check_files` and its variants are based on: @@ -138,3 +140,33 @@ fn check_files_mut_path_buf(files: &[(FileType, std::path::PathBuf)]) -> bool { fn get_file_path(_file_type: &FileType) -> Result { Ok(std::path::PathBuf::new()) } + +// Issue 12098 +// https://github.com/rust-lang/rust-clippy/issues/12098 +// no message emits +fn check_mut_iteratee_and_modify_inner_variable() { + struct Test { + list: Vec, + mut_this: bool, + } + + impl Test { + fn list(&self) -> &[String] { + &self.list + } + } + + let mut test = Test { + list: vec![String::from("foo"), String::from("bar")], + mut_this: false, + }; + + for _item in test.list().to_vec() { + println!("{}", _item); + + test.mut_this = true; + { + test.mut_this = true; + } + } +} diff --git a/tests/ui/unnecessary_iter_cloned.stderr b/tests/ui/unnecessary_iter_cloned.stderr index 9d3591e0d..fb98cfddc 100644 --- a/tests/ui/unnecessary_iter_cloned.stderr +++ b/tests/ui/unnecessary_iter_cloned.stderr @@ -1,5 +1,5 @@ error: unnecessary use of `copied` - --> tests/ui/unnecessary_iter_cloned.rs:29:22 + --> tests/ui/unnecessary_iter_cloned.rs:31:22 | LL | for (t, path) in files.iter().copied() { | ^^^^^^^^^^^^^^^^^^^^^ @@ -17,7 +17,7 @@ LL + let other = match get_file_path(t) { | error: unnecessary use of `copied` - --> tests/ui/unnecessary_iter_cloned.rs:44:22 + --> tests/ui/unnecessary_iter_cloned.rs:46:22 | LL | for (t, path) in files.iter().copied() { | ^^^^^^^^^^^^^^^^^^^^^