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

new lint: unnecessary_box_pin #13598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

y21
Copy link
Member

@y21 y21 commented Oct 23, 2024

This adds a new lint that looks for Box::pin calls that could be replaced with the pin! macro.
I've found that there's a lot of potential for false positives here, so for now this is fairly conservative and only lints if all uses go through .as_mut(). It does mean that there's a few false negatives I can think of, but hopefully no FPs.

changelog: [unnecessary_box_pin]: new lint

@rustbot
Copy link
Collaborator

rustbot commented Oct 23, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 23, 2024
@@ -5,7 +5,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this (and the similar line in book/src/README.md) be captured in this patch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wdym by captured? cargo dev update_lints automatically updates this and fails CI if this lint count is not up to date.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I didn't know that the CI would fail. I would consider this a problem though, as several proposed PR contain this mandatory change right now, while others don't and might need it if they are to be merged first.

Wouldn't it be better if this was updated at release time rather than when adding a lint? (and yes, this is unrelated to this particular PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's as much of a problem. Whichever PR gets merged first "wins" and all other PRs can just drop their change during the rebase. New lint PRs getting merged often create merge conflicts on most other lint PRs anyway so you often have to rebase either way.
Old PRs that don't have the lint count updated but need it will still fail when r+'d as bors makes sure that CI still passes with the PR integrated into master, and fixing it is very easy with cargo dev update_lints.

It also happens rather infrequently (last change on that line was 10 months ago) that I don't know if it's worth complicating this process (this number replacement is very little code in update_lints which already handles all other kinds of content generation).

But if you want to discuss it more, I'd suggest opening a thread on Zulip

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.

4 participants