mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-09-21 07:01:54 +00:00
Auto merge of #12650 - cocodery:issue/12098, r=xFrednet
fix false positive in Issue/12098 because lack of consideration of mutable caller fixes [Issue#12098](https://github.com/rust-lang/rust-clippy/issues/12098) In issue#12098, the former code doesn't consider the caller for clone is mutable, and suggests to delete clone function. In this change, we first get the inner caller requests for clone, and if it's immutable, the following code will suggest deleting clone. If it's mutable, the loop will check whether a borrow check violation exists, if exists, the lint should not execute, and the function will directly return; otherwise, the following code will handle this. changelog: [`clippy::unnecessary_to_owned`]: fix false positive
This commit is contained in:
commit
5a28d8f01e
4 changed files with 117 additions and 4 deletions
|
@ -3,10 +3,12 @@ use clippy_utils::diagnostics::span_lint_and_then;
|
||||||
use clippy_utils::higher::ForLoop;
|
use clippy_utils::higher::ForLoop;
|
||||||
use clippy_utils::source::snippet_opt;
|
use clippy_utils::source::snippet_opt;
|
||||||
use clippy_utils::ty::{get_iterator_item_ty, implements_trait};
|
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_errors::Applicability;
|
||||||
use rustc_hir::def_id::DefId;
|
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_lint::LateContext;
|
||||||
use rustc_span::{sym, Symbol};
|
use rustc_span::{sym, Symbol};
|
||||||
|
|
||||||
|
@ -40,6 +42,53 @@ pub fn check_for_loop_iter(
|
||||||
&& !clone_or_copy_needed
|
&& !clone_or_copy_needed
|
||||||
&& let Some(receiver_snippet) = snippet_opt(cx, receiver.span)
|
&& 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
|
let snippet = if let ExprKind::MethodCall(maybe_iter_method_name, collection, [], _) = receiver.kind
|
||||||
&& maybe_iter_method_name.ident.name == sym::iter
|
&& maybe_iter_method_name.ident.name == sym::iter
|
||||||
&& let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
|
&& let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
|
||||||
|
|
|
@ -21,6 +21,8 @@ fn main() {
|
||||||
let _ = check_files_ref_mut(&[(FileType::Account, path)]);
|
let _ = check_files_ref_mut(&[(FileType::Account, path)]);
|
||||||
let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
|
let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
|
||||||
let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]);
|
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:
|
// `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<std::path::PathBuf, std::io::Error> {
|
fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
|
||||||
Ok(std::path::PathBuf::new())
|
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<String>,
|
||||||
|
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;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
@ -21,6 +21,8 @@ fn main() {
|
||||||
let _ = check_files_ref_mut(&[(FileType::Account, path)]);
|
let _ = check_files_ref_mut(&[(FileType::Account, path)]);
|
||||||
let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
|
let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
|
||||||
let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]);
|
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:
|
// `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<std::path::PathBuf, std::io::Error> {
|
fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
|
||||||
Ok(std::path::PathBuf::new())
|
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<String>,
|
||||||
|
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;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
@ -1,5 +1,5 @@
|
||||||
error: unnecessary use of `copied`
|
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() {
|
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`
|
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() {
|
LL | for (t, path) in files.iter().copied() {
|
||||||
| ^^^^^^^^^^^^^^^^^^^^^
|
| ^^^^^^^^^^^^^^^^^^^^^
|
||||||
|
|
Loading…
Reference in a new issue