Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT #11314
Conversation
|
r? @Centri3 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
GuillaumeGomez
force-pushed
the
needless_ref_mut_async_block
branch
from
9683595
to
1d01f1b
Compare
Release-As: 0.1.24 Also ignore incorrect clippy `needless_pass_by_ref_mut` lint until rust-lang/rust-clippy#11314 is merged.
| ], | ||
| ), | ||
| .. | ||
| }) = self.tcx.hir().get(id) |
There was a problem hiding this comment.
I'm curious how stable diag_expr_id is. Is it free to change at any time if, perhaps, only highlighting the arguments of the closure is deemed better? Because using this to determine if it's from a closure feels a bit error-prone.
Maybe we need another parameter that's the borrower's HirId, or consumer, etc.
If it is ok, then the wording in consume should probably be updated in rustc since it's likely interpreted as the HirId for getting spans/the id for linting, not analysis.
There was a problem hiding this comment.
Sorry I'm a bit lost: what is diag_expr_id here? What's the connection with consume too?
In this case, async blocks generate a closure and an Expr containing a call to a closure which has a DefId and that's what I retrieve with this code. If this is changed in the compiler, the UI test will catch it.
There was a problem hiding this comment.
diag_expr_id i.e., id. That's the default name for it.
Its intended use is clarified in Delegate::consume's docs, from what I can tell it's not supposed to be used for analysis.
If this is changed in the compiler, the UI test will catch it.
That's true, but it'll be annoying for whoever wants to. If this is only supposed to be used pointing at something in diagnostics, then we probably shouldn't use it for this (at least, once we have an alternative). It would be reasonable for it to be changed, since if my assumption is true then code isn't supposed to depend on this (turning what would be a couple line change into adding a stable HirId you can use.)
There was a problem hiding this comment.
Oh ok I see. In this case, it's all generated by the compiler (it doesn't exist in user code). The case of async block seems to be special if you look at ExprKind::Closure. So what do you want to do in this case?
There was a problem hiding this comment.
It's probably fine as is, but once we can we should change it. For now however, we could change this to cmt.hir_id which would convey this better (diag_expr_id is currently always cmt.hir_id for borrow) as cmt (place_with_id) is usually where the binding was introduced I believe (in this case, the closure, though it would realistically be the parameters' HirId)
There was a problem hiding this comment.
That makes perfect sense, thanks for the detailed explanation!
Centri3
added
the
beta-nominated
|
Depending on whether this gets merged in the next hour or so, this could get into the backports I'm doing right now. Otherwise this will be backported in the next cycle. Both work out just as well, so no pressure. :) |
|
I updated the |
|
Guess now it is, then, thanks! @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
flip1995
removed
the
beta-nominated
|
Included in rust-lang/rust#114938 Not adding the beta-accepted label, since this fix will get into the same release cycle as the lint addition (1.73.0) |
|
It's already in stable (1.71.1) according to the playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cf8a9481e3df956c1a740d317f82967a Am I missing something? This should only be in nightly right now |
|
The playground clippy version is always the nightly version, no matter what Toolchain you select. I think this is true for all tools |
|
Oh
I have no idea how I never noticed that... |
|
Didn't know either. ^^' In any case, thanks for the review @Centri3 ! |
…_block, r=Centri3 Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT Fixes rust-lang#11299. The problem was that the `async block`s are popping a closure which we didn't go into, making it miss the mutable access to the variables. cc `@Centri3` changelog: none
Fixes #11299.
The problem was that the
async blocks are popping a closure which we didn't go into, making it miss the mutable access to the variables.cc @Centri3
changelog: none