Skip to content

Fix redundant_clone fp #7011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Fix redundant_clone fp #7011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
252 changes: 149 additions & 103 deletions clippy_lints/src/redundant_clone.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,79 +199,72 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone {
(local, deref_clone_ret)
};

let is_temp = mir.local_kind(ret_local) == mir::LocalKind::Temp;

// 1. `local` can be moved out if it is not used later.
// 2. If `ret_local` is a temporary and is neither consumed nor mutated, we can remove this `clone`
// call anyway.
let (used, consumed_or_mutated) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold(
(false, !is_temp),
|(used, consumed), (tbb, tdata)| {
// Short-circuit
if (used && consumed) ||
// Give up on loops
tdata.terminator().successors().any(|s| *s == bb)
{
return (true, true);
let clone_usage = if local == ret_local {
CloneUsage {
cloned_used: false,
cloned_consume_or_mutate_loc: None,
clone_consumed_or_mutated: true,
}
} else {
let clone_usage = visit_clone_usage(local, ret_local, &mir, bb);
if clone_usage.cloned_used && clone_usage.clone_consumed_or_mutated {
// cloned value is used, and the clone is modified or moved
continue;
} else if let Some(loc) = clone_usage.cloned_consume_or_mutate_loc {
// cloned value is mutated, and the clone is alive.
if possible_borrower.is_alive_at(ret_local, loc) {
continue;
}
}
clone_usage
};

let mut vis = LocalUseVisitor {
used: (local, false),
consumed_or_mutated: (ret_local, false),
};
vis.visit_basic_block_data(tbb, tdata);
(used || vis.used.1, consumed || vis.consumed_or_mutated.1)
},
);

if !used || !consumed_or_mutated {
let span = terminator.source_info.span;
let scope = terminator.source_info.scope;
let node = mir.source_scopes[scope]
.local_data
.as_ref()
.assert_crate_local()
.lint_root;

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())
);
let mut app = Applicability::MaybeIncorrect;

let call_snip = &snip[dot + 1..];
// Machine applicable when `call_snip` looks like `foobar()`
if let Some(call_snip) = call_snip.strip_suffix("()").map(str::trim) {
if call_snip.as_bytes().iter().all(|b| b.is_ascii_alphabetic() || *b == b'_') {
app = Applicability::MachineApplicable;
}
let span = terminator.source_info.span;
let scope = terminator.source_info.scope;
let node = mir.source_scopes[scope]
.local_data
.as_ref()
.assert_crate_local()
.lint_root;

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())
);
let mut app = Applicability::MaybeIncorrect;

let call_snip = &snip[dot + 1..];
// Machine applicable when `call_snip` looks like `foobar()`
if let Some(call_snip) = call_snip.strip_suffix("()").map(str::trim) {
if call_snip.as_bytes().iter().all(|b| b.is_ascii_alphabetic() || *b == b'_') {
app = Applicability::MachineApplicable;
}
}

span_lint_hir_and_then(cx, REDUNDANT_CLONE, node, sugg_span, "redundant clone", |diag| {
diag.span_suggestion(
sugg_span,
"remove this",
String::new(),
app,
span_lint_hir_and_then(cx, REDUNDANT_CLONE, node, sugg_span, "redundant clone", |diag| {
diag.span_suggestion(
sugg_span,
"remove this",
String::new(),
app,
);
if clone_usage.cloned_used {
diag.span_note(
span,
"cloned value is neither consumed nor mutated",
);
if used {
diag.span_note(
span,
"cloned value is neither consumed nor mutated",
);
} else {
diag.span_note(
span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())),
"this value is dropped without further use",
);
}
});
} else {
span_lint_hir(cx, REDUNDANT_CLONE, node, span, "redundant clone");
}
} else {
diag.span_note(
span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())),
"this value is dropped without further use",
);
}
});
} else {
span_lint_hir(cx, REDUNDANT_CLONE, node, span, "redundant clone");
}
}
}
Expand Down Expand Up @@ -365,49 +358,97 @@ fn base_local_and_movability<'tcx>(
(local, deref || field || slice)
}

