From 3ca08959200b69f1736c1af3e5bc944ab43e739b Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 23 Oct 2018 16:01:45 +0900 Subject: [PATCH 1/8] 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 7a91890633a1..eaff87e78f8b 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 000000000000..fc760a3ee29f --- /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 474f16679a72..8941d3031562 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 000000000000..5fd7ffae71b3 --- /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 000000000000..ad84a5754b5a --- /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 + From 5285372f686ed5ff7a84cd0cefb287af6b9c62b7 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Wed, 24 Oct 2018 22:57:31 +0900 Subject: [PATCH 2/8] Run update_lints --- CHANGELOG.md | 1 + README.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ad6514ee10b..5d9d470925fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -813,6 +813,7 @@ All notable changes to this project will be documented in this file. [`range_plus_one`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#range_plus_one [`range_step_by_zero`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#range_step_by_zero [`range_zip_with_len`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#range_zip_with_len +[`redundant_clone`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_clone [`redundant_closure`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_closure [`redundant_closure_call`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_closure_call [`redundant_field_names`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_field_names diff --git a/README.md b/README.md index a13d8ecef666..b4091cdab6c6 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 282 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 283 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: From 105ae712f4f136adb7c94f1cfa35da30bd0e4952 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 14:59:14 +0900 Subject: [PATCH 3/8] update_references indexing_slicing --- tests/ui/indexing_slicing.stderr | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index fafcb1bc4853..14e9627e5734 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -1,3 +1,29 @@ +error: index out of bounds: the len is 4 but the index is 4 + --> $DIR/indexing_slicing.rs:28:5 + | +28 | x[4]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^ + | + = note: #[deny(const_err)] on by default + +error: index out of bounds: the len is 4 but the index is 8 + --> $DIR/indexing_slicing.rs:29:5 + | +29 | x[1 << 3]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^^^^^^ + +error: index out of bounds: the len is 0 but the index is 0 + --> $DIR/indexing_slicing.rs:59:5 + | +59 | empty[0]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^^^^^ + +error: index out of bounds: the len is 4 but the index is 15 + --> $DIR/indexing_slicing.rs:90:5 + | +90 | x[N]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^ + error: indexing may panic. --> $DIR/indexing_slicing.rs:23:5 | @@ -279,5 +305,5 @@ error: range is out of bounds 98 | &x[10..num]; // should trigger out of bounds error | ^^ -error: aborting due to 39 previous errors +error: aborting due to 43 previous errors From 24d3f5b48f177379ba7b8727e5ba9b52b52da2f5 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 20:33:40 +0900 Subject: [PATCH 4/8] Implement visit_basic_block_data --- clippy_lints/src/redundant_clone.rs | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index fc760a3ee29f..fa377dcca67a 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -261,10 +261,31 @@ struct LocalUseVisitor { } 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_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) { + let mir::BasicBlockData { + statements, + terminator, + is_cleanup: _, + } = data; + + for (statement_index, statement) in statements.iter().enumerate() { + self.visit_statement(block, statement, mir::Location { block, statement_index }); + + // Once flagged, skip remaining statements + if self.used_other_than_drop { + return; + } + } + + if let Some(terminator) = terminator { + self.visit_terminator( + block, + terminator, + mir::Location { + block, + statement_index: statements.len(), + }, + ); } } From 9a150b4aa123a6d67fbf8819fe67f2ef1015b726 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 21:08:32 +0900 Subject: [PATCH 5/8] Use lint_root --- clippy_lints/src/redundant_clone.rs | 14 ++++++++++---- clippy_lints/src/utils/mod.rs | 17 +++++++++++++++++ tests/ui/redundant_clone.rs | 3 +++ tests/ui/redundant_clone.stderr | 8 ++++---- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index fa377dcca67a..1a8a62733586 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -23,7 +23,7 @@ use crate::syntax::{ 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, + in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node, span_lint_node_and_then, walk_ptrs_ty_depth, }; use if_chain::if_chain; @@ -87,7 +87,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { 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` + // Looks for `call(x: &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; @@ -225,6 +225,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { if !used_later { let span = terminator.source_info.span; + let node = if let mir::ClearCrossCrate::Set(scope_local_data) = &mir.source_scope_local_data { + scope_local_data[terminator.source_info.scope].lint_root + } else { + unreachable!() + }; + if_chain! { if !in_macro(span); if let Some(snip) = snippet_opt(cx, span); @@ -234,7 +240,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { span.lo() + BytePos(u32::try_from(dot).unwrap()) ); - span_lint_and_then(cx, REDUNDANT_CLONE, sugg_span, "redundant clone", |db| { + span_lint_node_and_then(cx, REDUNDANT_CLONE, node, sugg_span, "redundant clone", |db| { db.span_suggestion_with_applicability( sugg_span, "remove this", @@ -247,7 +253,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { ); }); } else { - span_lint(cx, REDUNDANT_CLONE, span, "redundant clone"); + span_lint_node(cx, REDUNDANT_CLONE, node, span, "redundant clone"); } } } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 05356f8d3856..1a8db837f32b 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -562,6 +562,23 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>( db.docs_link(lint); } +pub fn span_lint_node(cx: &LateContext<'_, '_>, lint: &'static Lint, node: NodeId, sp: Span, msg: &str) { + DiagnosticWrapper(cx.tcx.struct_span_lint_node(lint, node, sp, msg)).docs_link(lint); +} + +pub fn span_lint_node_and_then( + cx: &LateContext<'_, '_>, + lint: &'static Lint, + node: NodeId, + sp: Span, + msg: &str, + f: impl FnOnce(&mut DiagnosticBuilder<'_>), +) { + let mut db = DiagnosticWrapper(cx.tcx.struct_span_lint_node(lint, node, sp, msg)); + f(&mut db.0); + db.docs_link(lint); +} + /// Add a span lint with a suggestion on how to fix it. /// /// These suggestions can be parsed by rustfix to allow it to automatically fix your code. diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index 5fd7ffae71b3..deedde382316 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -31,6 +31,9 @@ fn main() { let _ = OsString::new().to_owned(); let _ = OsString::new().to_os_string(); + + // Check that lint level works + #[allow(clippy::redundant_clone)] let _ = String::new().to_string(); } #[derive(Clone)] diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index ad84a5754b5a..db452822f891 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -96,15 +96,15 @@ note: this value is dropped without further use | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:40:22 + --> $DIR/redundant_clone.rs:43:22 | -40 | (a.clone(), a.clone()) +43 | (a.clone(), a.clone()) | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:40:21 + --> $DIR/redundant_clone.rs:43:21 | -40 | (a.clone(), a.clone()) +43 | (a.clone(), a.clone()) | ^ error: aborting due to 9 previous errors From 6d6ff885852e669a59013629193b74c2458005af Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 22:02:46 +0900 Subject: [PATCH 6/8] Refactor --- clippy_lints/src/redundant_clone.rs | 146 +++++++++++++--------------- 1 file changed, 67 insertions(+), 79 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 1a8a62733586..85f8b525677b 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -29,6 +29,15 @@ use crate::utils::{ use if_chain::if_chain; use std::convert::TryFrom; +macro_rules! unwrap_or_continue { + ($x:expr) => { + match $x { + Some(x) => x, + None => continue, + } + }; +} + /// **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. /// @@ -87,40 +96,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let def_id = cx.tcx.hir.body_owner_def_id(body.id()); let mir = cx.tcx.optimized_mir(def_id); - // Looks for `call(x: &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; - }; + let terminator = unwrap_or_continue!(&bbdata.terminator); // 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 (fn_def_id, arg, arg_ty, _) = unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind)); 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) @@ -135,43 +119,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { 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; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } (from_deref) + // In case of `from_deref`, `arg` is already a reference since it is `deref`ed in the previous + // block. + let cloned = unwrap_or_continue!(find_stmt_assigns_to(arg, from_borrow, bbdata.statements.iter().rev())); // _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); + if ps.len() != 1 { + continue; + } + let pred_terminator = unwrap_or_continue!(&mir[ps[0]].terminator); + 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 let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) = + is_call_with_ref_arg(cx, mir, &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); @@ -182,27 +146,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { } }; - 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; - } + unwrap_or_continue!(find_stmt_assigns_to(pred_arg, true, mir[ps[0]].statements.iter().rev())) } else { cloned }; @@ -261,6 +205,50 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { } } +/// If `kind` is `y = func(x: &T)` where `T: !Copy`, returns `(DefId of func, x, T, y)`. +fn is_call_with_ref_arg<'tcx>( + cx: &LateContext<'_, 'tcx>, + mir: &'tcx mir::Mir<'tcx>, + kind: &'tcx mir::TerminatorKind<'tcx>, +) -> Option<(def_id::DefId, mir::Local, ty::Ty<'tcx>, Option<&'tcx mir::Place<'tcx>>)> { + if_chain! { + if let TerminatorKind::Call { func, args, destination, .. } = 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, destination.as_ref().map(|(dest, _)| dest))) + } else { + None + } + } +} + +/// Finds the first `to = (&)from`, and returns `Some(from)`. +fn find_stmt_assigns_to<'a, 'tcx: 'a>( + to: mir::Local, + by_ref: bool, + mut stmts: impl Iterator>, +) -> Option { + stmts.find_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { + if *local == to { + if by_ref { + if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v { + return Some(r); + } + } else if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v { + return Some(r); + } + } + } + + None + }) +} + struct LocalUseVisitor { local: mir::Local, used_other_than_drop: bool, From a8286927800f2e9f050f0d8c3d6797d5f0885656 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 26 Oct 2018 01:27:28 +0900 Subject: [PATCH 7/8] Use BasicBlockData::terminator --- clippy_lints/src/redundant_clone.rs | 35 +++++++++++------------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 85f8b525677b..85f7bbb637ca 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -97,7 +97,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let mir = cx.tcx.optimized_mir(def_id); for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { - let terminator = unwrap_or_continue!(&bbdata.terminator); + let terminator = bbdata.terminator(); // Give up on loops if terminator.successors().any(|s| *s == bb) { @@ -130,7 +130,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { if ps.len() != 1 { continue; } - let pred_terminator = unwrap_or_continue!(&mir[ps[0]].terminator); + let pred_terminator = mir[ps[0]].terminator(); let pred_arg = if_chain! { if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) = @@ -152,11 +152,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { }; 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; - } + // Give up on loops + if tdata.terminator().successors().any(|s| *s == bb) { + return true; } let mut vis = LocalUseVisitor { @@ -256,12 +254,7 @@ struct LocalUseVisitor { impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) { - let mir::BasicBlockData { - statements, - terminator, - is_cleanup: _, - } = data; - + let statements = &data.statements; for (statement_index, statement) in statements.iter().enumerate() { self.visit_statement(block, statement, mir::Location { block, statement_index }); @@ -271,16 +264,14 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { } } - if let Some(terminator) = terminator { - self.visit_terminator( + self.visit_terminator( + block, + data.terminator(), + mir::Location { block, - terminator, - mir::Location { - block, - statement_index: statements.len(), - }, - ); - } + statement_index: statements.len(), + }, + ); } fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) { From 9034b87a539904c96c01ece3c43878ce25887214 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 26 Oct 2018 03:07:29 +0900 Subject: [PATCH 8/8] Move in_macro check --- clippy_lints/src/redundant_clone.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 85f7bbb637ca..8c8959159217 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -99,6 +99,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { let terminator = bbdata.terminator(); + if in_macro(terminator.source_info.span) { + continue; + } + // Give up on loops if terminator.successors().any(|s| *s == bb) { continue; @@ -174,7 +178,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { }; if_chain! { - if !in_macro(span); if let Some(snip) = snippet_opt(cx, span); if let Some(dot) = snip.rfind('.'); then {