chore: multipart_suggestions for manual_assert #13787

scottgerring · 2024-12-06T09:49:11Z

This should address #13099 for the let_unit test.

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

rustbot · 2024-12-06T09:49:17Z

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 · 2024-12-06T10:22:12Z

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 · 2024-12-10T20:36:50Z

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 · 2024-12-11T07:18:11Z

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

tests/ui/manual_assert.edition2018.stderr

scottgerring · 2024-12-17T15:24:31Z

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

y21 · 2024-12-17T19:46:00Z

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 · 2024-12-17T19:51:48Z

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!

clippy_lints/src/manual_assert.rs

y21

LGTM, thank you for pushing this through!

y21 · 2025-02-11T01:26:21Z

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 · 2025-02-11T01:33:22Z

clippy_lints/src/manual_assert.rs

+            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);


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 · 2025-09-17T02:45:08Z

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

scottgerring · 2025-09-17T05:56:06Z

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

rustbot · 2025-09-17T08:03:55Z

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 · 2025-09-17T08:36:07Z

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 · 2025-09-17T09:58:51Z

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 · 2025-09-26T10:33:27Z

clippy_lints/src/manual_assert.rs

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


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,

);

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

scottgerring · 2025-10-08T14:16:11Z

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

rustbot assigned y21 Dec 6, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 6, 2024

scottgerring force-pushed the chore/fix-manual-assert branch from a2618b9 to 09589ff Compare December 6, 2024 09:57

scottgerring force-pushed the chore/fix-manual-assert branch 3 times, most recently from bc9e5a6 to e146ead Compare December 11, 2024 10:16

scottgerring commented Dec 11, 2024

View reviewed changes

tests/ui/manual_assert.edition2018.stderr Show resolved Hide resolved

scottgerring marked this pull request as ready for review December 11, 2024 10:48

This was referenced Dec 18, 2024

chore: use multipart_suggestions for str_splitn #13789

Merged

Tracking Issue: Fix auto-applicable lint suggestions by using multipart suggestions #13099

Closed

y21 reviewed Dec 22, 2024

View reviewed changes

clippy_lints/src/manual_assert.rs Outdated Show resolved Hide resolved

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 31, 2025

This comment has been minimized.

Sign in to view

scottgerring force-pushed the chore/fix-manual-assert branch from 7a68957 to a805a6f Compare January 31, 2025 14:24

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 31, 2025

scottgerring force-pushed the chore/fix-manual-assert branch from a805a6f to 5e448ee Compare January 31, 2025 14:52

y21 approved these changes Feb 11, 2025

View reviewed changes

y21 added this pull request to the merge queue Feb 11, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2025

y21 reviewed Feb 11, 2025

View reviewed changes

y21 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 15, 2025

This comment has been minimized.

Sign in to view

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Sep 17, 2025

scottgerring force-pushed the chore/fix-manual-assert branch from 5d8489b to 5e448ee Compare September 17, 2025 07:56

This comment has been minimized.

Sign in to view

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 17, 2025

This comment has been minimized.

Sign in to view

scottgerring force-pushed the chore/fix-manual-assert branch from 5e448ee to 0f8c62c Compare September 17, 2025 08:03

scottgerring force-pushed the chore/fix-manual-assert branch 2 times, most recently from 3346a38 to 793c00a Compare September 17, 2025 08:26

y21 approved these changes Sep 26, 2025

View reviewed changes

chore: update manual_assert span suggestions

8ef9057

scottgerring force-pushed the chore/fix-manual-assert branch from 793c00a to 8ef9057 Compare October 8, 2025 10:59

y21 added this pull request to the merge queue Oct 8, 2025

Merged via the queue into rust-lang:master with commit e70b206 Oct 8, 2025
11 checks passed

rustbot mentioned this pull request Oct 8, 2025

fix(manual_assert): don't lint on if cfg!(..) { panic!(..) } #15651

Open

y21 mentioned this pull request Oct 8, 2025

fix(manual_assert): use multipart suggestion instead of simple span_suggestion #15226

Closed

scottgerring deleted the chore/fix-manual-assert branch October 9, 2025 12:29

chore: multipart_suggestions for manual_assert #13787

chore: multipart_suggestions for manual_assert #13787

Conversation

scottgerring commented Dec 6, 2024

Uh oh!

rustbot commented Dec 6, 2024

Uh oh!

scottgerring commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

y21 commented Dec 10, 2024

Uh oh!

scottgerring commented Dec 11, 2024

Uh oh!

Uh oh!

scottgerring commented Dec 17, 2024

Uh oh!

y21 commented Dec 17, 2024

Uh oh!

scottgerring commented Dec 17, 2024

Uh oh!

Uh oh!

This comment has been minimized.

y21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

y21 commented Feb 11, 2025

Uh oh!

y21 Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Jarcho commented Sep 17, 2025

Uh oh!

scottgerring commented Sep 17, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Sep 17, 2025

Uh oh!

github-actions bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottgerring commented Sep 17, 2025

Uh oh!

y21 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y21 Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

scottgerring Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

scottgerring commented Oct 8, 2025

Uh oh!

Uh oh!

Uh oh!

scottgerring commented Dec 6, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2025 •

edited

Loading

y21 left a comment •

edited by rustbot

Loading