diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ad6514ee..5d9d47092 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 a13d8ecef..b4091cdab 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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7a9189063..eaff87e78 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 000000000..8c8959159 --- /dev/null +++ b/clippy_lints/src/redundant_clone.rs @@ -0,0 +1,290 @@ +// 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_node, span_lint_node_and_then, + walk_ptrs_ty_depth, +}; +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. +/// +/// **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); + + 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; + } + + 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) + || (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); } (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 = mir[ps[0]].terminator(); + + let pred_arg = if_chain! { + 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 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; + } + }; + + unwrap_or_continue!(find_stmt_assigns_to(pred_arg, true, mir[ps[0]].statements.iter().rev())) + } else { + cloned + }; + + let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| { + // Give up on loops + if tdata.terminator().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; + 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 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_node_and_then(cx, REDUNDANT_CLONE, node, 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_node(cx, REDUNDANT_CLONE, node, span, "redundant clone"); + } + } + } + } + } +} + +/// 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, +} + +impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { + fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) { + let statements = &data.statements; + 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; + } + } + + self.visit_terminator( + block, + data.terminator(), + mir::Location { + block, + statement_index: statements.len(), + }, + ); + } + + 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/mod.rs b/clippy_lints/src/utils/mod.rs index 05356f8d3..1a8db837f 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/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 474f16679..8941d3031 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/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index fafcb1bc4..14e9627e5 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 diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs new file mode 100644 index 000000000..deedde382 --- /dev/null +++ b/tests/ui/redundant_clone.rs @@ -0,0 +1,47 @@ +// 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(); + + // Check that lint level works + #[allow(clippy::redundant_clone)] let _ = String::new().to_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 000000000..db452822f --- /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:43:22 + | +43 | (a.clone(), a.clone()) + | ^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:43:21 + | +43 | (a.clone(), a.clone()) + | ^ + +error: aborting due to 9 previous errors +