Enhance never loop

[MassimilianoBaglioni]

changelog: [never_loop]: add notes for non trivial cases to indicate why a loop is detected as non terminating

With these changes the lint is able to report non trivial diverging expressions that do not allow the loop to ever execute one iteration. I interpreted “non-trivial” expressions as those not matched by the previous cases in never_loop_expr.

This is my first contribution to Clippy. I'm sorry if I misunderstood the issue or if the code is not good enough. Any feedback is appreciated, and I'm ready to implement any changes needed.

fixes #16056

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @samueltardieu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[samueltardieu]

This is too much work before we even know that the lint will trigger.

Instead of storing the non-obvious spans, you should store expr.hir_id (and rename the field to non_obvious_exprs). Then, when you know that the lint will trigger, you can convert each HirId to &Expr<'_> (by calling cx.tcx.hir_expect_expr(hir_id)) and compute the spans. This way, the lookup for non obvious spans only happens if the lint triggers.

View changes since the review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[samueltardieu]

Could you please squash all commits?

View changes since this review

[samueltardieu]

No need to materialize a Vec here, an Iterator will do:

Suggested change

	let non_obvious_spans: Vec<Span> = non_obvious_exprs
	.iter()
	.map(|hir_id| cx.tcx.hir_expect_expr(*hir_id))
	.flat_map(|expr| find_non_obvious_spans(cx, expr))
	.collect();
	let non_obvious_spans = non_obvious_exprs
	.iter()
	.map(|hir_id| cx.tcx.hir_expect_expr(*hir_id))
	.flat_map(|expr| find_non_obvious_spans(cx, expr));

[samueltardieu]

Creating a variable is a bit overkill here, it doesn't make the code clearer and breaks symmetry with never_spans.

Suggested change

	let non_obvious_exprs = vec![expr.hir_id];
	NeverLoopResult::Diverging {
	break_spans: vec![],
	never_spans: all_spans_after_expr(cx, expr),
	non_obvious_exprs,
	}
	NeverLoopResult::Diverging {
	break_spans: vec![],
	never_spans: all_spans_after_expr(cx, expr),
	non_obvious_exprs: vec![expr.hir_id],
	}

[samueltardieu]

How did you choose the list here? Can you add a comment so that future kinds can be added if needed?

I think I would have preferred if you listed the 27 missing variants instead of _ so that if a new ExprKind is added it has to be handled properly here.

[samueltardieu]

What are those comments?

[rustbot]

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.