hir-ty: walk container exprs for unused_must_use#22405
Open
MavenRain wants to merge 1 commit into
Open
Conversation
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
requested changes
May 19, 2026
| fn collect_unused_must_use( | ||
| &self, | ||
| expr: ExprId, | ||
| diags: &mut Vec<BodyValidationDiagnostic<'db>>, |
Contributor
There was a problem hiding this comment.
You can just push to self.diagnostics.
| // Walk through container expressions so that the value-producing leaf is | ||
| // checked even when wrapped in a block, `unsafe { .. }`, `if`/`match`, or | ||
| // a `const { .. }` block. | ||
| match &self.body[expr] { |
Contributor
There was a problem hiding this comment.
Or better, there is no need for a recursion here, given a block has up to one tail expression. Instead loop until you reach it.
MavenRain
added a commit
to MavenRain/rust-analyzer
that referenced
this pull request
May 20, 2026
Address ChayimFriedman2 review feedback on rust-lang#22405: * Push directly to self.diagnostics rather than threading a &mut Vec parameter through the recursion. Drops the check_unused_must_use / collect_unused_must_use split and the allocation in the thin wrapper. * Replace the recursive single-tail-chain walk (Block/Unsafe/Const) with a loop that reassigns expr until reaching a non-chain leaf. Branching containers (If/Match) still recurse since they have multiple arms. The borrow checker required hoisting the Statement::Expr pass out of the main loop in validate_block, since MatchCheckCtx holds &self.infcx and the must_use pass now needs &mut self. The two passes are independent (must_use on expression statements, non_exhaustive_let on let statements) so order is free. 19 unused_must_use tests + 981 hir-ty tests + 693 ide-diagnostics tests pass. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
This comment has been minimized.
This comment has been minimized.
MavenRain
added a commit
to MavenRain/rust-analyzer
that referenced
this pull request
May 21, 2026
Address ChayimFriedman2 review feedback on rust-lang#22405: * Push directly to self.diagnostics rather than threading a &mut Vec parameter through the recursion. Drops the check_unused_must_use / collect_unused_must_use split and the allocation in the thin wrapper. * Replace the recursive single-tail-chain walk (Block/Unsafe/Const) with a loop that reassigns expr until reaching a non-chain leaf. Branching containers (If/Match) still recurse since they have multiple arms. The borrow checker required hoisting the Statement::Expr pass out of the main loop in validate_block, since MatchCheckCtx holds &self.infcx and the must_use pass now needs &mut self. The two passes are independent (must_use on expression statements, non_exhaustive_let on let statements) so order is free. 19 unused_must_use tests + 981 hir-ty tests + 693 ide-diagnostics tests pass. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
MavenRain
force-pushed
the
feat-must-use-container-exprs
branch
from
2db699b to
ffe5641
Compare
ChayimFriedman2
requested changes
May 25, 2026
| let (Expr::Block { statements, .. } | Expr::Unsafe { statements, .. }) = expr else { | ||
| return; | ||
| }; | ||
| if self.validate_lints { |
Contributor
There was a problem hiding this comment.
Why did you separate to a new loop? And why this if?
Contributor
Author
There was a problem hiding this comment.
Good catch . . . the split was a workaround: making check_unused_must_use take &mut self so it could push to self.diagnostics directly meant it could no longer run in the same loop as the MatchCheckCtx (which borrows &self.infcx). I've reverted it to &self with a small accumulator the caller drains, so both checks share the single loop again and validate_lints is back on the match arm.
ChayimFriedman2
requested changes
May 30, 2026
| } | ||
|
|
||
| #[test] | ||
| fn allow_attribute_on_fn_silences_lint() { |
Contributor
There was a problem hiding this comment.
We already have tests for lint attributes, we don't need to test them for every lint. Please remove those tests.
Contributor
Author
|
Done: removed the four allow_/deny_ lock-in tests; kept only the container-expression behavior tests. Agreed the generic lint-attribute handling is already covered and doesn't need re-testing per lint. |
ChayimFriedman2
approved these changes
Jun 5, 2026
ExprValidator::check_unused_must_use previously matched only Expr::Call and Expr::MethodCall directly on the expression of a Statement::Expr with semicolon. This left several stmt-with-semi cases unwarned: blocks whose tail is a #[must_use] call, unsafe-blocks, if/else arms, match arms, and const blocks.
Refactors check_unused_must_use to walk into Expr::Block { tail }, Expr::Unsafe { tail }, Expr::If { then_branch, else_branch }, Expr::Match { arms }, and Expr::Const(inner) before applying the existing leaf check (callee or method #[must_use] attribute, or #[must_use] ADT return type). The diagnostic stays at the leaf call so the span points to the actual must_use producer rather than the wrapping block.
Adds ide-diagnostics tests covering each new container case.
Follow-up to rust-lang#22239.
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
MavenRain
force-pushed
the
feat-must-use-container-exprs
branch
from
2632230 to
c859808
Compare
Contributor
Author
|
@ChayimFriedman2 CI is red solely from the cancelled powerpc + wasm32); all 13 real jobs are green; could you re-run the cross job or merge past it (contributors can't re-trigger upstream CI)? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ExprValidator::check_unused_must_use previously matched only Expr::Call and Expr::MethodCall directly on the expression of a Statement::Expr with semicolon. This left several stmt-with-semi cases unwarned: blocks whose tail is a #[must_use] call (
{ produces() };), unsafe-blocks (unsafe { produces() };), if/else arms, match arms, and const blocks.Refactors check_unused_must_use into a recursive collector that walks into Expr::Block { tail, .. }, Expr::Unsafe { tail, .. }, Expr::If { then_branch, else_branch, .. }, Expr::Match { arms, .. }, and Expr::Const(inner) before applying the existing leaf check (callee or method #[must_use] attribute, or #[must_use] ADT return type). The diagnostic stays at the leaf call so the span points to the actual must_use producer rather than the wrapping block.
Adds ide-diagnostics tests covering each new container case (block tail, unsafe block tail, nested blocks, if/else branches, match arms, const block, #[must_use] ADT through a block) and lock-in tests confirming that the existing RustcLint plumbing already honors #[allow(unused_must_use)], #[allow(unused)], and #[deny(unused_must_use)] attribute resolution.
Follow-up to #22239.