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 waker_clone_and_wake lint to check needless Waker clones #11698

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Oct 23, 2023

Check for patterns of waker.clone().wake() and replace them with waker.wake_by_ref().

An alternative name could be waker_clone_then_wake

changelog: [ waker_clone_wake]: new lint

@rustbot
Copy link
Collaborator

rustbot commented Oct 23, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 23, 2023
@Alexendoo Alexendoo assigned Alexendoo and unassigned giraffate Oct 23, 2023
@Alexendoo
Copy link
Member

This'd be a good one @y21

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Implementation looks pretty good already! Most of my comments are just nits.

Unsure about the lint name. What about just waker_clone_wake? I think that would go well with how some other lints are named after a chain of method calls, e.g. filter_map_bool_then

clippy_lints/src/methods/waker_clone_and_wake.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/waker_clone_and_wake.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/waker_clone_and_wake.rs Outdated Show resolved Hide resolved
tests/ui/waker_clone_and_wake.rs Outdated Show resolved Hide resolved
@y21
Copy link
Member

y21 commented Oct 24, 2023

Can you add a changelog entry to the PR description?
Something like

changelog: [waker_clone_wake]: new lint

should be good enough

@a1phyr a1phyr requested a review from y21 October 24, 2023 13:42
@a1phyr
Copy link
Contributor Author

a1phyr commented Oct 24, 2023

Isn't that the role of release ?

Where should I put that ? Section for 1.75 isn't there yet.

@y21
Copy link
Member

y21 commented Oct 24, 2023

Where should I put that ? Section for 1.75 isn't there yet.

Sorry, I meant the PR body. The text area where you're currently describing what the PR does.
image

The changelog entry should go there (doesn't really matter where I don't think, could put that at the bottom). This is all normally explained in the input box when you (are about to) open a PR, it might be that you skipped that part. :)
You can take a look at other open PR descriptions for how this should look like.

Isn't that the role of release ?

For every release, someone manually reviews all the changelog comments from all merged PRs in that timeframe and prepares the changelog.md for that release.
These changelog comments are automatically extracted by a script.
So, every PR must have a changelog: ... entry in the description, or else it can't be merged.
(That's how I understand it, anyway)

@a1phyr
Copy link
Contributor Author

a1phyr commented Oct 24, 2023

Done!

tests/ui/waker_clone_wake.rs Show resolved Hide resolved
clippy_lints/src/methods/waker_clone_wake.rs Outdated Show resolved Hide resolved
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

This looks good to me. cc @Alexendoo

@Alexendoo
Copy link
Member

Looks good, thanks!

@bors r=y21

@bors
Copy link
Contributor

bors commented Oct 26, 2023

📌 Commit ebf6667 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 26, 2023

⌛ Testing commit ebf6667 with merge 2f0f4dd...

@bors
Copy link
Contributor

bors commented Oct 26, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 2f0f4dd to master...

@bors bors merged commit 2f0f4dd into rust-lang:master Oct 26, 2023
8 checks passed
@y21
Copy link
Member

y21 commented Oct 26, 2023

Thanks for implementing this lint @a1phyr! :)

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.

6 participants