Skip to content

Commit f058fe7

Browse files
authored
Fix cloned_ref_to_slice_refs FN on to_owned() (#16329)
*[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/16329)* Closes #16320 changelog: [`cloned_ref_to_slice_refs`] fix FN on `to_owned()`
2 parents 87e4c91 + f1f5580 commit f058fe7

5 files changed

Lines changed: 319 additions & 27 deletions

File tree

clippy_lints/src/cloned_ref_to_slice_refs.rs

Lines changed: 107 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,22 @@
1+
use std::ops::ControlFlow;
2+
13
use clippy_config::Conf;
24
use clippy_utils::diagnostics::span_lint_and_sugg;
35
use clippy_utils::msrvs::{self, Msrv};
46
use clippy_utils::res::{MaybeDef, MaybeTypeckRes};
57
use clippy_utils::sugg::Sugg;
68
use clippy_utils::visitors::is_const_evaluatable;
7-
use clippy_utils::{is_in_const_context, is_mutable};
9+
use clippy_utils::{is_in_const_context, is_mutable, sym};
10+
use rustc_ast::Mutability;
811
use rustc_errors::Applicability;
9-
use rustc_hir::{Expr, ExprKind};
12+
use rustc_hir::{Expr, ExprKind, HirId, LangItem};
1013
use rustc_lint::{LateContext, LateLintPass};
14+
use rustc_middle::ty;
15+
use rustc_middle::ty::adjustment::{Adjust, DerefAdjustKind, OverloadedDeref};
1116
use rustc_session::impl_lint_pass;
12-
use rustc_span::sym;
17+
use rustc_span::Symbol;
18+
19+
use crate::methods::is_clone_like;
1320

1421
declare_clippy_lint! {
1522
/// ### What it does
@@ -73,29 +80,116 @@ impl<'tcx> LateLintPass<'tcx> for ClonedRefToSliceRefs<'_> {
7380
&& let ExprKind::Array([item]) = &arr.kind
7481

7582
// check for clones
76-
&& let ExprKind::MethodCall(_, val, _, _) = item.kind
77-
&& cx.ty_based_def(item).opt_parent(cx).is_diag_item(cx, sym::Clone)
83+
&& let ExprKind::MethodCall(path, recv, _, _) = item.kind
84+
&& let Some(adjustment) = is_needless_clone_or_equivalent(cx, recv, path.ident.name, item.hir_id)
7885

7986
// check for immutability or purity
80-
&& (!is_mutable(cx, val) || is_const_evaluatable(cx, val))
87+
&& (!is_mutable(cx, recv) || is_const_evaluatable(cx, recv))
8188

8289
// get appropriate crate for `slice::from_ref`
8390
&& let Some(builtin_crate) = clippy_utils::std_or_core(cx)
8491
{
85-
let mut sugg = Sugg::hir(cx, val, "_");
86-
if !cx.typeck_results().expr_ty(val).is_ref() {
87-
sugg = sugg.addr();
88-
}
92+
let mut applicability = Applicability::MachineApplicable;
93+
let sugg = Sugg::hir_with_context(cx, recv, expr.span.ctxt(), "_", &mut applicability);
8994

9095
span_lint_and_sugg(
9196
cx,
9297
CLONED_REF_TO_SLICE_REFS,
9398
expr.span,
94-
format!("this call to `clone` can be replaced with `{builtin_crate}::slice::from_ref`"),
99+
format!(
100+
"unnecessary use of `{}` to create a slice from a reference",
101+
path.ident.name
102+
),
95103
"try",
96-
format!("{builtin_crate}::slice::from_ref({sugg})"),
97-
Applicability::MaybeIncorrect,
104+
format!("{builtin_crate}::slice::from_ref({adjustment}{sugg})"),
105+
applicability,
98106
);
99107
}
100108
}
101109
}
110+
111+
/// Checks if a method call is a needless clone or equivalent. If so, returns the necessary
112+
/// adjustments to use the method receiver directly without cloning.
113+
/// For example, in the code below:
114+
/// ```rust,no_run
115+
/// use std::path::PathBuf;
116+
///
117+
/// let w = &PathBuf::new();
118+
/// let b = &[w.to_path_buf()];
119+
/// ```
120+
/// We would replace `&[w.to_path_buf()]` with `std::slice::from_ref(&*w)`,
121+
/// hence we return `Some("&*")` as the adjustment.
122+
fn is_needless_clone_or_equivalent<'tcx>(
123+
cx: &LateContext<'tcx>,
124+
method_recv: &'tcx Expr<'tcx>,
125+
method_name: Symbol,
126+
hir_id: HirId,
127+
) -> Option<String> {
128+
let method_def = cx.ty_based_def(hir_id).opt_parent(cx)?;
129+
if !method_def.is_lang_item(cx, LangItem::Clone) && !is_clone_like(cx, method_name, method_def) {
130+
return None;
131+
}
132+
133+
let method_ret_ty = cx.typeck_results().node_type(hir_id);
134+
let method_recv_ty = cx.typeck_results().expr_ty_adjusted(method_recv);
135+
let ty::Ref(_, method_recv_ty_inner, Mutability::Not) = method_recv_ty.kind() else {
136+
return None;
137+
};
138+
139+
let method_recv_adjustments = cx.typeck_results().expr_adjustments(method_recv);
140+
141+
// The return type of the clone-like method should be the same as the inner type of the reference
142+
// being cloned, except for the following special cases:
143+
// 1. `OsString`, which is first dereferenced to `OsStr` and the borrowed as `&OsStr`.
144+
// 2. `PathBuf`, which is first dereferenced to `Path` and then borrowed as `&Path`.
145+
let adjust_target_ty = if method_ret_ty == *method_recv_ty_inner {
146+
method_ret_ty
147+
} else if let Some(after_special_case_ty_name @ (sym::OsStr | sym::Path)) = method_recv_ty_inner.opt_diag_name(cx)
148+
// Looking for the `OSString -> OSStr` or `PathBuf -> Path` adjustment in the abovementioned special cases
149+
&& let [preceeding_derefs @ .., special_case, last_borrow] = method_recv_adjustments
150+
&& matches!(
151+
special_case.kind,
152+
Adjust::Deref(DerefAdjustKind::Overloaded(OverloadedDeref {
153+
mutbl: Mutability::Not,
154+
..
155+
}))
156+
)
157+
&& matches!(last_borrow.kind, Adjust::Borrow(_))
158+
&& special_case.target.is_diag_item(cx, after_special_case_ty_name)
159+
&& let before_special_case_ty = preceeding_derefs
160+
.last().map_or_else(|| cx.typeck_results().expr_ty(method_recv), |a| a.target)
161+
&& matches!(
162+
(before_special_case_ty.opt_diag_name(cx)?, after_special_case_ty_name),
163+
(sym::OsString, sym::OsStr) | (sym::PathBuf, sym::Path))
164+
{
165+
before_special_case_ty
166+
} else {
167+
return None;
168+
};
169+
170+
// Find the number of adjustments required until `method_recv_ty_source` becomes `adjust_target_ty`
171+
let method_recv_ty_source = cx.typeck_results().expr_ty(method_recv);
172+
let adjust_count = method_recv_adjustments
173+
.iter()
174+
.enumerate()
175+
.try_fold(method_recv_ty_source, |ty, (i, a)| {
176+
if ty == adjust_target_ty {
177+
ControlFlow::Break(i)
178+
} else {
179+
ControlFlow::Continue(a.target)
180+
}
181+
})
182+
.break_value()?;
183+
184+
let (needs_borrow, deref_count) = if adjust_count == 0 || !method_recv_ty_source.is_ref() {
185+
(true, adjust_count)
186+
} else {
187+
(false, adjust_count - 1)
188+
};
189+
190+
Some(if needs_borrow {
191+
format!("&{}", "*".repeat(deref_count))
192+
} else {
193+
"*".repeat(deref_count)
194+
})
195+
}

