From 3ca08959200b69f1736c1af3e5bc944ab43e739b Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 23 Oct 2018 16:01:45 +0900 Subject: [PATCH] Add redundant_clone lint --- clippy_lints/src/lib.rs | 3 + clippy_lints/src/redundant_clone.rs | 281 ++++++++++++++++++++++++++++ clippy_lints/src/utils/paths.rs | 8 + tests/ui/redundant_clone.rs | 44 +++++ tests/ui/redundant_clone.stderr | 111 +++++++++++ 5 files changed, 447 insertions(+) create mode 100644 clippy_lints/src/redundant_clone.rs create mode 100644 tests/ui/redundant_clone.rs create mode 100644 tests/ui/redundant_clone.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7a91890633a..eaff87e78f8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -179,6 +179,7 @@ pub mod ptr; pub mod ptr_offset_with_cast; pub mod question_mark; pub mod ranges; +pub mod redundant_clone; pub mod redundant_field_names; pub mod redundant_pattern_matching; pub mod reference; @@ -452,6 +453,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box indexing_slicing::IndexingSlicing); reg.register_late_lint_pass(box non_copy_const::NonCopyConst); reg.register_late_lint_pass(box ptr_offset_with_cast::Pass); + reg.register_late_lint_pass(box redundant_clone::RedundantClone); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -981,6 +983,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { fallible_impl_from::FALLIBLE_IMPL_FROM, mutex_atomic::MUTEX_INTEGER, needless_borrow::NEEDLESS_BORROW, + redundant_clone::REDUNDANT_CLONE, unwrap::PANICKING_UNWRAP, unwrap::UNNECESSARY_UNWRAP, ]); diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs new file mode 100644 index 00000000000..fc760a3ee29 --- /dev/null +++ b/clippy_lints/src/redundant_clone.rs @@ -0,0 +1,281 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use crate::rustc::hir::intravisit::FnKind; +use crate::rustc::hir::{def_id, Body, FnDecl}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::mir::{ + self, traversal, + visit::{PlaceContext, Visitor}, + TerminatorKind, +}; +use crate::rustc::ty; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::rustc_errors::Applicability; +use crate::syntax::{ + ast::NodeId, + source_map::{BytePos, Span}, +}; +use crate::utils::{ + in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint, span_lint_and_then, + walk_ptrs_ty_depth, +}; +use if_chain::if_chain; +use std::convert::TryFrom; + +/// **What it does:** Checks for a redudant `clone()` (and its relatives) which clones an owned +/// value that is going to be dropped without further use. +/// +/// **Why is this bad?** It is not always possible for the compiler to eliminate useless +/// allocations and deallocations generated by redundant `clone()`s. +/// +/// **Known problems:** +/// +/// * Suggestions made by this lint could require NLL to be enabled. +/// * False-positive if there is a borrow preventing the value from moving out. +/// +/// ```rust +/// let x = String::new(); +/// +/// let y = &x; +/// +/// foo(x.clone()); // This lint suggests to remove this `clone()` +/// ``` +/// +/// **Example:** +/// ```rust +/// { +/// let x = Foo::new(); +/// call(x.clone()); +/// call(x.clone()); // this can just pass `x` +/// } +/// +/// ["lorem", "ipsum"].join(" ").to_string() +/// +/// Path::new("/a/b").join("c").to_path_buf() +/// ``` +declare_clippy_lint! { + pub REDUNDANT_CLONE, + nursery, + "`clone()` of an owned value that is going to be dropped immediately" +} + +pub struct RedundantClone; + +impl LintPass for RedundantClone { + fn get_lints(&self) -> LintArray { + lint_array!(REDUNDANT_CLONE) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { + fn check_fn( + &mut self, + cx: &LateContext<'a, 'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl, + body: &'tcx Body, + _: Span, + _: NodeId, + ) { + let def_id = cx.tcx.hir.body_owner_def_id(body.id()); + let mir = cx.tcx.optimized_mir(def_id); + + // Looks for `call(&T)` where `T: !Copy` + let call = |kind: &mir::TerminatorKind<'tcx>| -> Option<(def_id::DefId, mir::Local, ty::Ty<'tcx>)> { + if_chain! { + if let TerminatorKind::Call { func, args, .. } = kind; + if args.len() == 1; + if let mir::Operand::Move(mir::Place::Local(local)) = &args[0]; + if let ty::FnDef(def_id, _) = func.ty(&*mir, cx.tcx).sty; + if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx)); + if !is_copy(cx, inner_ty); + then { + Some((def_id, *local, inner_ty)) + } else { + None + } + } + }; + + for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { + let terminator = if let Some(terminator) = &bbdata.terminator { + terminator + } else { + continue; + }; + + // Give up on loops + if terminator.successors().any(|s| *s == bb) { + continue; + } + + let (fn_def_id, arg, arg_ty) = if let Some(t) = call(&terminator.kind) { + t + } else { + continue; + }; + + let from_borrow = match_def_path(cx.tcx, fn_def_id, &paths::CLONE_TRAIT_METHOD) + || match_def_path(cx.tcx, fn_def_id, &paths::TO_OWNED_METHOD) + || (match_def_path(cx.tcx, fn_def_id, &paths::TO_STRING_METHOD) + && match_type(cx, arg_ty, &paths::STRING)); + + let from_deref = !from_borrow + && (match_def_path(cx.tcx, fn_def_id, &paths::PATH_TO_PATH_BUF) + || match_def_path(cx.tcx, fn_def_id, &paths::OS_STR_TO_OS_STRING)); + + if !from_borrow && !from_deref { + continue; + } + + // _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } + let cloned = if let Some(referent) = bbdata + .statements + .iter() + .rev() + .filter_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { + if *local == arg { + if from_deref { + // `r` is already a reference. + if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v { + return Some(r); + } + } else if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v { + return Some(r); + } + } + } + + None + }) + .next() + { + referent + } else { + continue; + }; + + // _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }` + let referent = if from_deref { + let ps = mir.predecessors_for(bb); + let pred_arg = if_chain! { + if ps.len() == 1; + if let Some(pred_terminator) = &mir[ps[0]].terminator; + if let mir::TerminatorKind::Call { destination: Some((res, _)), .. } = &pred_terminator.kind; + if *res == mir::Place::Local(cloned); + if let Some((pred_fn_def_id, pred_arg, pred_arg_ty)) = call(&pred_terminator.kind); + if match_def_path(cx.tcx, pred_fn_def_id, &paths::DEREF_TRAIT_METHOD); + if match_type(cx, pred_arg_ty, &paths::PATH_BUF) + || match_type(cx, pred_arg_ty, &paths::OS_STRING); + then { + pred_arg + } else { + continue; + } + }; + + if let Some(referent) = mir[ps[0]] + .statements + .iter() + .rev() + .filter_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(l), v) = &stmt.kind { + if *l == pred_arg { + if let mir::Rvalue::Ref(_, _, mir::Place::Local(referent)) = **v { + return Some(referent); + } + } + } + + None + }) + .next() + { + referent + } else { + continue; + } + } else { + cloned + }; + + let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| { + if let Some(term) = &tdata.terminator { + // Give up on loops + if term.successors().any(|s| *s == bb) { + return true; + } + } + + let mut vis = LocalUseVisitor { + local: referent, + used_other_than_drop: false, + }; + vis.visit_basic_block_data(tbb, tdata); + vis.used_other_than_drop + }); + + if !used_later { + let span = terminator.source_info.span; + if_chain! { + if !in_macro(span); + if let Some(snip) = snippet_opt(cx, span); + if let Some(dot) = snip.rfind('.'); + then { + let sugg_span = span.with_lo( + span.lo() + BytePos(u32::try_from(dot).unwrap()) + ); + + span_lint_and_then(cx, REDUNDANT_CLONE, sugg_span, "redundant clone", |db| { + db.span_suggestion_with_applicability( + sugg_span, + "remove this", + String::new(), + Applicability::MaybeIncorrect, + ); + db.span_note( + span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())), + "this value is dropped without further use", + ); + }); + } else { + span_lint(cx, REDUNDANT_CLONE, span, "redundant clone"); + } + } + } + } + } +} + +struct LocalUseVisitor { + local: mir::Local, + used_other_than_drop: bool, +} + +impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { + fn visit_statement(&mut self, block: mir::BasicBlock, statement: &mir::Statement<'tcx>, location: mir::Location) { + // Once flagged, skip remaining statements + if !self.used_other_than_drop { + self.super_statement(block, statement, location); + } + } + + fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) { + match ctx { + PlaceContext::Drop | PlaceContext::StorageDead => return, + _ => {}, + } + + if *local == self.local { + self.used_other_than_drop = true; + } + } +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 474f16679a7..8941d303156 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -23,6 +23,7 @@ pub const BTREEMAP: [&str; 5] = ["alloc", "collections", "btree", "map", "BTreeM pub const BTREEMAP_ENTRY: [&str; 5] = ["alloc", "collections", "btree", "map", "Entry"]; pub const BTREESET: [&str; 5] = ["alloc", "collections", "btree", "set", "BTreeSet"]; pub const CLONE_TRAIT: [&str; 3] = ["core", "clone", "Clone"]; +pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"]; pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"]; pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"]; @@ -31,6 +32,7 @@ pub const C_VOID: [&str; 3] = ["core", "ffi", "c_void"]; pub const C_VOID_LIBC: [&str; 2] = ["libc", "c_void"]; pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"]; pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"]; +pub const DEREF_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "Deref", "deref"]; pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"]; pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"]; pub const DROP: [&str; 3] = ["core", "mem", "drop"]; @@ -67,7 +69,11 @@ pub const OPTION: [&str; 3] = ["core", "option", "Option"]; pub const OPTION_NONE: [&str; 4] = ["core", "option", "Option", "None"]; pub const OPTION_SOME: [&str; 4] = ["core", "option", "Option", "Some"]; pub const ORD: [&str; 3] = ["core", "cmp", "Ord"]; +pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"]; +pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"]; pub const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"]; +pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"]; +pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; pub const PTR_NULL: [&str; 2] = ["ptr", "null"]; pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"]; pub const RANGE: [&str; 3] = ["core", "ops", "Range"]; @@ -100,7 +106,9 @@ pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "", "into_vec pub const SLICE_ITER: [&str; 3] = ["core", "slice", "Iter"]; pub const STRING: [&str; 3] = ["alloc", "string", "String"]; pub const TO_OWNED: [&str; 3] = ["alloc", "borrow", "ToOwned"]; +pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"]; pub const TO_STRING: [&str; 3] = ["alloc", "string", "ToString"]; +pub const TO_STRING_METHOD: [&str; 4] = ["alloc", "string", "ToString", "to_string"]; pub const TRANSMUTE: [&str; 4] = ["core", "intrinsics", "", "transmute"]; pub const TRY_INTO_RESULT: [&str; 4] = ["std", "ops", "Try", "into_result"]; pub const UNINIT: [&str; 4] = ["core", "intrinsics", "", "uninit"]; diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs new file mode 100644 index 00000000000..5fd7ffae71b --- /dev/null +++ b/tests/ui/redundant_clone.rs @@ -0,0 +1,44 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![warn(clippy::redundant_clone)] + +use std::path::Path; +use std::ffi::OsString; + +fn main() { + let _ = ["lorem", "ipsum"].join(" ").to_string(); + + let s = String::from("foo"); + let _ = s.clone(); + + let s = String::from("foo"); + let _ = s.to_string(); + + let s = String::from("foo"); + let _ = s.to_owned(); + + let _ = Path::new("/a/b/").join("c").to_owned(); + + let _ = Path::new("/a/b/").join("c").to_path_buf(); + + let _ = OsString::new().to_owned(); + + let _ = OsString::new().to_os_string(); +} + +#[derive(Clone)] +struct Alpha; +fn double(a: Alpha) -> (Alpha, Alpha) { + if true { + (a.clone(), a.clone()) + } else { + (Alpha, a) + } +} diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr new file mode 100644 index 00000000000..ad84a5754b5 --- /dev/null +++ b/tests/ui/redundant_clone.stderr @@ -0,0 +1,111 @@ +error: redundant clone + --> $DIR/redundant_clone.rs:16:41 + | +16 | let _ = ["lorem", "ipsum"].join(" ").to_string(); + | ^^^^^^^^^^^^ help: remove this + | + = note: `-D clippy::redundant-clone` implied by `-D warnings` +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:16:13 + | +16 | let _ = ["lorem", "ipsum"].join(" ").to_string(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:19:14 + | +19 | let _ = s.clone(); + | ^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:19:13 + | +19 | let _ = s.clone(); + | ^ + +error: redundant clone + --> $DIR/redundant_clone.rs:22:14 + | +22 | let _ = s.to_string(); + | ^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:22:13 + | +22 | let _ = s.to_string(); + | ^ + +error: redundant clone + --> $DIR/redundant_clone.rs:25:14 + | +25 | let _ = s.to_owned(); + | ^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:25:13 + | +25 | let _ = s.to_owned(); + | ^ + +error: redundant clone + --> $DIR/redundant_clone.rs:27:41 + | +27 | let _ = Path::new("/a/b/").join("c").to_owned(); + | ^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:27:13 + | +27 | let _ = Path::new("/a/b/").join("c").to_owned(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:29:41 + | +29 | let _ = Path::new("/a/b/").join("c").to_path_buf(); + | ^^^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:29:13 + | +29 | let _ = Path::new("/a/b/").join("c").to_path_buf(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:31:28 + | +31 | let _ = OsString::new().to_owned(); + | ^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:31:13 + | +31 | let _ = OsString::new().to_owned(); + | ^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:33:28 + | +33 | let _ = OsString::new().to_os_string(); + | ^^^^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:33:13 + | +33 | let _ = OsString::new().to_os_string(); + | ^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:40:22 + | +40 | (a.clone(), a.clone()) + | ^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:40:21 + | +40 | (a.clone(), a.clone()) + | ^ + +error: aborting due to 9 previous errors +