Add `unnecessary_to_owned` lint #7978

smoelius · 2021-11-16T01:12:56Z

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

rust-highfive · 2021-11-16T01:12:59Z

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

flip1995 · 2021-11-16T15:38:25Z

Oh this is a nice lint! I only skimmed the code. It'll probably take me a few days to do a in-depth review. In the mean time, could you add some documentation to the functions that makes it easier to understand what those functions are checking for, please? Especially the check_call_arg and the check_addr_of_expr functions.

However, I couldn't figure out how to determine whether a function input type was instantiated from a parameter with a trait bound.

Shouldn't implements_trait on the arg Ty return true for the AsRef trait? In other words: I don't think it should matter if you have x: impl AsRef or <T: AsRef>(x; T). But maybe that's a bug/shortcoming of the implements_trait utility 🤔

smoelius · 2021-11-16T17:46:15Z

Oh this is a nice lint! ... In the mean time, could you add some documentation to the functions that makes it easier to understand what those functions are checking for, please?

Thank you. :) I will.

Shouldn't implements_trait on the arg Ty return true for the AsRef trait? In other words: I don't think it should matter if you have x: impl AsRef or <T: AsRef>(x; T). But maybe that's a bug/shortcoming of the implements_trait utility thinking

Sorry for not explaining well. Currently, I call fn_sig to get the function's signature. For these two functions:

fn foo(_: impl AsRef<str>) {}
fn bar<T: AsRef<str>>(_: T) {}

fn_sig gives you structures with the following debug representations:

([impl AsRef<str>]; c_variadic: false)->()
([T]; c_variadic: false)->()

Currently, the lint catches calls to the foo function, because it parses the ParamTy name, which is impl AsRef<str> in the first debug representation above. But for the bar case, the ParamTy name is just T, so there's not enough information in the name to recognize that the argument has to implement AsRef<str>.

Presumably, there is some rustc API that allows one to query this, e.g., "What are the bounds on the type parameters of the function with definition id def_id?" But I couldn't find that API.

bors · 2021-11-17T02:02:12Z

☔ The latest upstream changes (presumably #7896) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995 · 2021-11-17T10:23:20Z

The FnSig::inputs method gives you a slice of Tys. Calling implements_trait(cx, arg_ty, as_ref_did, &[???]) should return true for fn bar<T: AsRef<str>>(_: T) {}? Or am I completely misunderstanding something?

smoelius · 2021-11-17T10:35:10Z

Well, it could be that I am misunderstanding something.

The FnSig::inputs method gives you a slice of Tys.

I think once you have Tys it is too late. Because what you really want to know is: what were the requirements when those Tys where chosen? Could we have chosen something else?

I'm not sure implements_trait(.., as_ref_id, ..) is of help here, because we can't be sure that AsRef<..> was a bound on the argument type.

That's sort of the crux of the issue: how do you find out what those bounds were?

flip1995 · 2021-11-17T10:58:26Z

I think once you have Tys it is too late.

The Ty must retain all type information, so also all the traits this type implements. Once Clippy has access to Tys, type resolution is fully done, so all relevant trait information for the type should be there.

So it doesn't really matter what bounds the argument type had in the code. After trait resolution and type checking, the generic type should implement the trait, no matter what actual type it is.

smoelius · 2021-11-17T11:12:03Z

Sorry, I think I may have just found the answer: https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.predicates_of

Please give me a few days to look into this and update the lint.

I apologize for my confusion.

smoelius · 2021-11-21T22:56:55Z

@flip1995 I think this is working as it should and you are welcome to start reviewing whenever you have time.

camsteffen · 2021-11-28T01:30:01Z

I just want to make sure we are careful with how this lint overlaps with implicit_clone and redundant_clone. This lint probably should share a list of methods with implicit_clone. I think this lint should just skip over cases of redundant_clone. Or you could make this lint pass responsible for those particular instances of redundant_clone if that simplifies the code overall.

smoelius · 2021-11-28T01:54:51Z

I think this lint should just skip over cases of redundant_clone. Or you could make this lint pass responsible for those particular instances of redundant_clone if that simplifies the code overall.

How would you handle this, @camsteffen? Is there a way to, say, to call redundant_clone's check_fn and check whether it fired?

camsteffen · 2021-11-28T04:00:35Z

Maybe you could do it the other way around - call the new lint implemention inside of redundant_clone.

smoelius · 2021-11-28T13:01:02Z

Actually maybe a better strategy is the following: have this lint apply only when the receiver is copy-able, or when the to_owned-like function is Cow::into_owned.

I think that would eliminate the possibility of overlap between redundant_clone and this lint, but would otherwise handle all cases that this lint currently tests for.

An alternative might be to do just the first part and extend redundant_clone to handle Cow::into_owned. But I'm not sure if that would go too much against the grain of redundant_clone (i.e., because in a call to Cow::into_owned, the receiver is moved).

camsteffen · 2021-11-28T14:47:21Z

I was just thinking of cases like path_buf.to_path_buf().

smoelius · 2021-11-29T00:36:24Z

I was just thinking of cases like path_buf.to_path_buf().

OK. Then I will implement what I called a "better strategy" above. That should handle the case that you mentioned, and is less invasive than the other options (no changes to redundant_clone).

I will also implement the list of methods shared with implicit_clone.

@flip1995 Please give me a day or two to implement these changes.

smoelius · 2021-11-30T11:02:41Z

@flip1995 I think the just pushed changes address all of @camsteffen's concerns.

* Share a list of methods with `implicit_clone` * Ensure no overlap with `redundant_clone`

hellow554 · 2021-12-13T14:21:44Z

clippy_lints/src/methods/mod.rs

-    NEEDLESS_SPLITN
+    NEEDLESS_SPLITN,
+    UNNECESSARY_TO_OWNED


IMHO this cloud/should have a trailing , so that the diff is only one insertion, instead of 1 delete, 2 insertions

hellow554 · 2021-12-13T14:22:59Z

clippy_lints/src/unit_return_expecting_ord.rs

@@ -169,7 +169,7 @@ impl<'tcx> LateLintPass<'tcx> for UnitReturnExpectingOrd {
                                    trait_name
                                ),
                                Some(last_semi),
-                                &"probably caused by this trailing semicolon".to_string(),
+                                "probably caused by this trailing semicolon",


hellow554 · 2021-12-13T17:36:04Z

@smoelius can you please test the following code if the new lint triggers incorrectly?

#![feature(drain_filter)]

fn main() {
    let mut a = vec![3, 5, 7, -4, -6];
    for x in a.drain_filter(|x| *x < 0).collect::<Vec<_>>() {
        a.push(-x);
    }
}

The problem is, that I can't remove collect, because else I would get borrowchecker error.

smoelius · 2021-12-13T17:46:46Z

For me, the new lint doesn't seem to fire on that example. ~~Could you please tell me what you are seeing?~~

Please tell me if you observe something else.

hellow554 · 2021-12-13T18:03:58Z

For me, the new lint doesn't seem to fire on that example

that's great <3

Please tell me if you observe something else.

I haven't observed anything, because I didn't want to compile clippy on my machine ;) That's why I asked you to run that code for me and as it turns out, it looks fine :)
Thanks!