clippy_lints/src/methods/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,6 @@ use clippy_utils::macros::FormatArgsStorage;
155155
use clippy_utils::msrvs::{self, Msrv};
156156
use clippy_utils::res::{MaybeDef, MaybeTypeckRes};
157157
use clippy_utils::{contains_return, iter_input_pats, peel_blocks, sym};
158-
pub use path_ends_with_ext::DEFAULT_ALLOWED_DOTFILES;
159158
use rustc_data_structures::fx::FxHashSet;
160159
use rustc_hir::{self as hir, Expr, ExprKind, Node, Stmt, StmtKind, TraitItem, TraitItemKind};
161160
use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -165,6 +164,9 @@ use rustc_span::{Span, Symbol};
165164

166165
use crate::matches::manual_filter;
167166

167+
pub use implicit_clone::is_clone_like;
168+
pub use path_ends_with_ext::DEFAULT_ALLOWED_DOTFILES;
169+
168170
declare_clippy_lint! {
169171
/// ### What it does
170172
/// Checks for usage of `_.and_then(|x| Some(y))`, `_.and_then(|x| Ok(y))`

tests/ui/cloned_ref_to_slice_refs.fixed

Lines changed: 77 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![allow(clippy::borrow_deref_ref)]
12
#![warn(clippy::cloned_ref_to_slice_refs)]
23

34
#[derive(Clone)]
@@ -7,18 +8,18 @@ fn main() {
78
{
89
let data = Data;
910
let data_ref = &data;
10-
let _ = std::slice::from_ref(data_ref); //~ ERROR: this call to `clone` can be replaced with `std::slice::from_ref`
11+
let _ = std::slice::from_ref(data_ref); //~ cloned_ref_to_slice_refs
1112
}
1213

1314
{
14-
let _ = std::slice::from_ref(&Data); //~ ERROR: this call to `clone` can be replaced with `std::slice::from_ref`
15+
let _ = std::slice::from_ref(&Data); //~ cloned_ref_to_slice_refs
1516
}
1617

1718
{
1819
#[derive(Clone)]
1920
struct Point(i32, i32);
2021

21-
let _ = std::slice::from_ref(&Point(0, 0)); //~ ERROR: this call to `clone` can be replaced with `std::slice::from_ref`
22+
let _ = std::slice::from_ref(&Point(0, 0)); //~ cloned_ref_to_slice_refs
2223
}
2324

2425
// the string was cloned with the intention to not mutate
@@ -62,3 +63,76 @@ fn main() {
6263
let _ = &[data_1.clone(), data_2.clone()];
6364
}
6465
}
66+
67+
fn issue16320(items: &[String]) {
68+
use std::ffi::OsString;
69+
use std::ops::Deref;
70+
use std::path::PathBuf;
71+
72+
let _a = String::new();
73+
let _b = std::slice::from_ref(&_a);
74+
//~^ cloned_ref_to_slice_refs
75+
let _c = std::slice::from_ref(&_a);
76+
//~^ cloned_ref_to_slice_refs
77+
78+
let _a = OsString::new();
79+
let _b = std::slice::from_ref(&_a);
80+
//~^ cloned_ref_to_slice_refs
81+
82+
let _a = PathBuf::new();
83+
let _b = std::slice::from_ref(&_a);
84+
//~^ cloned_ref_to_slice_refs
85+
86+
let _a = &PathBuf::new();
87+
let _b = std::slice::from_ref(_a);
88+
//~^ cloned_ref_to_slice_refs
89+
90+
#[derive(Clone)]
91+
struct A(i32);
92+
93+
impl std::fmt::Display for A {
94+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
95+
write!(f, "{}", self.0)
96+
}
97+
}
98+
99+
let a = A(42);
100+
_ = &[a.to_string()];
101+
102+
struct Wrapper<T>(T);
103+
impl<T> Deref for Wrapper<T> {
104+
type Target = T;
105+
fn deref(&self) -> &Self::Target {
106+
&self.0
107+
}
108+
}
109+
110+
let w = Wrapper(String::from("hello"));
111+
let w = Wrapper(w);
112+
let _b = std::slice::from_ref(&**w);
113+
//~^ cloned_ref_to_slice_refs
114+
115+
let w = Wrapper(&PathBuf::new());
116+
let w = Wrapper(w);
117+
let _b = std::slice::from_ref(&***w);
118+
//~^ cloned_ref_to_slice_refs
119+
}
120+
121+
fn wrongly_unmangled_macros(items: &[String]) {
122+
use std::path::PathBuf;
123+
124+
struct Wrapper {
125+
inner: PathBuf,
126+
}
127+
128+
let _a = Wrapper { inner: PathBuf::new() };
129+
130+
macro_rules! accessor {
131+
($e:expr) => {
132+
$e.inner
133+
};
134+
}
135+
136+
let _d = std::slice::from_ref(&accessor!(_a));
137+
//~^ cloned_ref_to_slice_refs
138+
}

