Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-10 15:14:29 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge #3355

Browse source 
3355: Lint to remove redundant `clone()`s r=oli-obk a=sinkuu

This PR adds lint `redundant_clone`. It suggests to remove redundant `clone()` that clones a owned value that will be dropped without any usage after that.

Real-world example: https://github.com/rust-lang/rust/compare/7b0735a..sinkuu:redundant_clone2

Co-authored-by: Shotaro Yamada <sinkuu@sinkuu.xyz>
This commit is contained in:
bors[bot] 2018-10-26 10:36:43 +00:00
commit 457e7f12e9
9 changed files with 505 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -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

2
README.md
View file

@ -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:

3
clippy_lints/src/lib.rs
View file

@ -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,
]);

290
clippy_lints/src/redundant_clone.rs Normal file
View file

@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<Item = &'a mir::Statement<'tcx>>,
) -> Option<mir::Local> {
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;
}
}
}

17
clippy_lints/src/utils/mod.rs
View file

@ -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.

8
clippy_lints/src/utils/paths.rs
View file

@ -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", "<impl [T]>", "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"];

28
tests/ui/indexing_slicing.stderr
View file

@ -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

47
tests/ui/redundant_clone.rs Normal file
View file

@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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)
}
}

111
tests/ui/redundant_clone.stderr Normal file
View file

@ -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