Lint more cases in `collapsible_if` #14231

samueltardieu · 2025-02-16T15:09:15Z

Replace the use of Sugg::ast() which prevented combining if together when they contained comments by span manipulation.

A new configuration option lint_commented_code, which is true by default, allows opting out from this behavior.

If reviewed on GitHub, the second commit of this PR is best looked at side by side, with whitespace differences turned off.

changelog: [collapsible_if]: lint more cases

rustbot · 2025-02-16T15:09:19Z

r? @llogiq

rustbot has assigned @llogiq.
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

samueltardieu · 2025-02-18T07:55:51Z

I think I'll add a configuration option to merge if containing comments, as some people might not like it.

@rustbot author

samueltardieu · 2025-02-18T11:52:37Z

Done, with the lint_commented_code option, true by default.

@rustbot review

samueltardieu · 2025-02-28T23:02:02Z

Rebased

samueltardieu · 2025-03-04T08:32:37Z

Rebased

samueltardieu · 2025-03-12T08:39:21Z

r? clippy

samueltardieu · 2025-03-20T20:19:50Z

Rebased after rustup

flip1995

LGTM overall, just a few NITs. After a full review, I noticed that the comment is kept, so defaulting the config to true is more reasonable. I would still be a bit more conservative and set it to false for this PR, in order to get it merged more quickly and then revisit it in a later PR.

flip1995 · 2025-03-22T12:26:48Z

clippy_config/src/conf.rs

+    /// Whether collapsible `if` chains are linted if they contain comments inside the parts
+    /// that would be collapsed.
+    #[lints(collapsible_if)]
+    lint_commented_code: bool = true,


Suggested change

lint_commented_code: bool = true,

lint_commented_code: bool = false,

If there is a comment between the 2 if stmts, it is a strong indication that the separation is intentional and for readability.

flip1995 · 2025-03-22T12:28:18Z

clippy_lints/src/collapsible_if.rs

 }
+
+/// If the expression is a `||`, suggest parentheses around it.
+fn par_around_or(expr: &ast::Expr) -> Vec<(Span, String)> {


NIT: parens is a more common abbreviation of parentheses.

Suggested change

fn par_around_or(expr: &ast::Expr) -> Vec<(Span, String)> {

fn parens_around_or(expr: &ast::Expr) -> Vec<(Span, String)> {

Done. I think I played with Sugg::maybe_par() first, and kept this abbreviation. I'll add checking if proposing to rename this to Sugg::maybe_paren() to my TODO list.

Replace the use of `Sugg::ast()` which prevented combining `if` together when they contained comments by span manipulation. A new configuration option `lint_commented_code`, which is `true` by default, opts out from this behavior.

samueltardieu · 2025-03-22T14:27:03Z

LGTM overall, just a few NITs. After a full review, I noticed that the comment is kept, so defaulting the config to true is more reasonable. I would still be a bit more conservative and set it to false for this PR, in order to get it merged more quickly and then revisit it in a later PR.

I've set the default to false for the moment, and rebased the patch to be able to set it to true for linting Clippy code and back to false when running lintcheck in the CI.

samueltardieu · 2025-03-22T14:32:55Z

Interesting: the lintcheck test fails, because in the base test the option does not exist yet and thus cannot be overriden in the PR that introduces it. I'll have to split linting Clippy code itself with this new option into its own PR.

flip1995

Thanks!

flip1995 · 2025-03-23T11:17:40Z

r? @flip1995

This PR enables the new ability to collapse `if` statements containing comments (without losing them) in Clippy sources, excluding tests and lintcheck, where the default behaviour (no collapsing in presence of comments) is preserved. To be applied after #14231. When it is applied, #14455 will be marked as ready for review, then #14228 afterwards. changelog: none r? ghost

rustbot assigned llogiq Feb 16, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2025

samueltardieu mentioned this pull request Feb 16, 2025

Collapse if and if let chains in Clippy itself #14228

Closed

samueltardieu force-pushed the push-uopxwlslsuwp branch 2 times, most recently from b8d9f37 to a94f321 Compare February 16, 2025 15:32

rustbot 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 Feb 18, 2025

samueltardieu force-pushed the push-uopxwlslsuwp branch from a94f321 to bc44671 Compare February 18, 2025 11:52

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

samueltardieu force-pushed the push-uopxwlslsuwp branch 2 times, most recently from 419a0a2 to 24c2681 Compare February 20, 2025 16:52

samueltardieu force-pushed the push-uopxwlslsuwp branch 2 times, most recently from 6d030ab to f065214 Compare February 28, 2025 00:42

samueltardieu changed the title ~~Lint more cases in collapsible_if, without suggestions~~ Lint more cases in collapsible_if Feb 28, 2025

samueltardieu force-pushed the push-uopxwlslsuwp branch from f065214 to 14aa958 Compare February 28, 2025 23:01

samueltardieu force-pushed the push-uopxwlslsuwp branch from 14aa958 to affd2a7 Compare March 4, 2025 08:32

rustbot assigned Jarcho and unassigned llogiq Mar 12, 2025

samueltardieu force-pushed the push-uopxwlslsuwp branch from affd2a7 to 43b8c88 Compare March 20, 2025 20:19

flip1995 approved these changes Mar 22, 2025

View reviewed changes

Lint more cases in collapsible_if

8238160

Replace the use of `Sugg::ast()` which prevented combining `if` together when they contained comments by span manipulation. A new configuration option `lint_commented_code`, which is `true` by default, opts out from this behavior.

samueltardieu force-pushed the push-uopxwlslsuwp branch from 43b8c88 to 9c39dc5 Compare March 22, 2025 14:24

samueltardieu force-pushed the push-uopxwlslsuwp branch from 9c39dc5 to 8238160 Compare March 22, 2025 14:38

samueltardieu mentioned this pull request Mar 22, 2025

Apply collapsible if to Clippy sources #14451

Merged

flip1995 approved these changes Mar 23, 2025

View reviewed changes

rustbot assigned flip1995 and unassigned Jarcho Mar 23, 2025

flip1995 added this pull request to the merge queue Mar 23, 2025

Merged via the queue into rust-lang:master with commit cad9083 Mar 23, 2025
11 checks passed

samueltardieu deleted the push-uopxwlslsuwp branch March 23, 2025 17:50

ada4a mentioned this pull request Aug 24, 2025

collapsible_if lint is confused by comments and sometimes deletes them with --fix #9232

Open

	lint_commented_code: bool = true,
	lint_commented_code: bool = false,

	fn par_around_or(expr: &ast::Expr) -> Vec<(Span, String)> {
	fn parens_around_or(expr: &ast::Expr) -> Vec<(Span, String)> {

Lint more cases in collapsible_if #14231

Lint more cases in collapsible_if #14231

Uh oh!

Conversation

samueltardieu commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 16, 2025

Uh oh!

samueltardieu commented Feb 18, 2025

Uh oh!

samueltardieu commented Feb 18, 2025

Uh oh!

samueltardieu commented Feb 28, 2025

Uh oh!

samueltardieu commented Mar 4, 2025

Uh oh!

samueltardieu commented Mar 12, 2025

Uh oh!

samueltardieu commented Mar 20, 2025

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

flip1995 Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Mar 22, 2025

Uh oh!

samueltardieu commented Mar 22, 2025

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Mar 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Lint more cases in `collapsible_if` #14231

Lint more cases in `collapsible_if` #14231

samueltardieu commented Feb 16, 2025 •

edited

Loading