Skip to content

Fix needless_borrow causing mutable borrows to be moved #8217

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

Merged
merged 3 commits into from
Jan 23, 2022

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jan 3, 2022

fixes #8191

changelog: Fix needless_borrow causing mutable borrows to be moved
changelog: Rename ref_in_deref to needless_borrow
changelog: Suggest removing the borrow on method call receivers in needless_borrow

@rust-highfive
Copy link

r? @camsteffen

(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 Jan 3, 2022
@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 2c9f1c3 to f48c0a8 Compare January 11, 2022 20:05
@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 11, 2022

This now includes all cases covered by ref_in_deref, which lints (&_).field. The description for both would lead one to believe they lint the same thing. Should I just rename it?

@camsteffen
Copy link
Contributor

I think we can remove/deprecate ref_in_deref then?

Tangentially, I noticed the other day that we don't lint (*x).y. I wonder where that could fit.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 11, 2022

I'm (very slowly) working on an explicit deref lint. It would fit in there whenever that's done.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 12, 2022

That successfully broke the changelog... Not really sure what to change there.

@camsteffen
Copy link
Contributor

You need to add declare_deprecated_lint! in clippy_lints/src/deprecated_lints.rs and also add a deprecation test in tests/ui/deprecated.rs. Then run cargo dev update_lints and it should be good.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 15, 2022

That would mean register_renamed can't be used, right?

@camsteffen
Copy link
Contributor

register_renamed would make it so any usage of ref_in_deref counts as needless_borrow. That may or may not be a good thing but I think the lints are different enough that we don't want that? In any case the error message should point to needless_borrow.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 15, 2022

The description for ref_inderef` is

Checks for references in expressions that use auto dereference.

The description for needless_borrow is

Checks for address of operations (&) that are going to be dereferenced immediately by the compiler.

Those describe exactly the same thing. o the point where anything covered by one and not he other would be a clear false negative. Given that renaming sound like the appropriate thing to do.

@camsteffen
Copy link
Contributor

camsteffen commented Jan 15, 2022

Okay, good point. So I think you can just add register_renamed by hand. To fix the error, change any ref_in_deref links in the changelog to not be links anymore.

@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 1ad870e to cb7fe3e Compare January 15, 2022 23:22
@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 15, 2022

I was trying to avoid editing the changelog, but I don't see any other option.

@camsteffen
Copy link
Contributor

One idea for the lint message but otherwise looks good. Please update "changelog" to include the lint rename and added functionality.

@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 4cda9ab to e94a2e7 Compare January 21, 2022 14:57
@camsteffen
Copy link
Contributor

Squash some commits then will merge!

@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from e94a2e7 to ddc3caa Compare January 23, 2022 00:57
@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from ddc3caa to 5eae868 Compare January 23, 2022 01:35
@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 5eae868 to c615140 Compare January 23, 2022 02:22
@camsteffen
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2022

📌 Commit c615140 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Jan 23, 2022

⌛ Testing commit c615140 with merge 788a8bc...

@bors
Copy link
Contributor

bors commented Jan 23, 2022

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

@bors bors merged commit 788a8bc into rust-lang:master Jan 23, 2022
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.

Bad suggestion for &mut *(&mut &mut T) from clippy::needless_borrow
4 participants