Skip to content

Commit 2db699b

Browse files
committed
hir-ty: simplify unused_must_use container walk per review
Address ChayimFriedman2 review feedback on #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>
1 parent 68efe3e commit 2db699b

1 file changed

Lines changed: 33 additions & 45 deletions

File tree

  • crates/hir-ty/src/diagnostics

crates/hir-ty/src/diagnostics/expr.rs

Lines changed: 33 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -330,21 +330,21 @@ impl<'db> ExprValidator<'db> {
330330
let (Expr::Block { statements, .. } | Expr::Unsafe { statements, .. }) = expr else {
331331
return;
332332
};
333+
if self.validate_lints {
334+
for stmt in &**statements {
335+
if let Statement::Expr { expr: stmt_expr, has_semi: true } = *stmt {
336+
self.check_unused_must_use(stmt_expr);
337+
}
338+
}
339+
}
333340
let pattern_arena = Arena::new();
334341
let cx = MatchCheckCtx::new(self.owner.module(self.db()), &self.infcx, self.env);
335342
for stmt in &**statements {
336-
match *stmt {
337-
Statement::Expr { expr: stmt_expr, has_semi: true } if self.validate_lints => {
338-
self.diagnostics.extend(self.check_unused_must_use(stmt_expr));
339-
}
340-
Statement::Let { pat, initializer, else_branch: None, .. } => {
341-
if let Some(diag) =
342-
self.check_non_exhaustive_let(&cx, &pattern_arena, pat, initializer)
343-
{
344-
self.diagnostics.push(diag);
345-
}
346-
}
347-
_ => {}
343+
if let Statement::Let { pat, initializer, else_branch: None, .. } = *stmt
344+
&& let Some(diag) =
345+
self.check_non_exhaustive_let(&cx, &pattern_arena, pat, initializer)
346+
{
347+
self.diagnostics.push(diag);
348348
}
349349
}
350350
}
@@ -416,43 +416,31 @@ impl<'db> ExprValidator<'db> {
416416
pattern
417417
}
418418

419-
fn check_unused_must_use(&self, expr: ExprId) -> Vec<BodyValidationDiagnostic<'db>> {
420-
let mut diags = Vec::new();
421-
self.collect_unused_must_use(expr, &mut diags);
422-
diags
423-
}
424-
425-
fn collect_unused_must_use(
426-
&self,
427-
expr: ExprId,
428-
diags: &mut Vec<BodyValidationDiagnostic<'db>>,
429-
) {
419+
fn check_unused_must_use(&mut self, mut expr: ExprId) {
430420
// Walk through container expressions so that the value-producing leaf is
431421
// checked even when wrapped in a block, `unsafe { .. }`, `if`/`match`, or
432-
// a `const { .. }` block.
433-
match &self.body[expr] {
434-
Expr::Block { tail: Some(tail), .. } | Expr::Unsafe { tail: Some(tail), .. } => {
435-
self.collect_unused_must_use(*tail, diags);
436-
return;
437-
}
438-
Expr::If { then_branch, else_branch, .. } => {
439-
self.collect_unused_must_use(*then_branch, diags);
440-
if let Some(else_branch) = else_branch {
441-
self.collect_unused_must_use(*else_branch, diags);
422+
// a `const { .. }` block. Single-tail chains are followed by reassigning
423+
// `expr`; branching containers (`if`/`match`) recurse on each arm.
424+
loop {
425+
match &self.body[expr] {
426+
Expr::Block { tail: Some(tail), .. }
427+
| Expr::Unsafe { tail: Some(tail), .. }
428+
| Expr::Const(tail) => expr = *tail,
429+
Expr::If { then_branch, else_branch, .. } => {
430+
self.check_unused_must_use(*then_branch);
431+
if let Some(else_branch) = else_branch {
432+
self.check_unused_must_use(*else_branch);
433+
}
434+
return;
442435
}
443-
return;
444-
}
445-
Expr::Match { arms, .. } => {
446-
for arm in arms.iter() {
447-
self.collect_unused_must_use(arm.expr, diags);
436+
Expr::Match { arms, .. } => {
437+
for arm in arms.iter() {
438+
self.check_unused_must_use(arm.expr);
439+
}
440+
return;
448441
}
449-
return;
450-
}
451-
Expr::Const(inner) => {
452-
self.collect_unused_must_use(*inner, diags);
453-
return;
442+
_ => break,
454443
}
455-
_ => {}
456444
}
457445

458446
let fn_def = match &self.body[expr] {
@@ -480,7 +468,7 @@ impl<'db> ExprValidator<'db> {
480468
.map(|message| BodyValidationDiagnostic::UnusedMustUse { expr, message })
481469
};
482470
if let Some(diag) = must_use_diag(fn_def).or_else(|| must_use_diag(ty_def)) {
483-
diags.push(diag);
471+
self.diagnostics.push(diag);
484472
}
485473
}
486474

0 commit comments

Comments
 (0)