Skip to content

collapsible_if: split collapsible_else_if into its own lint so we can enable/disable it particularly #6544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 4, 2021
Merged

collapsible_if: split collapsible_else_if into its own lint so we can enable/disable it particularly #6544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1896,6 +1896,7 @@ Released 2018-09-13
[`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null
[`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned
[`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
[`collapsible_else_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
[`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
[`collapsible_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match
[`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
Expand Down
44 changes: 30 additions & 14 deletions clippy_lints/src/collapsible_if.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@ use rustc_errors::Applicability;

declare_clippy_lint! {
/// **What it does:** Checks for nested `if` statements which can be collapsed
/// by `&&`-combining their conditions and for `else { if ... }` expressions
/// that
/// can be collapsed to `else if ...`.
/// by `&&`-combining their conditions.
///
/// **Why is this bad?** Each `if`-statement adds one level of nesting, which
/// makes code look more complex than it really is.
Expand All @@ -40,7 +38,31 @@ declare_clippy_lint! {
/// }
/// }
///
/// // or
/// ```
///
/// Should be written:
///
/// ```rust.ignore
/// if x && y {
/// …
/// }
/// ```
pub COLLAPSIBLE_IF,
style,
"nested `if`s that can be collapsed (e.g., `if x { if y { ... } }`"
}

declare_clippy_lint! {
/// **What it does:** Checks for collapsible `else { if ... }` expressions
/// that can be collapsed to `else if ...`.
///
/// **Why is this bad?** Each `if`-statement adds one level of nesting, which
/// makes code look more complex than it really is.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust,ignore
///
/// if x {
/// …
Expand All @@ -54,24 +76,18 @@ declare_clippy_lint! {
/// Should be written:
///
/// ```rust.ignore
/// if x && y {
/// …
/// }
///
/// // or
///
/// if x {
/// …
/// } else if y {
/// …
/// }
/// ```
pub COLLAPSIBLE_IF,
pub COLLAPSIBLE_ELSE_IF,
style,
Comment on lines +85 to 86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the split out lint in the style category will result in people having to allow this lint again in their code, if they don't like it.

I guess the argument that this is fine, is that this case of the lint is easier to fix than the other case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, hopefully it won't be too annoying. :/
I also think that the if {} else { if {} } warning is actually more useful than the if { if {} } warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at your Rust PR, where you linked to this, it seems Joshua also agrees. So I'm ok with this.

"`if`s that can be collapsed (e.g., `if x { if y { ... } }` and `else { if x { ... } }`)"
"nested `else`-`if` expressions that can be collapsed (e.g., `else { if x { ... } }`)"
}

declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF]);
declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]);

impl EarlyLintPass for CollapsibleIf {
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
Expand Down Expand Up @@ -112,7 +128,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
COLLAPSIBLE_IF,
COLLAPSIBLE_ELSE_IF,
block.span,
"this `else { if .. }` block can be collapsed",
"collapse nested if block",
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&cargo_common_metadata::CARGO_COMMON_METADATA,
&checked_conversions::CHECKED_CONVERSIONS,
&cognitive_complexity::COGNITIVE_COMPLEXITY,
&collapsible_if::COLLAPSIBLE_ELSE_IF,
&collapsible_if::COLLAPSIBLE_IF,
&collapsible_match::COLLAPSIBLE_MATCH,
&comparison_chain::COMPARISON_CHAIN,
Expand Down Expand Up @@ -1384,6 +1385,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&booleans::LOGIC_BUG),
LintId::of(&booleans::NONMINIMAL_BOOL),
LintId::of(&bytecount::NAIVE_BYTECOUNT),
LintId::of(&collapsible_if::COLLAPSIBLE_ELSE_IF),
LintId::of(&collapsible_if::COLLAPSIBLE_IF),
LintId::of(&collapsible_match::COLLAPSIBLE_MATCH),
LintId::of(&comparison_chain::COMPARISON_CHAIN),
Expand Down Expand Up @@ -1653,6 +1655,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&attrs::UNKNOWN_CLIPPY_LINTS),
LintId::of(&blacklisted_name::BLACKLISTED_NAME),
LintId::of(&blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
LintId::of(&collapsible_if::COLLAPSIBLE_ELSE_IF),
LintId::of(&collapsible_if::COLLAPSIBLE_IF),
LintId::of(&collapsible_match::COLLAPSIBLE_MATCH),
LintId::of(&comparison_chain::COMPARISON_CHAIN),
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/collapsible_else_if.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#[rustfmt::skip]
#[warn(clippy::collapsible_if)]
#[warn(clippy::collapsible_else_if)]

fn main() {
let x = "hello";
let y = "world";
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/collapsible_else_if.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#[rustfmt::skip]
#[warn(clippy::collapsible_if)]
#[warn(clippy::collapsible_else_if)]

fn main() {
let x = "hello";
let y = "world";
Expand Down
16 changes: 8 additions & 8 deletions tests/ui/collapsible_else_if.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:12:12
--> $DIR/collapsible_else_if.rs:14:12
|
LL | } else {
| ____________^
Expand All @@ -9,7 +9,7 @@ LL | | }
LL | | }
| |_____^
|
= note: `-D clippy::collapsible-if` implied by `-D warnings`
= note: `-D clippy::collapsible-else-if` implied by `-D warnings`
help: collapse nested if block
|
LL | } else if y == "world" {
Expand All @@ -18,7 +18,7 @@ LL | }
|

