added new lint `owned_to_owned` #6730

anall · 2021-02-12T19:05:11Z

Adding new lint owned_to_owned

Creating draft PR to have this looked over.
I think this takes all advice I received into account.

I did have to update the redundant_clone test to ignore this lint -- I felt that was the safest action.

closes: #6715
changelog: added new lint implicit_clone

rust-highfive · 2021-02-12T19:05:14Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

clippy_lints/src/methods/mod.rs

anall · 2021-02-12T20:05:09Z

Should I move my implementation to methods/owned_to_owned.rs to give less work for whoever takes up methods as part of #6680?

camsteffen

This is looking great! I know this PR is a draft but it seems nearly done to me 😄.

I did have to update the redundant_clone test to ignore this lint -- I felt that was the safest action.

Makes sense. And I think it's okay if those lints both fire on the same code.

You need tests for calling on a reference to a Vec (or other). You will probably need input_type.peel_refs() before calling same_type.

I'm sorry to bikeshed even more but I'm having second thoughts on the lint name. How about implicit_clone? It seems to me that the fact that we are dealing with owned types is not actually very relevant. The problem being linted is that the code is using a round-about way (deref + to_owned) to clone something rather than cloning explicitly. This lint might apply to a to_something method that does not produce an "owned" type. Or, this lint could apply to usage of from or into to clone something. I think this name is more self-explanatory too.

camsteffen · 2021-02-15T20:56:17Z

clippy_lints/src/methods/owned_to_owned.rs

