Conversation
|
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
rustbot
added
the
S-waiting-on-review
| 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) |
There was a problem hiding this comment.
Should this (and the similar line in book/src/README.md) be captured in this patch?
There was a problem hiding this comment.
Wdym by captured? cargo dev update_lints automatically updates this and fails CI if this lint count is not up to date.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
|
☔ The latest upstream changes (presumably #13376) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
rustbot
added
S-waiting-on-author
y21
added
S-waiting-on-review
|
Ping @Alexendoo from triage. This is still waiting on a review. |
|
Ping @y21 do you still have time to work on this? r? Jarcho |
Oh yeah, sorry. This is gonna need to be rebased and probably cleaned up a little from being over a year behind master but I'll try to do that over the weekend. |
68e9a60 to
1fcc911
Compare
rustbot
added
the
needs-fcp
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
1fcc911 to
fed638d
Compare
|
@rustbot label lint-nominated |
|
This lint has been nominated for inclusion. |
| match cx.tcx.parent_hir_node(expr.hir_id) { | ||
| Node::Expr(as_mut_expr) | ||
| if let ExprKind::MethodCall(segment, recv, [], span) = as_mut_expr.kind | ||
| && recv.hir_id == expr.hir_id |
There was a problem hiding this comment.
This isn't needed. There's only one expression which can be a direct child.
| for_each_local_use_after_expr(cx, local_id, expr.hir_id, |expr| { | ||
| if check_pin_box_use(cx, expr, true, enclosing_body).is_continue() | ||
| // Make sure the `Pin` is not captured by a closure. | ||
| && cx.tcx.hir_enclosing_body_owner(expr.hir_id) == enclosing_body |
There was a problem hiding this comment.
Why are closures not fine?
| fn check_pin_box_use<'tcx>( | ||
| cx: &LateContext<'tcx>, | ||
| expr: &'tcx Expr<'tcx>, | ||
| moved: bool, |
There was a problem hiding this comment.
This just makes things harder to read. Inlining just the relevant part into the for_each_local closure would be nicer. It adds two lines to the closure, but getting rid of the mode switch is worth a fair bit more.
for_each_local_use_after_expr(cx, local_id, expr.hir_id, |expr| {
if let Node::Expr(parent) = cx.tcx.parent_hir_node(expr.hir_id)
&& let ExprKind::MethodCall(segment, _, [], _) = parent.kind
&& segment.ident.name == sym::as_mut
// Make sure the `Pin` is not captured by a closure.
&& cx.tcx.hir_enclosing_body_owner(expr.hir_id) == enclosing_body
{
ControlFlow::Continue(())
} else {
ControlFlow::Break(())
}
})?;
rustbot
added
S-waiting-on-author
|
☔ The latest upstream changes (possibly #16953) made this pull request unmergeable. Please resolve the merge conflicts. |
This branch has not been deployed
6 participants
This adds a new lint that looks for
Box::pincalls that could be replaced with thepin!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