Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #9771 (unnecessary_to_owned false positive) #9796

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Nov 4, 2022

Fixes #9771

In that issue's example(s), the lint tried to add a & to a value, which implicitly changed the type of a field to a reference. The fix is to add the reference to receiver_ty (the type of the receiver of the to_owned-like method), before passing receiver_ty to can_change_type. can_change_type properly rejects the modified receiver_ty.

cc: @mikerite just because I think he was the author of can_change_type.

changelog: fix unnecessary_to_owned false positive which implicitly tried to change the type of a field to a reference

@rust-highfive
Copy link

r? @flip1995

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

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

pub fn from(c: &[u8]) -> Key<Vec<u8>> {
let v = [c].concat();
Key(v.to_vec())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this issue was not a false positive, ideally this should be corrected to Key(v), as .concat() already returns a Vec<u8>.

Suggested change
Key(v.to_vec())
Key(v)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take your point, but I think we have to treat the issue as a false positive for now.

Replacing v.to_vec() with v would move v, and unnecessary_to_owned doesn't currently have a way to tell whether a value can be moved.

I suspect it would be possible to incorporate something like possible_borrowers from redundant_clone. But I also think that would be a significant change and, hence, an enhancement.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, just wanted to make sure that you understood the issue correctly. I don't know much about the internals of clippy, so I can't help you with implementing this.

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Nov 22, 2022

📌 Commit f27ca5c has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 22, 2022

⌛ Testing commit f27ca5c with merge 3f3131b...

bors added a commit that referenced this pull request Nov 22, 2022
Fix #9771 (`unnecessary_to_owned` false positive)

Fixes #9771

In that issue's example(s), the lint tried to add a `&` to a value, which implicitly changed the type of a field to a reference. The fix is to add the reference to `receiver_ty` (the type of the receiver of the `to_owned`-like method), before passing `receiver_ty` to `can_change_type`. `can_change_type` properly rejects the modified `receiver_ty`.

cc: `@mikerite` just because I think he was the author of `can_change_type`.

changelog: fix `unnecessary_to_owned` false positive which implicitly tried to change the type of a field to a reference
@flip1995
Copy link
Member

@bors r-

@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 22, 2022

📌 Commit f27ca5c has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 22, 2022

⌛ Testing commit f27ca5c with merge 94ce446...

@bors
Copy link
Collaborator

bors commented Nov 22, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 94ce446 to master...

@bors bors merged commit 94ce446 into rust-lang:master Nov 22, 2022
@smoelius smoelius deleted the issue-9771 branch November 22, 2022 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary_to_owned includes invalid & in suggestion
5 participants