chore: multipart_suggestions for manual_assert

[scottgerring]

This should address #13099 for the let_unit test.

changelog: [manual_assert]: Updated manual_assert to use multipart_suggestions where appropriate

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottgerring]

I could use a pointer on this one. We're trying to avoid displaying any comments that are captured within the span including the if/assert block, and only retain them in the tool-applied suggestion:

rust-clippy/clippy_lints/src/manual_assert.rs

Lines 68 to 84 in f83a227

	span_lint_and_then(
	cx,
	MANUAL_ASSERT,
	expr.span,
	"only a `panic!` in `if`-then statement",
	|diag| {
	// comments can be noisy, do not show them to the user
	if !comments.is_empty() {
	diag.tool_only_span_suggestion(
	expr.span.shrink_to_lo(),
	"add comments back",
	comments,
	applicability,
	);
	}
	diag.span_suggestion(expr.span, "try instead", sugg, applicability);
	},

I've preserved this here by keeping the tool-only suggestion, and replacing only the span_suggestion with multipart. I believe this serves the intended purpose, but the kind of "undesired upshot" is that we end up with two different outputs from the test framework:

• manual_assert.editionX.1.fixed - user-facing suggestion only - replaces the if/assert, does not add comment back
• manual_assert.edition.X.2.fixed - in cases where there is a comment, runs the machine-suggestion only - essentially duplicating the comment, but not replacing the if/assert.

The second case fails the testing, because the if/assert has not been removed.

I think this is the correct behaviour from the runtime perspective, but because the testing framework automatically diverges when there are multiple suggestions, i'm stuck. Options I see:

• Stop trying to hide the comments from the user
• Leverage some mechanism to group both machine-only and regular changes together into one multipart suggestion
• Leverage some mechanism to tell the testing framework to ignore the .2. outputs

Any tips @y21 ?

[y21]

I suppose there would also be the option to have a single multipart suggestion that removes the if cond { + } and changes the panic! to assert!(cond. Then the comments in between the condition and the assert are naturally preserved and the lint wouldn't need to bother extracting them at all, but maybe that would look confusing. Just including the comments in the same suggestion also sounds fine to me though.

[scottgerring]

Cheers @y21 - i'll add them back in and see where we land 🙌

[scottgerring]

Hey @y21 , do you have a chance to look at this? We are inches away from finishing the whole issue 🤣

[y21]

Sorry for the delay, will try to get to this soon (probably this weekend). I have a bunch of other PRs + other stuff going on at the same time

[scottgerring]

Sorry for the delay, will try to get to this soon (probably this weekend). I have a bunch of other PRs + other stuff going on at the same time

Sorry I figured it might’ve fallen through the cracks! It’s probably not a high priority so no stress. Hope you can have a good break!

[y21]

LGTM, thank you for pushing this through!

[y21]

Looks like CI is failing with a compile error. The signature of reindent_multiline was changed recently so most likely you'll have to rebase it so see/fix this locally.

[y21]

While we're already here and have to change something anyway: since this base_sugg is only used once we could inline it into the format macro directly to get rid of an allocation

Suggested change

	let base_sugg = format!("assert!({not}{cond_sugg}, {format_args_snip}){semicolon}");
	let indent = indent_of(cx, expr.span);
	let full_sugg = reindent_multiline(format!("{comments}{base_sugg}").into(), true, indent);
	let indent = indent_of(cx, expr.span);
	let full_sugg = reindent_multiline(&*format!("{comments}assert!({not}{cond_sugg}, {format_args_snip}){semicolon}"), true, indent);

(may need some formatting changes if the line is too long)

---

Also, can you move all of this code that prepares/formats the suggestion into the span_lint_and_then closure? The lint is in pedantic, so often the lint isn't enabled and we don't need to emit/prepare any suggestion as the closure won't be called.

[Jarcho]

Ping @scottgerring from triage. Do you plan to return to working on this? Looks like it was almost ready.

[scottgerring]

hey @Jarcho sorry this dropped off my radar! I'll try rebase it this week.

[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.

[github-actions]

Lintcheck changes for 8ef9057

Lint	Added	Removed	Changed
clippy::manual_assert	0	0	70

This comment will be updated if you push new changes

[scottgerring]

Hey @Jarcho I reckon this is good to go now 🙏

[y21]

One minor nit I only now realized (sorry), other than that looks good to me. Could you also squash the commits the next push? Then this should be ready to merge

View changes since this review

[y21]

Actually, since this is just always a single suggestion now, can't we just use .span_suggestion() now (or rather .span_suggestion_verbose() to keep having it on a separate line)? That should be identical but removes the need for creating a vec here.

Suggested change

	diag.multipart_suggestion(
	"replace `if`-then-`panic!` with `assert!`",
	vec![(expr.span, full_sugg)],
	applicability,
	diag.span_suggestion_verbose(
	expr.span,
	"replace `if`-then-`panic!` with `assert!`",
	full_sugg,
	applicability,
	);

[scottgerring]

Hey @y21 im AFK on holidays at the moment, but I can certainly polish it when I’m back in a couple weeks 👌

[scottgerring]

@y21 - let's try again :D Squashed into a single commit, using span_suggestion_verbose.