Skip to content

Commit 40fd785

Browse files
committed
Auto merge of #7978 - smoelius:master, r=llogiq
Add `unnecessary_to_owned` lint This PR adds a lint to check for unnecessary calls to `ToOwned::to_owned` and other similar functions (e.g., `Cow::into_owned`, `ToString::to_string`, etc.). The lint checks for expressions of the form `&receiver.to_owned_like()` used in a position requiring type `&T` where one of the following is true: * `receiver`'s type is `T` exactly * `receiver`'s type implements `Deref<Target = T>` * `receiver`'s type implements `AsRef<T>` The lint additionally checks for expressions of the form `receiver.to_owned_like()` used as arguments of type `impl AsRef<T>`. It would be nice if the lint could also check for expressions used as arguments to functions like the following: ``` fn foo<T: AsRef<str>>(x: T) { ... } ``` However, I couldn't figure out how to determine whether a function input type was instantiated from a parameter with a trait bound. If someone could offer me some guidance, I would be happy to add such functionality. Closes #7933 changelog: Add [`unnecessary_to_owned`] lint
2 parents aa3648a + b891389 commit 40fd785

17 files changed

+1887
-20
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3210,6 +3210,7 @@ Released 2018-09-13
32103210
[`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
32113211
[`unnecessary_self_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_self_imports
32123212
[`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by
3213+
[`unnecessary_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned
32133214
[`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap
32143215
[`unnecessary_wraps`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
32153216
[`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern

clippy_lints/src/lib.register_all.rs

+1
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
181181
LintId::of(methods::UNNECESSARY_FILTER_MAP),
182182
LintId::of(methods::UNNECESSARY_FOLD),
183183
LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS),
184+
LintId::of(methods::UNNECESSARY_TO_OWNED),
184185
LintId::of(methods::UNWRAP_OR_ELSE_DEFAULT),
185186
LintId::of(methods::USELESS_ASREF),
186187
LintId::of(methods::WRONG_SELF_CONVENTION),

clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,7 @@ store.register_lints(&[
315315
methods::UNNECESSARY_FILTER_MAP,
316316
methods::UNNECESSARY_FOLD,
317317
methods::UNNECESSARY_LAZY_EVALUATIONS,
318+
methods::UNNECESSARY_TO_OWNED,
318319
methods::UNWRAP_OR_ELSE_DEFAULT,
319320
methods::UNWRAP_USED,
320321
methods::USELESS_ASREF,

clippy_lints/src/lib.register_perf.rs

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
1717
LintId::of(methods::MANUAL_STR_REPEAT),
1818
LintId::of(methods::OR_FUN_CALL),
1919
LintId::of(methods::SINGLE_CHAR_PATTERN),
20+
LintId::of(methods::UNNECESSARY_TO_OWNED),
2021
LintId::of(misc::CMP_OWNED),
2122
LintId::of(mutex_atomic::MUTEX_ATOMIC),
2223
LintId::of(redundant_clone::REDUNDANT_CLONE),

clippy_lints/src/methods/implicit_clone.rs

+20-9
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,7 @@ use super::IMPLICIT_CLONE;
1212
pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, span: Span) {
1313
if_chain! {
1414
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
15-
if match method_name {
16-
"to_os_string" => is_diag_item_method(cx, method_def_id, sym::OsStr),
17-
"to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned),
18-
"to_path_buf" => is_diag_item_method(cx, method_def_id, sym::Path),
19-
"to_vec" => cx.tcx.impl_of_method(method_def_id)
20-
.map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl())
21-
== Some(true),
22-
_ => false,
23-
};
15+
if is_clone_like(cx, method_name, method_def_id);
2416
let return_type = cx.typeck_results().expr_ty(expr);
2517
let input_type = cx.typeck_results().expr_ty(recv).peel_refs();
2618
if let Some(ty_name) = input_type.ty_adt_def().map(|adt_def| cx.tcx.item_name(adt_def.did));
@@ -38,3 +30,22 @@ pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv
3830
}
3931
}
4032
}
33+
34+
/// Returns true if the named method can be used to clone the receiver.
35+
/// Note that `to_string` is not flagged by `implicit_clone`. So other lints that call
36+
/// `is_clone_like` and that do flag `to_string` must handle it separately. See, e.g.,
37+
/// `is_to_owned_like` in `unnecessary_to_owned.rs`.
38+
pub fn is_clone_like(cx: &LateContext<'_>, method_name: &str, method_def_id: hir::def_id::DefId) -> bool {
39+
match method_name {
40+
"to_os_string" => is_diag_item_method(cx, method_def_id, sym::OsStr),
41+
"to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned),
42+
"to_path_buf" => is_diag_item_method(cx, method_def_id, sym::Path),
43+
"to_vec" => {
44+
cx.tcx
45+
.impl_of_method(method_def_id)
46+
.map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl())
47+
== Some(true)
48+
},
49+
_ => false,
50+
}
51+
}

clippy_lints/src/methods/mod.rs