+        if match_def_path(cx, meth_did, expected_path) &&
+            expected_type.map_or(true, |expected_type_path| match_type(cx,input_type,expected_type_path));
+        then {
+            if is_copy(cx,return_type) {


I don't think there will ever be an "owned" type that implements Copy.

Nothing seems to prevent to_owned() from being called on a custom type that implements Copy (like the Kitten type in my test), do we want this lint to fire on those cases?

Without this check, this will suggest replacing to_owned with clone, and then clone_on_copy will suggest dropping the .clone() entirely.

Oh I see. It is technically possible but I think this situation is unusual enough that we shouldn't have a special case for it. The purpose of having a borrowed variant of a type is to avoid expensive copies. But if a type implements Copy, then it shouldn't be expensive to copy. In any case, I think the lint should just fire normally because clone is still better than to_owned.

clippy_lints/src/methods/owned_to_owned.rs

camsteffen · 2021-02-15T21:02:14Z

clippy_lints/src/methods/owned_to_owned.rs

+        let input_type = cx.typeck_results().expr_ty(arg);
+        if TyS::same_type(return_type, input_type);
+        if match_def_path(cx, meth_did, expected_path) &&
+            expected_type.map_or(true, |expected_type_path| match_type(cx,input_type,expected_type_path));


I believe the "expected type" check is redundant since you are verifying the method.

For the current version here, I believe you are right but further changes in prep of using diagnostic items instead (including the ones I'm trying to get added in rust-lang/rust#82128) checks either the trait of the method (for to_owned) or the return type (for to_vec, …)

I can commit/push a new checkpoint if you'd like.

I believe this is no longer redundant after some cleanup refactoring + moving to being able to use diagnostic items (including the ones I'm trying to have added in rust-lang/rust#82128)

I haven't committed/pushed that change yet, but I check if the method belongs to a trait_path or the type.

I'll be pushing my new changes once I've gone through everything else.

I should clarify: when I say "to be able to use diagnostic items" I mean hopefully easily switch the function from taking paths to the function taking diagnostic items -- not a mix of both.

checks either the trait of the method (for to_owned) or the return type (for to_vec, …)

I think it would be better to just check the method for every case. If you only check the return type, then you might have a false positive if someone rolls their own trait with to_vec with different semantics (even though that would be a little odd). If you just check the method, there should be no such risk.

I guess by "check the method" while using diagnostic types, you mean "check the type/trait the method is on as well as checking the diagnostic item of return value of the type itself"?

Also, I just noticed os_string.to_os_string() is actually a method on OsStr so I will need to make sure that has a diagnostic item as well.

"check the method":

the method name is "to_vec"

the method is associated with type X

type X is diagnostic item "slice"

Not sure how to make the "associated" link but maybe tcx.associated_item and/or tcx.impl_of_method. We might need a util method to make this easier like is_diagnostic_assoc_fn(def_id, sym::slice, sym::to_vec).

clippy_lints/src/methods/owned_to_owned.rs

anall · 2021-02-15T23:38:19Z

You need tests for calling on a reference to a Vec (or other). You will probably need input_type.peel_refs() before calling same_type.

Wouldn't that end up suggesting that even calling to_owned on a &Vec should be turned into clone()? Not objecting, but that seems to be going against the original issue.

I did add a few (not-yet-committed) negative tests

I'm sorry to bikeshed even more but I'm having second thoughts on the lint name. How about implicit_clone? It seems to me that the fact that we are dealing with owned types is not actually very relevant. The problem being linted is that the code is using a round-about way (deref + to_owned) to clone something rather than cloning explicitly. This lint might apply to a to_something method that does not produce an "owned" type. Or, this lint could apply to usage of from or into to clone something. I think this name is more self-explanatory too.

That seems quite reasonable -- I'm not sure how GitHub handles tracking changes across renames, so will hold off on the rename until at least the next review pass.

(I know git itself handles that sub-optimally unless you explicitly tell git to follow renames)

clippy_lints/src/methods/mod.rs

anall · 2021-02-15T23:58:10Z

I know it's been suggested (and I agree) that replacing string_to_string with this lint is possible, but I'd feel better if that could be in a separate pull request.

I'm also happy waiting for those diagnostic items and doing that change before merging this, or am fine with this being merged before and then changing to the diagnostic items as soon as they are available. Whichever you prefer.

camsteffen · 2021-02-16T01:11:01Z

Wouldn't that end up suggesting that even calling to_owned on a &Vec should be turned into clone()?

Yes. I don't know why that would be any less valid. Both to_owned and clone accept a reference anyways.

I know it's been suggested (and I agree) that replacing string_to_string with this lint is possible, but I'd feel better if that could be in a separate pull request.

A separate PR for that is a good idea. Getting the diagnostic items in this PR is also good.

Manishearth · 2021-02-16T19:06:07Z

@bors delegate=camsteffen

r? @camsteffen since you already started (thank you). Feel free to bounce the review back if you don't have time or want me to have a look too!

bors · 2021-02-16T19:06:08Z

✌️ @camsteffen can now approve this pull request

… r=davidtwco add diagnostic items for OsString/PathBuf/Owned as well as to_vec on slice This is adding diagnostic items to be used by rust-lang/rust-clippy#6730, but my understanding is the clippy-side change does need to be done over there since I am adding a new clippy feature. Add diagnostic items to the following types: OsString (os_string_type) PathBuf (path_buf_type) Owned (to_owned_trait) As well as the to_vec method on slice/[T]

clippy_lints/src/methods/owned_to_owned.rs

camsteffen

A few small things and then we'll ship it!

clippy_lints/src/methods/implicit_clone.rs

clippy_utils/src/lib.rs

clippy_lints/src/methods/mod.rs

clippy_lints/src/methods/implicit_clone.rs

camsteffen · 2021-02-26T22:50:53Z

Very nice. Please squash into fewer commits and I'll merge.

camsteffen · 2021-02-26T22:57:16Z

Oops I forgot one thing. Please change from style to pedantic and run cargo dev update_lints.

camsteffen · 2021-02-27T01:39:59Z

Thanks! @bors r+

bors · 2021-02-27T01:40:00Z

📌 Commit 3d3cfd3 has been approved by camsteffen

bors · 2021-02-27T01:40:09Z

⌛ Testing commit 3d3cfd3 with merge 7154b23...

bors · 2021-02-27T01:51:18Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 7154b23 to master...

rust-highfive assigned Manishearth Feb 12, 2021

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

camsteffen reviewed Feb 12, 2021

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

anall mentioned this pull request Feb 15, 2021

add diagnostic items for OsString/PathBuf/Owned as well as to_vec on slice rust-lang/rust#82128

Merged

camsteffen suggested changes Feb 15, 2021

View reviewed changes

giraffate reviewed Feb 15, 2021

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

rust-highfive assigned camsteffen and unassigned Manishearth Feb 16, 2021

anall force-pushed the feature/owned_to_owned_methods branch from 129e2f4 to 1467261 Compare February 26, 2021 16:06

anall commented Feb 26, 2021

View reviewed changes

clippy_lints/src/methods/owned_to_owned.rs Outdated Show resolved Hide resolved

anall marked this pull request as ready for review February 26, 2021 17:24

camsteffen suggested changes Feb 26, 2021

View reviewed changes

added new lint implicit_clone

3d3cfd3

anall force-pushed the feature/owned_to_owned_methods branch from a2cc8c5 to 3d3cfd3 Compare February 27, 2021 01:15

bors merged commit 7154b23 into rust-lang:master Feb 27, 2021

This was referenced Feb 27, 2021

New Lint: needless_for_each #6706

Merged

New lint: iter_count #6791

Merged

anall mentioned this pull request Feb 27, 2021

migrate paths to newly-added diagnostic items #6807

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added new lint `owned_to_owned` #6730

added new lint `owned_to_owned` #6730

anall commented Feb 12, 2021 •

edited

Loading

rust-highfive commented Feb 12, 2021

anall commented Feb 12, 2021

camsteffen left a comment

camsteffen Feb 15, 2021

anall Feb 15, 2021

camsteffen Feb 16, 2021

camsteffen Feb 15, 2021

anall Feb 15, 2021

anall Feb 15, 2021

anall Feb 16, 2021

camsteffen Feb 16, 2021

anall Feb 16, 2021

camsteffen Feb 16, 2021

anall commented Feb 15, 2021

anall commented Feb 15, 2021 •

edited

Loading

camsteffen commented Feb 16, 2021

Manishearth commented Feb 16, 2021

bors commented Feb 16, 2021

camsteffen left a comment

camsteffen commented Feb 26, 2021

camsteffen commented Feb 26, 2021

camsteffen commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

added new lint owned_to_owned #6730

added new lint owned_to_owned #6730

Conversation

anall commented Feb 12, 2021 • edited Loading

rust-highfive commented Feb 12, 2021

anall commented Feb 12, 2021

camsteffen 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

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

anall commented Feb 15, 2021

anall commented Feb 15, 2021 • edited Loading

camsteffen commented Feb 16, 2021

Manishearth commented Feb 16, 2021

bors commented Feb 16, 2021

camsteffen left a comment

Choose a reason for hiding this comment

camsteffen commented Feb 26, 2021

camsteffen commented Feb 26, 2021

camsteffen commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

added new lint `owned_to_owned` #6730

added new lint `owned_to_owned` #6730

anall commented Feb 12, 2021 •

edited

Loading

anall commented Feb 15, 2021 •

edited

Loading