Skip to content

Commit 290f74b

Browse files
smoeliussmoelius
committed
Address review comments
* Share a list of methods with `implicit_clone` * Ensure no overlap with `redundant_clone`
1 parent 468c86e commit 290f74b

File tree

6 files changed

+160
-67
lines changed

6 files changed

+160
-67
lines changed

‎clippy_lints/src/methods/implicit_clone.rs

Lines changed: 17 additions & 9 deletions
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,19 @@ 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+
pub fn is_clone_like(cx: &LateContext<'_>, method_name: &str, method_def_id: hir::def_id::DefId) -> bool {
36+
match method_name {
37+
"to_os_string" => is_diag_item_method(cx, method_def_id, sym::OsStr),
38+
"to_owned" => is_diag_trait_item(cx, method_def_id, sym::ToOwned),
39+
"to_path_buf" => is_diag_item_method(cx, method_def_id, sym::Path),
40+
"to_vec" => {
41+
cx.tcx
42+
.impl_of_method(method_def_id)
43+
.map(|impl_did| Some(impl_did) == cx.tcx.lang_items().slice_alloc_impl())
44+
== Some(true)
45+
},
46+
_ => false,
47+
}
48+
}

‎clippy_lints/src/methods/unnecessary_to_owned.rs

Lines changed: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1+
use super::implicit_clone::is_clone_like;
12
use clippy_utils::diagnostics::span_lint_and_sugg;
23
use clippy_utils::source::snippet_opt;
34
use clippy_utils::ty::{implements_trait, is_copy, peel_mid_ty_refs};
4-
use clippy_utils::{get_parent_expr, match_def_path, paths};
5+
use clippy_utils::{get_parent_expr, is_diag_item_method, is_diag_trait_item};
56
use rustc_errors::Applicability;
67
use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind};
78
use rustc_lint::LateContext;
@@ -14,28 +15,17 @@ use std::cmp::max;
1415

1516
use super::UNNECESSARY_TO_OWNED;
1617