struct LocalUseVisitor {
used: (mir::Local, bool),
consumed_or_mutated: (mir::Local, bool),
#[derive(Default)]
struct CloneUsage {
/// Whether the cloned value is used after the clone.
cloned_used: bool,
/// The first location where the cloned value is consumed or mutated, if any.
cloned_consume_or_mutate_loc: Option<mir::Location>,
/// Whether the clone value is mutated.
clone_consumed_or_mutated: 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(statement, mir::Location { block, statement_index });
}

self.visit_terminator(
data.terminator(),
mir::Location {
block,
statement_index: statements.len(),
},
);
fn visit_clone_usage(cloned: mir::Local, clone: mir::Local, mir: &mir::Body<'_>, bb: mir::BasicBlock) -> CloneUsage {
struct V {
cloned: mir::Local,
clone: mir::Local,
result: CloneUsage,
}
impl<'tcx> mir::visit::Visitor<'tcx> for V {
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(statement, mir::Location { block, statement_index });
}

fn visit_place(&mut self, place: &mir::Place<'tcx>, ctx: PlaceContext, _: mir::Location) {
let local = place.local;

if local == self.used.0
&& !matches!(
ctx,
PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_)
)
{
self.used.1 = true;
self.visit_terminator(
data.terminator(),
mir::Location {
block,
statement_index: statements.len(),
},
);
}

if local == self.consumed_or_mutated.0 {
match ctx {
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)
| PlaceContext::MutatingUse(MutatingUseContext::Borrow) => {
self.consumed_or_mutated.1 = true;
},
_ => {},
fn visit_place(&mut self, place: &mir::Place<'tcx>, ctx: PlaceContext, loc: mir::Location) {
let local = place.local;

if local == self.cloned
&& !matches!(
ctx,
PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_)
)
{
self.result.cloned_used = true;
self.result.cloned_consume_or_mutate_loc = self.result.cloned_consume_or_mutate_loc.or_else(|| {
matches!(
ctx,
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)
| PlaceContext::MutatingUse(MutatingUseContext::Borrow)
)
.then(|| loc)
});
} else if local == self.clone {
match ctx {
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)
| PlaceContext::MutatingUse(MutatingUseContext::Borrow) => {
self.result.clone_consumed_or_mutated = true;
},
_ => {},
}
}
}
}

let init = CloneUsage {
cloned_used: false,
cloned_consume_or_mutate_loc: None,
// Consider non-temporary clones consumed.
// TODO: Actually check for mutation of non-temporaries.
clone_consumed_or_mutated: mir.local_kind(clone) != mir::LocalKind::Temp,
};
traversal::ReversePostorder::new(&mir, bb)
.skip(1)
.fold(init, |usage, (tbb, tdata)| {
// Short-circuit
if (usage.cloned_used && usage.clone_consumed_or_mutated) ||
// Give up on loops
tdata.terminator().successors().any(|s| *s == bb)
{
return CloneUsage {
cloned_used: true,
clone_consumed_or_mutated: true,
..usage
};
}

let mut v = V {
cloned,
clone,
result: usage,
};
v.visit_basic_block_data(tbb, tdata);
v.result
})
}

/// Determines liveness of each local purely based on `StorageLive`/`Dead`.
Expand Down Expand Up @@ -623,4 +664,9 @@ impl PossibleBorrowerMap<'_, '_> {

self.bitset.0 == self.bitset.1
}

fn is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool {
self.maybe_live.seek_after_primary_effect(at);
self.maybe_live.contains(local)
}
}
24 changes: 24 additions & 0 deletions tests/ui/redundant_clone.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ fn main() {
not_consumed();
issue_5405();
manually_drop();
clone_then_move_cloned();
}

#[derive(Clone)]
Expand Down Expand Up @@ -182,3 +183,26 @@ fn manually_drop() {
Arc::from_raw(p);
}
}

fn clone_then_move_cloned() {
// issue #5973
let x = Some(String::new());
// ok, x is moved while the clone is in use.
assert_eq!(x.clone(), None, "not equal {}", x.unwrap());

// issue #5595
fn foo<F: Fn()>(_: &Alpha, _: F) {}
let x = Alpha;
// ok, data is moved while the clone is in use.
foo(&x.clone(), move || {
let _ = x;
});

// issue #6998
struct S(String);
impl S {
fn m(&mut self) {}
}
let mut x = S(String::new());
x.0.clone().chars().for_each(|_| x.m());
}
24 changes: 24 additions & 0 deletions tests/ui/redundant_clone.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ fn main() {
not_consumed();
issue_5405();
manually_drop();
clone_then_move_cloned();
}

#[derive(Clone)]
Expand Down Expand Up @@ -182,3 +183,26 @@ fn manually_drop() {
Arc::from_raw(p);
}
}

fn clone_then_move_cloned() {
// issue #5973
let x = Some(String::new());
// ok, x is moved while the clone is in use.
assert_eq!(x.clone(), None, "not equal {}", x.unwrap());

// issue #5595
fn foo<F: Fn()>(_: &Alpha, _: F) {}
let x = Alpha;
// ok, data is moved while the clone is in use.
foo(&x.clone(), move || {
let _ = x;
});

// issue #6998
struct S(String);
impl S {
fn m(&mut self) {}
}
let mut x = S(String::new());
x.0.clone().chars().for_each(|_| x.m());
}
Loading