Skip to content

Commit

Permalink
Auto merge of rust-lang#8201 - smoelius:master, r=camsteffen
Browse files Browse the repository at this point in the history
Change `unnecessary_to_owned` `into_iter` suggestions to `MaybeIncorrect`

I am having a hard time finding a good solution for rust-lang#8148, so I am wondering if is enough to just change the suggestion's applicability to `MaybeIncorrect`?

I apologize, as I realize this is a bit of a cop out.

changelog: none
  • Loading branch information
bors committed Jan 8, 2022
2 parents be7cf76 + 366234a commit 917890b
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 5 deletions.
5 changes: 5 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1895,6 +1895,11 @@ declare_clippy_lint! {
/// ### Why is this bad?
/// The unnecessary calls result in useless allocations.
///
/// ### Known problems
/// `unnecessary_to_owned` can falsely trigger if `IntoIterator::into_iter` is applied to an
/// owned copy of a resource and the resource is later used mutably. See
/// [#8148](https://github.com/rust-lang/rust-clippy/issues/8148).
///
/// ### Example
/// ```rust
/// let path = std::path::Path::new("x");
Expand Down
17 changes: 14 additions & 3 deletions clippy_lints/src/methods/unnecessary_iter_cloned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol
if let Some(callee_def_id) = fn_def_id(cx, parent);
if is_into_iter(cx, callee_def_id);
then {
check_for_loop_iter(cx, parent, method_name, receiver)
check_for_loop_iter(cx, parent, method_name, receiver, false)
} else {
false
}
Expand All @@ -34,6 +34,7 @@ pub fn check_for_loop_iter(
expr: &'tcx Expr<'tcx>,
method_name: Symbol,
receiver: &'tcx Expr<'tcx>,
cloned_before_iter: bool,
) -> bool {
if_chain! {
if let Some(grandparent) = get_parent_expr(cx, expr).and_then(|parent| get_parent_expr(cx, parent));
Expand Down Expand Up @@ -70,12 +71,22 @@ pub fn check_for_loop_iter(
expr.span,
&format!("unnecessary use of `{}`", method_name),
|diag| {
diag.span_suggestion(expr.span, "use", snippet, Applicability::MachineApplicable);
// If `check_into_iter_call_arg` called `check_for_loop_iter` because a call to
// a `to_owned`-like function was removed, then the next suggestion may be
// incorrect. This is because the iterator that results from the call's removal
// could hold a reference to a resource that is used mutably. See
// https://github.com/rust-lang/rust-clippy/issues/8148.
let applicability = if cloned_before_iter {
Applicability::MaybeIncorrect
} else {
Applicability::MachineApplicable
};
diag.span_suggestion(expr.span, "use", snippet, applicability);
for addr_of_expr in addr_of_exprs {
match addr_of_expr.kind {
ExprKind::AddrOf(_, _, referent) => {
let span = addr_of_expr.span.with_hi(referent.span.lo());
diag.span_suggestion(span, "remove this `&`", String::new(), Applicability::MachineApplicable);
diag.span_suggestion(span, "remove this `&`", String::new(), applicability);
}
_ => unreachable!(),
}
Expand Down
13 changes: 11 additions & 2 deletions clippy_lints/src/methods/unnecessary_to_owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,22 +187,31 @@ fn check_into_iter_call_arg(
if let Some(item_ty) = get_iterator_item_ty(cx, parent_ty);
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
then {
if unnecessary_iter_cloned::check_for_loop_iter(cx, parent, method_name, receiver) {
if unnecessary_iter_cloned::check_for_loop_iter(
cx,
parent,
method_name,
receiver,
true,
) {
return true;
}
let cloned_or_copied = if is_copy(cx, item_ty) {
"copied"
} else {
"cloned"
};
// The next suggestion may be incorrect because the removal of the `to_owned`-like
// function could cause the iterator to hold a reference to a resource that is used
// mutably. See https://github.com/rust-lang/rust-clippy/issues/8148.
span_lint_and_sugg(
cx,
UNNECESSARY_TO_OWNED,
parent.span,
&format!("unnecessary use of `{}`", method_name),
"use",
format!("{}.iter().{}()", receiver_snippet, cloned_or_copied),
Applicability::MachineApplicable,
Applicability::MaybeIncorrect,
);
return true;
}
Expand Down

0 comments on commit 917890b

Please sign in to comment.