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

Add let_underscore_drop #6305

Merged
merged 8 commits into from
Nov 9, 2020
Merged

Add let_underscore_drop #6305

merged 8 commits into from
Nov 9, 2020

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Nov 8, 2020

This line generalizes let_underscore_lock (#5101) to warn about any initializer expression that implements Drop.

So, for example, the following would generate a warning:

struct Droppable;
impl Drop for Droppable {
    fn drop(&mut self) {}
}
let _ = Droppable;

I tried to preserve the original let_underscore_lock functionality in the sense that the warning generated for

let _ = mutex.lock();

should be unchanged.

Please keep the line below
changelog: Add lint [let_underscore_drop]

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 8, 2020
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Don't forget to update the reference files and run cargo dev update_lints after applying the review.

clippy_lints/src/let_underscore.rs Outdated Show resolved Hide resolved
clippy_lints/src/let_underscore.rs Outdated Show resolved Hide resolved
clippy_lints/src/let_underscore.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 8, 2020
smoelius and others added 5 commits November 8, 2020 17:24
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: Philipp Krones <hello@philkrones.com>
@smoelius
Copy link
Contributor Author

smoelius commented Nov 8, 2020

@flip1995 Thanks for the review.

Could you please sanity check that this was the right way to address the map_clone and map_flatten tests?
06e81bb#diff-ef362d1c15b3b4ea77af80401c4b546728b49f799e29ff11f722c4690515ea8b
06e81bb#diff-154c4677379d20cf922a34a0a42cbce62c1a36d8852b28e7d5ada6aaa980cd03

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Could you please sanity check that this was the right way to address the map_clone and map_flatten tests?

LGTM

@@ -1,3 +1,12 @@
error: non-binding `let` on a type that implements `Drop`
Copy link
Member

Choose a reason for hiding this comment

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

Please also allow the lint in tests/ui/filter_methods.rs. Or better, swap out

#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::missing_docs_in_private_items)]

With just

#![warn(clippy::filter_map)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or better, ...

As an outsider, a change like that looks to me like it would go against an established convention. Maybe it would be better left for a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

It is an ancient convention to warn on all lints in test files. Now one test file should only test one lint. But we don't care enough about this to fix that.

So both's fine.

@flip1995
Copy link
Member

flip1995 commented Nov 9, 2020

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Nov 9, 2020

📌 Commit 4852cca has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Nov 9, 2020

⌛ Testing commit 4852cca with merge dd826b4...

@bors
Copy link
Collaborator

bors commented Nov 9, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants