Skip to content

Commit 9c78883

Browse files
borsbors
committed
Auto merge of rust-lang#8794 - smoelius:fix-8759, r=llogiq
Address `unnecessary_to_owned` false positive My proposed fix for rust-lang#8759 is to revise the conditions that delineate `redundant_clone` and `unnecessary_to_owned`: ```rust // Only flag cases satisfying at least one of the following three conditions: // * the referent and receiver types are distinct // * the referent/receiver type is a copyable array // * the method is `Cow::into_owned` // This restriction is to ensure there is no overlap between `redundant_clone` and this // lint. It also avoids the following false positive: // rust-lang/rust-clippy#8759 // Arrays are a bit of a corner case. Non-copyable arrays are handled by // `redundant_clone`, but copyable arrays are not. ``` This change causes a few cases that were previously flagged by `unnecessary_to_owned` to no longer be flagged. But one could argue those cases would be better handled by `redundant_clone`. Closes rust-lang#8759 changelog: none
2 parents 43756b6 + 91a822c commit 9c78883

File tree

5 files changed

+160
-45
lines changed

5 files changed

+160
-45
lines changed

‎clippy_lints/src/methods/unnecessary_to_owned.rs‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,12 @@ fn check_addr_of_expr(
6565
if let Some(parent) = get_parent_expr(cx, expr);
6666
if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) = parent.kind;
6767
let adjustments = cx.typeck_results().expr_adjustments(parent).iter().collect::<Vec<_>>();
68-
if let Some(target_ty) = match adjustments[..]
69-
{
68+
if let
7069
// For matching uses of `Cow::from`
7170
[
7271
Adjustment {
7372
kind: Adjust::Deref(None),
74-
..
73+
target: referent_ty,
7574
},
7675
Adjustment {
7776
kind: Adjust::Borrow(_),
@@ -82,7 +81,7 @@ fn check_addr_of_expr(
8281
| [
8382
Adjustment {
8483
kind: Adjust::Deref(None),
85-
..
84+
target: referent_ty,
8685
},
8786
Adjustment {
8887
kind: Adjust::Borrow(_),
@@ -97,7 +96,7 @@ fn check_addr_of_expr(
9796
| [
9897
Adjustment {
9998
kind: Adjust::Deref(None),
100-
..
99+
target: referent_ty,
101100
},
102101
Adjustment {
103102
kind: Adjust::Deref(Some(OverloadedDeref { .. })),
@@ -107,17 +106,24 @@ fn check_addr_of_expr(
107106
kind: Adjust::Borrow(_),
108107
target: target_ty,
109108
},
110-
] => Some(target_ty),
111-
_ => None,
112-
};
109+
] = adjustments[..];
113110
let receiver_ty = cx.typeck_results().expr_ty(receiver);
114-
// Only flag cases where the receiver is copyable or the method is `Cow::into_owned`. This
115-
// restriction is to ensure there is not overlap between `redundant_clone` and this lint.
116-
if is_copy(cx, receiver_ty) || is_cow_into_owned(cx, method_name, method_def_id);
111+
let (target_ty, n_target_refs) = peel_mid_ty_refs(*target_ty);
112+
let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
113+
// Only flag cases satisfying at least one of the following three conditions:
114+
// * the referent and receiver types are distinct
115+
// * the referent/receiver type is a copyable array
116+
// * the method is `Cow::into_owned`
117+
// This restriction is to ensure there is no overlap between `redundant_clone` and this
118+
// lint. It also avoids the following false positive:
119+
// https://github.com/rust-lang/rust-clippy/issues/8759
120+
// Arrays are a bit of a corner case. Non-copyable arrays are handled by
121+
// `redundant_clone`, but copyable arrays are not.
122+
if *referent_ty != receiver_ty
123+
|| (matches!(referent_ty.kind(), ty::Array(..)) && is_copy(cx, *referent_ty))
124+
|| is_cow_into_owned(cx, method_name, method_def_id);
117125
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
118126
then {
119-
let (target_ty, n_target_refs) = peel_mid_ty_refs(*target_ty);
120-
let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
121127
if receiver_ty == target_ty && n_target_refs >= n_receiver_refs {
122128
span_lint_and_sugg(
123129
cx,
@@ -207,7 +213,11 @@ fn check_into_iter_call_arg(
207213
if unnecessary_iter_cloned::check_for_loop_iter(cx, parent, method_name, receiver, true) {
208214
return true;
209215
}
210-
let cloned_or_copied = if is_copy(cx, item_ty) && meets_msrv(msrv, &msrvs::ITERATOR_COPIED) { "copied" } else { "cloned" };
216+
let cloned_or_copied = if is_copy(cx, item_ty) && meets_msrv(msrv, &msrvs::ITERATOR_COPIED) {
217+
"copied"
218+
} else {
219+
"cloned"
220+
};
211221
// The next suggestion may be incorrect because the removal of the `to_owned`-like
212222
// function could cause the iterator to hold a reference to a resource that is used
213223
// mutably. See https://github.com/rust-lang/rust-clippy/issues/8148.

‎tests/ui/recursive_format_impl.stderr‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,6 @@ LL | write!(f, "{}", self.to_string())
66
|
77
= note: `-D clippy::recursive-format-impl` implied by `-D warnings`
88

9-
error: unnecessary use of `to_string`
10-
--> $DIR/recursive_format_impl.rs:61:50
11-
|
12-
LL | Self::E(string) => write!(f, "E {}", string.to_string()),
13-
| ^^^^^^^^^^^^^^^^^^
14-
|
15-
= note: `-D clippy::unnecessary-to-owned` implied by `-D warnings`
16-
= note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)
17-
189
error: using `self` as `Display` in `impl Display` will cause infinite recursion
1910
--> $DIR/recursive_format_impl.rs:73:9
2011
|
@@ -87,5 +78,5 @@ LL | write!(f, "{}", &&**&&*self)
8778
|
8879
= note: this error originates in the macro `write` (in Nightly builds, run with -Z macro-backtrace for more info)
8980

90-
error: aborting due to 11 previous errors
81+
error: aborting due to 10 previous errors
9182

‎tests/ui/unnecessary_to_owned.fixed‎

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,10 @@ fn main() {
7878
require_slice(array.as_ref());
7979
require_slice(array_ref.as_ref());
8080
require_slice(slice);
81-
require_slice(x_ref);
81+
require_slice(&x_ref.to_owned()); // No longer flagged because of #8759.
8282

8383
require_x(&Cow::<X>::Owned(x.clone()));
84-
require_x(x_ref);
84+
require_x(&x_ref.to_owned()); // No longer flagged because of #8759.
8585

8686
require_deref_c_str(c_str);
8787
require_deref_os_str(os_str);
@@ -152,6 +152,7 @@ fn main() {
152152
require_os_str(&OsString::from("x"));
153153
require_path(&std::path::PathBuf::from("x"));
154154
require_str(&String::from("x"));
155+
require_slice(&[String::from("x")]);
155156
}
156157

157158
fn require_c_str(_: &CStr) {}
@@ -272,3 +273,59 @@ mod issue_8507 {
272273
Box::new(build(y))
273274
}
274275
}
276+
277+
// https://github.com/rust-lang/rust-clippy/issues/8759
278+
mod issue_8759 {
279+
#![allow(dead_code)]
280+
281+
#[derive(Default)]
282+
struct View {}
283+
284+
impl std::borrow::ToOwned for View {
285+
type Owned = View;
286+
fn to_owned(&self) -> Self::Owned {
287+
View {}
288+
}
289+
}
290+
291+
#[derive(Default)]
292+
struct RenderWindow {
293+
default_view: View,
294+
}
295+
296+
impl RenderWindow {
297+
fn default_view(&self) -> &View {
298+
&self.default_view
299+
}
300+
fn set_view(&mut self, _view: &View) {}
301+
}
302+
303+
fn main() {
304+
let mut rw = RenderWindow::default();
305+
rw.set_view(&rw.default_view().to_owned());
306+
}
307+
}
308+
309+
mod issue_8759_variant {
310+
#![allow(dead_code)]
311+
312+
#[derive(Clone, Default)]
313+
struct View {}
314+
315+
#[derive(Default)]
316+
struct RenderWindow {
317+
default_view: View,
318+
}
319+
320+
impl RenderWindow {
321+
fn default_view(&self) -> &View {
322+
&self.default_view
323+
}
324+
fn set_view(&mut self, _view: &View) {}
325+
}
326+
327+
fn main() {
328+
let mut rw = RenderWindow::default();
329+
rw.set_view(&rw.default_view().to_owned());
330+
}
331+
}

‎tests/ui/unnecessary_to_owned.rs‎

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,10 @@ fn main() {
7878
require_slice(&array.to_owned());
7979
require_slice(&array_ref.to_owned());
8080
require_slice(&slice.to_owned());
81-
require_slice(&x_ref.to_owned());
81+
require_slice(&x_ref.to_owned()); // No longer flagged because of #8759.
8282

8383
require_x(&Cow::<X>::Owned(x.clone()).into_owned());
84-
require_x(&x_ref.to_owned());
84+
require_x(&x_ref.to_owned()); // No longer flagged because of #8759.
8585

8686
require_deref_c_str(c_str.to_owned());
8787
require_deref_os_str(os_str.to_owned());
@@ -152,6 +152,7 @@ fn main() {
152152
require_os_str(&OsString::from("x").to_os_string());
153153
require_path(&std::path::PathBuf::from("x").to_path_buf());
154154
require_str(&String::from("x").to_string());
155+
require_slice(&[String::from("x")].to_owned());
155156
}
156157

157158
fn require_c_str(_: &CStr) {}
@@ -272,3 +273,59 @@ mod issue_8507 {
272273
Box::new(build(y.to_string()))
273274
}
274275
}
276+
277+
// https://github.com/rust-lang/rust-clippy/issues/8759
278+
mod issue_8759 {
279+
#![allow(dead_code)]
280+
281+
#[derive(Default)]
282+
struct View {}
283+
284+
impl std::borrow::ToOwned for View {
285+
type Owned = View;
286+
fn to_owned(&self) -> Self::Owned {
287+
View {}
288+
}
289+
}
290+
291+
#[derive(Default)]
292+
struct RenderWindow {
293+
default_view: View,
294+
}
295+
296+
impl RenderWindow {
297+
fn default_view(&self) -> &View {
298+
&self.default_view
299+
}
300+
fn set_view(&mut self, _view: &View) {}
301+
}
302+
303+
fn main() {
304+
let mut rw = RenderWindow::default();
305+
rw.set_view(&rw.default_view().to_owned());
306+
}
307+
}
308+
309+
mod issue_8759_variant {
310+
#![allow(dead_code)]
311+
312+
#[derive(Clone, Default)]
313+
struct View {}
314+
315+
#[derive(Default)]
316+
struct RenderWindow {
317+
default_view: View,
318+
}
319+
320+
impl RenderWindow {
321+
fn default_view(&self) -> &View {
322+
&self.default_view
323+
}
324+
fn set_view(&mut self, _view: &View) {}
325+
}
326+
327+
fn main() {
328+
let mut rw = RenderWindow::default();
329+
rw.set_view(&rw.default_view().to_owned());
330+
}
331+
}

‎tests/ui/unnecessary_to_owned.stderr‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,18 @@ note: this value is dropped without further use
4747
LL | require_str(&String::from("x").to_string());
4848
| ^^^^^^^^^^^^^^^^^
4949

50+
error: redundant clone
51+
--> $DIR/unnecessary_to_owned.rs:155:39
52+
|
53+
LL | require_slice(&[String::from("x")].to_owned());
54+
| ^^^^^^^^^^^ help: remove this
55+
|
56+
note: this value is dropped without further use
57+
--> $DIR/unnecessary_to_owned.rs:155:20
58+
|
59+
LL | require_slice(&[String::from("x")].to_owned());
60+
| ^^^^^^^^^^^^^^^^^^^
61+
5062
error: unnecessary use of `into_owned`
5163
--> $DIR/unnecessary_to_owned.rs:60:36
5264
|
@@ -151,24 +163,12 @@ error: unnecessary use of `to_owned`
151163
LL | require_slice(&slice.to_owned());
152164
| ^^^^^^^^^^^^^^^^^ help: use: `slice`
153165

154-
error: unnecessary use of `to_owned`
155-
--> $DIR/unnecessary_to_owned.rs:81:19
156-
|
157-
LL | require_slice(&x_ref.to_owned());
158-
| ^^^^^^^^^^^^^^^^^ help: use: `x_ref`
159-
160166
error: unnecessary use of `into_owned`
161167
--> $DIR/unnecessary_to_owned.rs:83:42
162168
|
163169
LL | require_x(&Cow::<X>::Owned(x.clone()).into_owned());
164170
| ^^^^^^^^^^^^^ help: remove this
165171

166-
error: unnecessary use of `to_owned`
167-
--> $DIR/unnecessary_to_owned.rs:84:15
168-
|
169-
LL | require_x(&x_ref.to_owned());
170-
| ^^^^^^^^^^^^^^^^^ help: use: `x_ref`
171-
172172
error: unnecessary use of `to_owned`
173173
--> $DIR/unnecessary_to_owned.rs:86:25
174174
|
@@ -476,7 +476,7 @@ LL | let _ = IntoIterator::into_iter([std::path::PathBuf::new()][..].to_owne
476476
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[std::path::PathBuf::new()][..].iter().cloned()`
477477

478478
error: unnecessary use of `to_vec`
479-
--> $DIR/unnecessary_to_owned.rs:197:14
479+
--> $DIR/unnecessary_to_owned.rs:198:14
480480
|
481481
LL | for t in file_types.to_vec() {
482482
| ^^^^^^^^^^^^^^^^^^^
@@ -492,22 +492,22 @@ LL + let path = match get_file_path(t) {
492492
|
493493

494494
error: unnecessary use of `to_vec`
495-
--> $DIR/unnecessary_to_owned.rs:220:14
495+
--> $DIR/unnecessary_to_owned.rs:221:14
496496
|
497497
LL | let _ = &["x"][..].to_vec().into_iter();
498498
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().cloned()`
499499

500500
error: unnecessary use of `to_vec`
501-
--> $DIR/unnecessary_to_owned.rs:225:14
501+
--> $DIR/unnecessary_to_owned.rs:226:14
502502
|
503503
LL | let _ = &["x"][..].to_vec().into_iter();
504504
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().copied()`
505505

506506
error: unnecessary use of `to_string`
507-
--> $DIR/unnecessary_to_owned.rs:272:24
507+
--> $DIR/unnecessary_to_owned.rs:273:24
508508
|
509509
LL | Box::new(build(y.to_string()))
510510
| ^^^^^^^^^^^^^ help: use: `y`
511511

512-
error: aborting due to 79 previous errors
512+
error: aborting due to 78 previous errors
513513

0 commit comments

Comments
 (0)