error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:20:12
--> $DIR/collapsible_else_if.rs:22:12
|
LL | } else {
| ____________^
Expand All @@ -36,7 +36,7 @@ LL | }
|

error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:28:12
--> $DIR/collapsible_else_if.rs:30:12
|
LL | } else {
| ____________^
Expand All @@ -59,7 +59,7 @@ LL | }
|

error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:39:12
--> $DIR/collapsible_else_if.rs:41:12
|
LL | } else {
| ____________^
Expand All @@ -82,7 +82,7 @@ LL | }
|

error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:50:12
--> $DIR/collapsible_else_if.rs:52:12
|
LL | } else {
| ____________^
Expand All @@ -105,7 +105,7 @@ LL | }
|

error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:61:12
--> $DIR/collapsible_else_if.rs:63:12
|
LL | } else {
| ____________^
Expand All @@ -128,7 +128,7 @@ LL | }
|

error: this `else { if .. }` block can be collapsed
--> $DIR/collapsible_else_if.rs:72:12
--> $DIR/collapsible_else_if.rs:74:12
|
LL | } else {
| ____________^
Expand Down
1 change: 1 addition & 0 deletions tests/ui/if_same_then_else2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#![warn(clippy::if_same_then_else)]
#![allow(
clippy::blacklisted_name,
clippy::collapsible_else_if,
clippy::collapsible_if,
clippy::ifs_same_cond,
clippy::needless_return,
Expand Down
24 changes: 12 additions & 12 deletions tests/ui/if_same_then_else2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this `if` has identical blocks
--> $DIR/if_same_then_else2.rs:20:12
--> $DIR/if_same_then_else2.rs:21:12
|
LL | } else {
| ____________^
Expand All @@ -13,7 +13,7 @@ LL | | }
|
= note: `-D clippy::if-same-then-else` implied by `-D warnings`
note: same as this
--> $DIR/if_same_then_else2.rs:11:13
--> $DIR/if_same_then_else2.rs:12:13
|
LL | if true {
| _____________^
Expand All @@ -26,7 +26,7 @@ LL | | } else {
| |_____^

error: this `if` has identical blocks
--> $DIR/if_same_then_else2.rs:34:12
--> $DIR/if_same_then_else2.rs:35:12
|
LL | } else {
| ____________^
Expand All @@ -36,7 +36,7 @@ LL | | }
| |_____^
|
note: same as this
--> $DIR/if_same_then_else2.rs:32:13
--> $DIR/if_same_then_else2.rs:33:13
|
LL | if true {
| _____________^
Expand All @@ -45,7 +45,7 @@ LL | | } else {
| |_____^

error: this `if` has identical blocks
--> $DIR/if_same_then_else2.rs:41:12
--> $DIR/if_same_then_else2.rs:42:12
|
LL | } else {
| ____________^
Expand All @@ -55,7 +55,7 @@ LL | | }
| |_____^
|
note: same as this
--> $DIR/if_same_then_else2.rs:39:13
--> $DIR/if_same_then_else2.rs:40:13
|
LL | if true {
| _____________^
Expand All @@ -64,7 +64,7 @@ LL | | } else {
| |_____^

error: this `if` has identical blocks
--> $DIR/if_same_then_else2.rs:91:12
--> $DIR/if_same_then_else2.rs:92:12
|
LL | } else {
| ____________^
Expand All @@ -74,7 +74,7 @@ LL | | };
| |_____^
|
note: same as this
--> $DIR/if_same_then_else2.rs:89:21
--> $DIR/if_same_then_else2.rs:90:21
|
LL | let _ = if true {
| _____________________^
Expand All @@ -83,7 +83,7 @@ LL | | } else {
| |_____^

error: this `if` has identical blocks
--> $DIR/if_same_then_else2.rs:98:12
--> $DIR/if_same_then_else2.rs:99:12
|
LL | } else {
| ____________^
Expand All @@ -93,7 +93,7 @@ LL | | }
| |_____^
|
note: same as this
--> $DIR/if_same_then_else2.rs:96:13
--> $DIR/if_same_then_else2.rs:97:13
|
LL | if true {
| _____________^
Expand All @@ -102,7 +102,7 @@ LL | | } else {
| |_____^

error: this `if` has identical blocks
--> $DIR/if_same_then_else2.rs:123:12
--> $DIR/if_same_then_else2.rs:124:12
|
LL | } else {
| ____________^
Expand All @@ -112,7 +112,7 @@ LL | | }
| |_____^
|
note: same as this
--> $DIR/if_same_then_else2.rs:120:20
--> $DIR/if_same_then_else2.rs:121:20
|
LL | } else if true {
| ____________________^
Expand Down