tests/ui/cloned_ref_to_slice_refs.rs

Lines changed: 77 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![allow(clippy::borrow_deref_ref)]
12
#![warn(clippy::cloned_ref_to_slice_refs)]
23

34
#[derive(Clone)]
@@ -7,18 +8,18 @@ fn main() {
78
{
89
let data = Data;
910
let data_ref = &data;
10-
let _ = &[data_ref.clone()]; //~ ERROR: this call to `clone` can be replaced with `std::slice::from_ref`
11+
let _ = &[data_ref.clone()]; //~ cloned_ref_to_slice_refs
1112
}
1213

1314
{
14-
let _ = &[Data.clone()]; //~ ERROR: this call to `clone` can be replaced with `std::slice::from_ref`
15+
let _ = &[Data.clone()]; //~ cloned_ref_to_slice_refs
1516
}
1617

1718
{
1819
#[derive(Clone)]
1920
struct Point(i32, i32);
2021

21-
let _ = &[Point(0, 0).clone()]; //~ ERROR: this call to `clone` can be replaced with `std::slice::from_ref`
22+
let _ = &[Point(0, 0).clone()]; //~ cloned_ref_to_slice_refs
2223
}
2324

2425
// the string was cloned with the intention to not mutate
@@ -62,3 +63,76 @@ fn main() {
6263
let _ = &[data_1.clone(), data_2.clone()];
6364
}
6465
}
66+
67+
fn issue16320(items: &[String]) {
68+
use std::ffi::OsString;
69+
use std::ops::Deref;
70+
use std::path::PathBuf;
71+
72+
let _a = String::new();
73+
let _b = &[_a.to_owned()];
74+
//~^ cloned_ref_to_slice_refs
75+
let _c = &[_a.to_string()];
76+
//~^ cloned_ref_to_slice_refs
77+
78+
let _a = OsString::new();
79+
let _b = &[_a.to_os_string()];
80+
//~^ cloned_ref_to_slice_refs
81+
82+
let _a = PathBuf::new();
83+
let _b = &[_a.to_path_buf()];
84+
//~^ cloned_ref_to_slice_refs
85+
86+
let _a = &PathBuf::new();
87+
let _b = &[_a.to_path_buf()];
88+
//~^ cloned_ref_to_slice_refs
89+
90+
#[derive(Clone)]
91+
struct A(i32);
92+
93+
impl std::fmt::Display for A {
94+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
95+
write!(f, "{}", self.0)
96+
}
97+
}
98+
99+
let a = A(42);
100+
_ = &[a.to_string()];
101+
102+
struct Wrapper<T>(T);
103+
impl<T> Deref for Wrapper<T> {
104+
type Target = T;
105+
fn deref(&self) -> &Self::Target {
106+
&self.0
107+
}
108+
}
109+
110+
let w = Wrapper(String::from("hello"));
111+
let w = Wrapper(w);
112+
let _b = &[w.to_string()];
113+
//~^ cloned_ref_to_slice_refs
114+
115+
let w = Wrapper(&PathBuf::new());
116+
let w = Wrapper(w);
117+
let _b = &[w.to_path_buf()];
118+
//~^ cloned_ref_to_slice_refs
119+
}
120+
121+
fn wrongly_unmangled_macros(items: &[String]) {
122+
use std::path::PathBuf;
123+
124+
struct Wrapper {
125+
inner: PathBuf,
126+
}
127+
128+
let _a = Wrapper { inner: PathBuf::new() };
129+
130+
macro_rules! accessor {
131+
($e:expr) => {
132+
$e.inner
133+
};
134+
}
135+
136+
let _d = &[accessor!(_a).to_path_buf()];
137+
//~^ cloned_ref_to_slice_refs
138+
}

0 commit comments

Comments
 (0)