17-
const TO_OWNED_LIKE_PATHS: &[&[&str]] = &[
18-
&paths::COW_INTO_OWNED,
19-
&paths::OS_STR_TO_OS_STRING,
20-
&paths::PATH_TO_PATH_BUF,
21-
&paths::SLICE_TO_VEC,
22-
&paths::TO_OWNED_METHOD,
23-
&paths::TO_STRING_METHOD,
24-
];
25-
2618
pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol, args: &'tcx [Expr<'tcx>]) {
2719
if_chain! {
2820
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
29-
if TO_OWNED_LIKE_PATHS
30-
.iter()
31-
.any(|path| match_def_path(cx, method_def_id, path));
21+
if is_to_owned_like(cx, method_name, method_def_id);
3222
if let [receiver] = args;
3323
then {
3424
// At this point, we know the call is of a `to_owned`-like function. The functions
3525
// `check_addr_of_expr` and `check_call_arg` determine whether the call is unnecessary
3626
// based on its context, that is, whether it is a referent in an `AddrOf` expression or
3727
// an argument in a function call.
38-
if check_addr_of_expr(cx, expr, method_name, receiver) {
28+
if check_addr_of_expr(cx, expr, method_name, method_def_id, receiver) {
3929
return;
4030
}
4131
check_call_arg(cx, expr, method_name, receiver);
@@ -45,10 +35,12 @@ pub fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: Symbol
4535

4636
/// Checks whether `expr` is a referent in an `AddrOf` expression and, if so, determines whether its
4737
/// call of a `to_owned`-like function is unnecessary.
38+
#[allow(clippy::too_many_lines)]
4839
fn check_addr_of_expr(
4940
cx: &LateContext<'tcx>,
5041
expr: &'tcx Expr<'tcx>,
5142
method_name: Symbol,
43+
method_def_id: DefId,
5244
receiver: &'tcx Expr<'tcx>,
5345
) -> bool {
5446
if_chain! {
@@ -100,14 +92,17 @@ fn check_addr_of_expr(
10092
] => Some(target_ty),
10193
_ => None,
10294
};
95+
let receiver_ty = cx.typeck_results().expr_ty(receiver);
96+
// Only flag cases where the receiver is copyable or the method is `Cow::into_owned`. This
97+
// restriction is to ensure there is not overlap between `redundant_clone` and this lint.
98+
if is_copy(cx, receiver_ty) || is_cow_into_owned(cx, method_name, method_def_id);
99+
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
103100
then {
104101
let (target_ty, n_target_refs) = peel_mid_ty_refs(target_ty);
105-
let receiver_ty = cx.typeck_results().expr_ty(receiver);
106102
let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
107103
if_chain! {
108104
if receiver_ty == target_ty;
109105
if n_target_refs >= n_receiver_refs;
110-
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
111106
then {
112107
span_lint_and_sugg(
113108
cx,
@@ -122,21 +117,32 @@ fn check_addr_of_expr(
122117
}
123118
}
124119
if implements_deref_trait(cx, receiver_ty, target_ty) {
125-
span_lint_and_sugg(
126-
cx,
127-
UNNECESSARY_TO_OWNED,
128-
expr.span.with_lo(receiver.span.hi()),
129-
&format!("unnecessary use of `{}`", method_name),
130-
"remove this",
131-
String::new(),
132-
Applicability::MachineApplicable,
133-
);
120+
if n_receiver_refs > 0 {
121+
span_lint_and_sugg(
122+
cx,
123+
UNNECESSARY_TO_OWNED,
124+
parent.span,
125+
&format!("unnecessary use of `{}`", method_name),
126+
"use",
127+
receiver_snippet,
128+
Applicability::MachineApplicable,
129+
);
130+
} else {
131+
span_lint_and_sugg(
132+
cx,
133+
UNNECESSARY_TO_OWNED,
134+
expr.span.with_lo(receiver.span.hi()),
135+
&format!("unnecessary use of `{}`", method_name),
136+
"remove this",
137+
String::new(),
138+
Applicability::MachineApplicable,
139+
);
140+
}
134141
return true;
135142
}
136143
if_chain! {
137144
if let Some(as_ref_trait_id) = cx.tcx.get_diagnostic_item(sym::AsRef);
138145
if implements_trait(cx, receiver_ty, as_ref_trait_id, &[GenericArg::from(target_ty)]);
139-
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
140146
then {
141147
span_lint_and_sugg(
142148
cx,
@@ -326,3 +332,21 @@ fn implements_deref_trait(cx: &LateContext<'tcx>, ty: Ty<'tcx>, deref_target_ty:
326332
}
327333
}
328334
}
335+
336+
/// Returns true if the named method can be used to convert the receiver to its "owned"
337+
/// representation.
338+
fn is_to_owned_like(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
339+
is_clone_like(cx, &*method_name.as_str(), method_def_id)
340+
|| is_cow_into_owned(cx, method_name, method_def_id)
341+
|| is_to_string(cx, method_name, method_def_id)
342+
}
343+
344+
/// Returns true if the named method is `Cow::into_owned`.
345+
fn is_cow_into_owned(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
346+
method_name.as_str() == "into_owned" && is_diag_item_method(cx, method_def_id, sym::Cow)
347+
}
348+
349+
/// Returns true if the named method is `ToString::to_string`.
350+
fn is_to_string(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
351+
method_name.as_str() == "to_string" && is_diag_trait_item(cx, method_def_id, sym::ToString)
352+
}

‎clippy_utils/src/paths.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
3636
pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
3737
pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"];
3838
pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"];
39-
pub const COW_INTO_OWNED: [&str; 4] = ["alloc", "borrow", "Cow", "into_owned"];
4039
pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
4140
pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
4241
pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"];
@@ -171,7 +170,6 @@ pub const SLICE_FROM_RAW_PARTS: [&str; 4] = ["core", "slice", "raw", "from_raw_p
171170
pub const SLICE_FROM_RAW_PARTS_MUT: [&str; 4] = ["core", "slice", "raw", "from_raw_parts_mut"];
172171
pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "into_vec"];
173172
pub const SLICE_ITER: [&str; 4] = ["core", "slice", "iter", "Iter"];
174-
pub const SLICE_TO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "to_vec"];
175173
pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"];
176174
pub const STDOUT: [&str; 4] = ["std", "io", "stdio", "stdout"];
177175
pub const CONVERT_IDENTITY: [&str; 3] = ["core", "convert", "identity"];

‎tests/ui/unnecessary_to_owned.fixed

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
// run-rustfix
22

33
#![allow(clippy::ptr_arg)]
4-
// Some of the expressions that `redundant_clone` flags overlap with ours. Enabling it interferes
5-
// with `rustfix`.
6-
#![allow(clippy::redundant_clone)]
7-
// `needless_borrow` is for checking the fixed code.
4+
// `needless_borrow` is to ensure there are no needles borrows in the fixed code.
85
#![warn(clippy::needless_borrow)]
6+
// `redundant_clone` is to ensure there is no overlap between that lint and this one.
7+
#![warn(clippy::redundant_clone)]
98
#![warn(clippy::unnecessary_to_owned)]
109

1110
use std::borrow::Cow;
12-
use std::ffi::{CStr, OsStr};
11+
use std::ffi::{CStr, CString, OsStr, OsString};
1312
use std::ops::Deref;
1413

1514
#[derive(Clone)]
@@ -51,6 +50,7 @@ fn main() {
5150
let array_ref = &["x"];
5251
let slice = &["x"][..];
5352
let x = X(String::from("x"));
53+
let x_ref = &x;
5454

5555
require_c_str(&Cow::from(c_str));
5656
require_c_str(c_str);
@@ -66,17 +66,17 @@ fn main() {
6666
require_str(s);
6767
require_str(&Cow::from(s));
6868
require_str(s);
69-
require_str(x.as_ref());
69+
require_str(x_ref.as_ref());
7070

7171
require_slice(slice);
7272
require_slice(&Cow::from(slice));
7373
require_slice(array.as_ref());
7474
require_slice(array_ref.as_ref());
7575
require_slice(slice);
76-
require_slice(&x);
76+
require_slice(x_ref);
7777

7878
require_x(&Cow::<X>::Owned(x.clone()));
79-
require_x(&x);
79+
require_x(x_ref);
8080

8181
require_deref_c_str(c_str);
8282
require_deref_os_str(os_str);
@@ -118,16 +118,23 @@ fn main() {
118118
require_as_ref_slice_str(array_ref, s);
119119
require_as_ref_slice_str(slice, s);
120120

121-
let _ = x.join(&x);
121+
let _ = x.join(x_ref);
122122

123123
// negative tests
124124
require_string(&s.to_string());
125125
require_string(&Cow::from(s).into_owned());
126126
require_string(&s.to_owned());
127-
require_string(&x.to_string());
127+
require_string(&x_ref.to_string());
128128

129129
// `X` isn't copy.
130+
require_slice(&x.to_owned());
130131
require_deref_slice(x.to_owned());
132+
133+
// The following should be flagged by `redundant_clone`, but not by this lint.
134+
require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap());
135+
require_os_str(&OsString::from("x"));
136+
require_path(&std::path::PathBuf::from("x"));
137+
require_str(&String::from("x"));
131138
}
132139

133140
fn require_c_str(_: &CStr) {}

‎tests/ui/unnecessary_to_owned.rs

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
// run-rustfix
22

33
#![allow(clippy::ptr_arg)]
4-
// Some of the expressions that `redundant_clone` flags overlap with ours. Enabling it interferes
5-
// with `rustfix`.
6-
#![allow(clippy::redundant_clone)]
7-
// `needless_borrow` is for checking the fixed code.
4+
// `needless_borrow` is to ensure there are no needles borrows in the fixed code.
85
#![warn(clippy::needless_borrow)]
6+
// `redundant_clone` is to ensure there is no overlap between that lint and this one.
7+
#![warn(clippy::redundant_clone)]
98
#![warn(clippy::unnecessary_to_owned)]
109

1110
use std::borrow::Cow;
12-
use std::ffi::{CStr, OsStr};
11+
use std::ffi::{CStr, CString, OsStr, OsString};
1312
use std::ops::Deref;
1413

1514
#[derive(Clone)]
@@ -51,6 +50,7 @@ fn main() {
5150
let array_ref = &["x"];
5251
let slice = &["x"][..];
5352
let x = X(String::from("x"));
53+
let x_ref = &x;
5454

5555
require_c_str(&Cow::from(c_str).into_owned());
5656
require_c_str(&c_str.to_owned());
@@ -66,17 +66,17 @@ fn main() {
6666
require_str(&s.to_string());
6767
require_str(&Cow::from(s).into_owned());
6868
require_str(&s.to_owned());
69-
require_str(&x.to_string());
69+
require_str(&x_ref.to_string());
7070

7171
require_slice(&slice.to_vec());
7272
require_slice(&Cow::from(slice).into_owned());
7373
require_slice(&array.to_owned());
7474
require_slice(&array_ref.to_owned());
7575
require_slice(&slice.to_owned());
76-
require_slice(&x.to_owned());
76+
require_slice(&x_ref.to_owned());
7777

7878
require_x(&Cow::<X>::Owned(x.clone()).into_owned());
79-
require_x(&x.to_owned());
79+
require_x(&x_ref.to_owned());
8080

8181
require_deref_c_str(c_str.to_owned());
8282
require_deref_os_str(os_str.to_owned());
@@ -118,16 +118,23 @@ fn main() {
118118
require_as_ref_slice_str(array_ref.to_owned(), s.to_owned());
119119
require_as_ref_slice_str(slice.to_owned(), s.to_owned());
120120

121-
let _ = x.join(&x.to_string());
121+
let _ = x.join(&x_ref.to_string());
122122

123123
// negative tests
124124
require_string(&s.to_string());
125125
require_string(&Cow::from(s).into_owned());
126126
require_string(&s.to_owned());
127-
require_string(&x.to_string());
127+
require_string(&x_ref.to_string());
128128

129129
// `X` isn't copy.
130+
require_slice(&x.to_owned());
130131
require_deref_slice(x.to_owned());
132+
133+
// The following should be flagged by `redundant_clone`, but not by this lint.
134+
require_c_str(&CString::from_vec_with_nul(vec![0]).unwrap().to_owned());
135+
require_os_str(&OsString::from("x").to_os_string());
136+
require_path(&std::path::PathBuf::from("x").to_path_buf());
137+
require_str(&String::from("x").to_string());
131138
}
132139

133140
fn require_c_str(_: &CStr) {}

0 commit comments

Comments
 (0)