+31-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@ mod suspicious_splitn;
5656
mod uninit_assumed_init;
5757
mod unnecessary_filter_map;
5858
mod unnecessary_fold;
59+
mod unnecessary_iter_cloned;
5960
mod unnecessary_lazy_eval;
61+
mod unnecessary_to_owned;
6062
mod unwrap_or_else_default;
6163
mod unwrap_used;
6264
mod useless_asref;
@@ -1885,6 +1887,32 @@ declare_clippy_lint! {
18851887
"usages of `str::splitn` that can be replaced with `str::split`"
18861888
}
18871889

1890+
declare_clippy_lint! {
1891+
/// ### What it does
1892+
/// Checks for unnecessary calls to [`ToOwned::to_owned`](https://doc.rust-lang.org/std/borrow/trait.ToOwned.html#tymethod.to_owned)
1893+
/// and other `to_owned`-like functions.
1894+
///
1895+
/// ### Why is this bad?
1896+
/// The unnecessary calls result in useless allocations.
1897+
///
1898+
/// ### Example
1899+
/// ```rust
1900+
/// let path = std::path::Path::new("x");
1901+
/// foo(&path.to_string_lossy().to_string());
1902+
/// fn foo(s: &str) {}
1903+
/// ```
1904+
/// Use instead:
1905+
/// ```rust
1906+
/// let path = std::path::Path::new("x");
1907+
/// foo(&path.to_string_lossy());
1908+
/// fn foo(s: &str) {}
1909+
/// ```
1910+
#[clippy::version = "1.58.0"]
1911+
pub UNNECESSARY_TO_OWNED,
1912+
perf,
1913+
"unnecessary calls to `to_owned`-like functions"
1914+
}
1915+
18881916
pub struct Methods {
18891917
avoid_breaking_exported_api: bool,
18901918
msrv: Option<RustcVersion>,
@@ -1964,7 +1992,8 @@ impl_lint_pass!(Methods => [
19641992
MANUAL_STR_REPEAT,
19651993
EXTEND_WITH_DRAIN,
19661994
MANUAL_SPLIT_ONCE,
1967-
NEEDLESS_SPLITN
1995+
NEEDLESS_SPLITN,
1996+
UNNECESSARY_TO_OWNED,
19681997
]);
19691998

19701999
/// Extracts a method call name, args, and `Span` of the method name.
@@ -2007,6 +2036,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
20072036
single_char_add_str::check(cx, expr, args);
20082037
into_iter_on_ref::check(cx, expr, *method_span, method_call.ident.name, args);
20092038
single_char_pattern::check(cx, expr, method_call.ident.name, args);
2039+
unnecessary_to_owned::check(cx, expr, method_call.ident.name, args);
20102040
},
20112041
hir::ExprKind::Binary(op, lhs, rhs) if op.node == hir::BinOpKind::Eq || op.node == hir::BinOpKind::Ne => {
20122042
let mut info = BinaryExprInfo {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::higher::ForLoop;
3+
use clippy_utils::source::snippet_opt;
4+
use clippy_utils::ty::{get_associated_type, get_iterator_item_ty, implements_trait};
5+
use clippy_utils::{fn_def_id, get_parent_expr, path_to_local_id, usage};
6+
use rustc_errors::Applicability;
7+
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
8+
use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind, HirId, LangItem, Mutability, Pat};
9+
use rustc_lint::LateContext;
10+
use rustc_middle::{hir::map::Map, ty};
11+
use rustc_span::{sym, Symbol};
12+
13+
use super::UNNECESSARY_TO_OWNED;
14+
15+
pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol, receiver: &'tcx Expr<'tcx>) -> bool {
16+
if_chain! {
17+
if let Some(parent) = get_parent_expr(cx, expr);
18+
if let Some(callee_def_id) = fn_def_id(cx, parent);
19+
if is_into_iter(cx, callee_def_id);
20+
then {
21+
check_for_loop_iter(cx, parent, method_name, receiver)
22+
} else {
23+
false
24+
}
25+
}
26+
}
27+
28+
/// Checks whether `expr` is an iterator in a `for` loop and, if so, determines whether the
29+
/// iterated-over items could be iterated over by reference. The reason why `check` above does not
30+
/// include this code directly is so that it can be called from
31+
/// `unnecessary_into_owned::check_into_iter_call_arg`.
32+
pub fn check_for_loop_iter(
33+
cx: &LateContext<'tcx>,
34+
expr: &'tcx Expr<'tcx>,
35+
method_name: Symbol,
36+
receiver: &'tcx Expr<'tcx>,
37+
) -> bool {
38+
if_chain! {
39+
if let Some(grandparent) = get_parent_expr(cx, expr).and_then(|parent| get_parent_expr(cx, parent));
40+
if let Some(ForLoop { pat, body, .. }) = ForLoop::hir(grandparent);
41+
let (clone_or_copy_needed, addr_of_exprs) = clone_or_copy_needed(cx, pat, body);
42+
if !clone_or_copy_needed;
43+
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
44+
then {
45+
let snippet = if_chain! {
46+
if let ExprKind::MethodCall(maybe_iter_method_name, _, [collection], _) = receiver.kind;
47+
if maybe_iter_method_name.ident.name == sym::iter;
48+
49+
if let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
50+
let receiver_ty = cx.typeck_results().expr_ty(receiver);
51+
if implements_trait(cx, receiver_ty, iterator_trait_id, &[]);
52+
if let Some(iter_item_ty) = get_iterator_item_ty(cx, receiver_ty);
53+
54+
if let Some(into_iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator);
55+
let collection_ty = cx.typeck_results().expr_ty(collection);
56+
if implements_trait(cx, collection_ty, into_iterator_trait_id, &[]);
57+
if let Some(into_iter_item_ty) = get_associated_type(cx, collection_ty, into_iterator_trait_id, "Item");
58+
59+
if iter_item_ty == into_iter_item_ty;
60+
if let Some(collection_snippet) = snippet_opt(cx, collection.span);
61+
then {
62+
collection_snippet
63+
} else {
64+
receiver_snippet
65+
}
66+
};
67+
span_lint_and_then(
68+
cx,
69+
UNNECESSARY_TO_OWNED,
70+
expr.span,
71+
&format!("unnecessary use of `{}`", method_name),
72+
|diag| {
73+
diag.span_suggestion(expr.span, "use", snippet, Applicability::MachineApplicable);
74+
for addr_of_expr in addr_of_exprs {
75+
match addr_of_expr.kind {
76+
ExprKind::AddrOf(_, _, referent) => {
77+
let span = addr_of_expr.span.with_hi(referent.span.lo());
78+
diag.span_suggestion(span, "remove this `&`", String::new(), Applicability::MachineApplicable);
79+
}
80+
_ => unreachable!(),
81+
}
82+
}
83+
}
84+
);
85+
return true;
86+
}
87+
}
88+
false
89+
}
90+
91+
/// The core logic of `check_for_loop_iter` above, this function wraps a use of
92+
/// `CloneOrCopyVisitor`.
93+
fn clone_or_copy_needed(
94+
cx: &LateContext<'tcx>,
95+
pat: &Pat<'tcx>,
96+
body: &'tcx Expr<'tcx>,
97+
) -> (bool, Vec<&'tcx Expr<'tcx>>) {
98+
let mut visitor = CloneOrCopyVisitor {
99+
cx,
100+
binding_hir_ids: pat_bindings(pat),
101+
clone_or_copy_needed: false,
102+
addr_of_exprs: Vec::new(),
103+
};
104+
visitor.visit_expr(body);
105+
(visitor.clone_or_copy_needed, visitor.addr_of_exprs)
106+
}
107+
108+
/// Returns a vector of all `HirId`s bound by the pattern.
109+
fn pat_bindings(pat: &Pat<'_>) -> Vec<HirId> {
110+
let mut collector = usage::ParamBindingIdCollector {
111+
binding_hir_ids: Vec::new(),
112+
};
113+
collector.visit_pat(pat);
114+
collector.binding_hir_ids
115+
}
116+
117+
/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only
118+
/// operations performed on `binding_hir_ids` are:
119+
/// * to take non-mutable references to them
120+
/// * to use them as non-mutable `&self` in method calls
121+
/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true
122+
/// when `CloneOrCopyVisitor` is done visiting.
123+
struct CloneOrCopyVisitor<'cx, 'tcx> {
124+
cx: &'cx LateContext<'tcx>,
125+
binding_hir_ids: Vec<HirId>,
126+
clone_or_copy_needed: bool,
127+
addr_of_exprs: Vec<&'tcx Expr<'tcx>>,
128+
}
129+
130+
impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> {
131+
type Map = Map<'tcx>;
132+
133+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
134+
NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
135+
}
136+
137+
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
138+
walk_expr(self, expr);
139+
if self.is_binding(expr) {
140+
if let Some(parent) = get_parent_expr(self.cx, expr) {
141+
match parent.kind {
142+
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => {
143+
self.addr_of_exprs.push(parent);
144+
return;
145+
},
146+
ExprKind::MethodCall(_, _, args, _) => {
147+
if_chain! {
148+
if args.iter().skip(1).all(|arg| !self.is_binding(arg));
149+
if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id);
150+
let method_ty = self.cx.tcx.type_of(method_def_id);
151+
let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder();
152+
if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not));
153+
then {
154+
return;
155+
}
156+
}
157+
},
158+
_ => {},
159+
}
160+
}
161+
self.clone_or_copy_needed = true;
162+
}
163+
}
164+
}
165+
166+
impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> {
167+
fn is_binding(&self, expr: &Expr<'tcx>) -> bool {
168+
self.binding_hir_ids
169+
.iter()
170+
.any(|hir_id| path_to_local_id(expr, *hir_id))
171+
}
172+
}
173+
174+
/// Returns true if the named method is `IntoIterator::into_iter`.
175+
pub fn is_into_iter(cx: &LateContext<'_>, callee_def_id: DefId) -> bool {
176+
cx.tcx.lang_items().require(LangItem::IntoIterIntoIter) == Ok(callee_def_id)
177+
}

0 commit comments

Comments
 (0)