llogiq

I'm impressed by the very comprehensive test suite. 👍 I only found a few small nits. With those picked, I think this should be deemed mergeable.

llogiq · 2021-12-14T14:50:34Z

clippy_lints/src/methods/mod.rs

+    /// and other `to_owned`-like functions.
+    ///
+    /// ### Why is this bad?
+    /// The unnecessary calls result in unnecessary allocations.


Style: "unnecessary" used twice. How about changing one to "needless" or "useless"?

llogiq · 2021-12-14T14:51:55Z

clippy_lints/src/methods/mod.rs

+    /// ### Example
+    /// ```rust
+    /// let path = std::path::Path::new("x");
+    /// foo(&path.to_string_lossy().to_string());


Is to_string even caught here? I didn't find any mention of it in is_clone_like and wondered if that was covered by another lint.

to_string is handled outside of is_clone_like:

rust-clippy/clippy_lints/src/methods/unnecessary_to_owned.rs

Line 390 in cb609a9

|| is_to_string(cx, method_name, method_def_id)

rust-clippy/clippy_lints/src/methods/unnecessary_to_owned.rs

Lines 399 to 401 in cb609a9

fn is_to_string(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {

method_name.as_str() == "to_string" && is_diag_trait_item(cx, method_def_id, sym::ToString)

}

The reason for this is that implicit_clone calls is_clone_like (as a consequence of this PR), and I didn't want to change implicit_clone's behavior:

rust-clippy/clippy_lints/src/methods/implicit_clone.rs

Line 15 in cb609a9

if is_clone_like(cx, method_name, method_def_id);

Please let me know if you think something should be adjusted.

Then we should at least have a comment stating this.

Please tell me if what I wrote is inadequate.

llogiq · 2021-12-14T14:54:24Z

clippy_lints/src/methods/unnecessary_to_owned.rs

+        then {
+            let (target_ty, n_target_refs) = peel_mid_ty_refs(target_ty);
+            let (receiver_ty, n_receiver_refs) = peel_mid_ty_refs(receiver_ty);
+            if_chain! {


This if_chain! has only two conditions that could simply be put together with &&, thus reducing rightwards drift.

smoelius · 2021-12-14T23:41:12Z

@llogiq Thank for the review and for your kind words. Please let me know if you have any comments following the adjustments.

llogiq · 2021-12-15T10:46:37Z

This looks good now. Thank you! @bors r+

bors · 2021-12-15T10:46:39Z

📌 Commit b891389 has been approved by llogiq

bors · 2021-12-15T10:46:46Z

⌛ Testing commit b891389 with merge 40fd785...

bors · 2021-12-15T11:01:01Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 40fd785 to master...

rust-highfive assigned flip1995 Nov 16, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 16, 2021

smoelius force-pushed the master branch 3 times, most recently from 1f2c732 to 24ac43c Compare November 21, 2021 22:51

camsteffen mentioned this pull request Dec 3, 2021

to_vec on for loop expression #8069

Closed

smoelius added 3 commits December 13, 2021 06:31

Add unnecessary_to_owned lint

468c86e

Address review comments

290f74b

* Share a list of methods with `implicit_clone` * Ensure no overlap with `redundant_clone`

Handle to_vec on for loop expression rust-lang#8069

3807905

smoelius force-pushed the master branch from ba1651c to 3807905 Compare December 13, 2021 12:05

hellow554 reviewed Dec 13, 2021

View reviewed changes

Add comma

cb609a9

llogiq reviewed Dec 14, 2021

View reviewed changes

Address review comments

3beb6b1

Expand is_clone_like comment

b891389

bors merged commit 40fd785 into rust-lang:master Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `unnecessary_to_owned` lint #7978

Add `unnecessary_to_owned` lint #7978

smoelius commented Nov 16, 2021

rust-highfive commented Nov 16, 2021

flip1995 commented Nov 16, 2021 •

edited

Loading

smoelius commented Nov 16, 2021 •

edited

Loading

bors commented Nov 17, 2021

flip1995 commented Nov 17, 2021

smoelius commented Nov 17, 2021 •

edited

Loading

flip1995 commented Nov 17, 2021

smoelius commented Nov 17, 2021

smoelius commented Nov 21, 2021

camsteffen commented Nov 28, 2021

smoelius commented Nov 28, 2021

camsteffen commented Nov 28, 2021

smoelius commented Nov 28, 2021

camsteffen commented Nov 28, 2021

smoelius commented Nov 29, 2021

smoelius commented Nov 30, 2021

hellow554 Dec 13, 2021 •

edited

Loading

smoelius Dec 13, 2021

hellow554 Dec 13, 2021

hellow554 commented Dec 13, 2021

smoelius commented Dec 13, 2021 •

edited

Loading

hellow554 commented Dec 13, 2021

llogiq left a comment

llogiq Dec 14, 2021

llogiq Dec 14, 2021

smoelius Dec 14, 2021

llogiq Dec 15, 2021

smoelius Dec 15, 2021

llogiq Dec 14, 2021

smoelius commented Dec 14, 2021

llogiq commented Dec 15, 2021

bors commented Dec 15, 2021

bors commented Dec 15, 2021

bors commented Dec 15, 2021

	fn is_to_string(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
	method_name.as_str() == "to_string" && is_diag_trait_item(cx, method_def_id, sym::ToString)
	}

Add unnecessary_to_owned lint #7978

Add unnecessary_to_owned lint #7978

Conversation

smoelius commented Nov 16, 2021

rust-highfive commented Nov 16, 2021

flip1995 commented Nov 16, 2021 • edited Loading

smoelius commented Nov 16, 2021 • edited Loading

bors commented Nov 17, 2021

flip1995 commented Nov 17, 2021

smoelius commented Nov 17, 2021 • edited Loading

flip1995 commented Nov 17, 2021

smoelius commented Nov 17, 2021

smoelius commented Nov 21, 2021

camsteffen commented Nov 28, 2021

smoelius commented Nov 28, 2021

camsteffen commented Nov 28, 2021

smoelius commented Nov 28, 2021

camsteffen commented Nov 28, 2021

smoelius commented Nov 29, 2021

smoelius commented Nov 30, 2021

hellow554 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellow554 commented Dec 13, 2021

smoelius commented Dec 13, 2021 • edited Loading

hellow554 commented Dec 13, 2021

llogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius commented Dec 14, 2021

llogiq commented Dec 15, 2021

bors commented Dec 15, 2021

bors commented Dec 15, 2021

bors commented Dec 15, 2021

Add `unnecessary_to_owned` lint #7978

Add `unnecessary_to_owned` lint #7978

flip1995 commented Nov 16, 2021 •

edited

Loading

smoelius commented Nov 16, 2021 •

edited

Loading

smoelius commented Nov 17, 2021 •

edited

Loading

hellow554 Dec 13, 2021 •

edited

Loading

smoelius commented Dec 13, 2021 •

edited

Loading