Skip to content

Commit

Permalink
Auto merge of #7011 - Jarcho:redundant_clone_fp, r=flip1995
Browse files Browse the repository at this point in the history 
Fix `redundant_clone` fp

fixes: #5973
fixes: #5595
fixes: #6998

changelog: Fix `redundant_clone` fp  where the cloned value is modified while the clone is in use.
  • Loading branch information
@bors
bors committed Apr 1, 2021
2 parents 38b1fd0 + aaba9b7 commit 92c4fc3
Show file tree
Hide file tree
Showing 4 changed files with 207 additions and 113 deletions.
252 changes: 149 additions & 103 deletions clippy_lints/src/redundant_clone.rs
View file
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
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
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

0 comments on commit 92c4fc3

Please sign in to comment.