Ban Stacked `if`'s

[bushrat011899]

What it does

This lint should prohibit placing bool returning if statements as the expression inside another if statement. Currently, there appears to be no limit to the depth of stacked if's permitted by both rust fmt and clippy

Advantage

• Stacked if's are hard to read

Drawbacks

• Ternary-style expressions couldn't be used inside an if statement, but I personally think that's poor form

Example

if if if a == b {
    b == c
} else {
    a == c
} {
    a == d
} else {
    c == d
} {
    println!("True!");
} else {
    println!("False!");
}

Could be written as:

let expression_1 = if a == b {
    b == c
} else {
    a == c
};

let expression_2 = if expression_1 {
    a == d
} else {
    c == d
};

if expression_2  {
    println!("True!");
} else {
    println!("False!");
}

[popematt]

I don't think it should be limited to if expressions that return bool. All if expressions that are nested in an if condition are fishy. I think things like this should be banned too.

if if a == b {
    "a"
} else {
    "b"
} == "a" {
    "c"
} else {
    "d"
}

But a partial solution is still helpful.

[bushrat011899]

I hadn't considered that, and I agree, it looks horrible. Perhaps if clippy ever sees if if that's the sign that something is wrong.

[Centri3]

This comes up occasionally. We'd probably want to suggest extracting it to a variable even though it won't be reached very often. (Also note that we don't have to entirely clean up in one run - just peeling one layer at a time is okay.)

Note that match match exists too.

Beware drop temps - check for If(DropTemps(If(...))) instead of 2 Ifs.

[Centri3]

Oh also, blocks in the condition of an if, while, etc., are pretty bad too usually. Could be a separate lint thugh

[workingjubilee]

@Centri3 I would greatly prefer that if { cond } { expr } else { expr } is not linted on except in a form of pedantic lint. While it sometimes is a bit peculiar to find, it often is the case that it looks more like if cond && { subcond } || alt_cond { }; or similar where it does not greatly affect readability because it does not wrap the entire condition so it is still clear where the condition begins and ends. There are patterns that are hard to express without (trivial, sometimes even single-expression) blocks, but which do not significantly affect readability. The braces should not be treated as more peculiar than parentheses.

In particular, my understanding is lints, including clippy lints, also fire on macro-generated code after all, and macros have to use lots of blocks and matches. It would be very annoying to have to also emit #[allow(clippy::whatever] each and every time we do something extremely normal in macros.

Returning to the main subject, if if is obviously extremely bad style. Even macros should avoid generating that if they can help it.

[Centri3]

I was thinking of longer blocks, not single expressions. { cond } is okay, it's hard to mix it up with the branch then, while something that spans multiple lines is harder to read and understand at first than if it was refactored. I see that come up pretty often (sometimes as do while loops). Also if unsafe {} or if match {} would be linted in that case but these are okay since they're single expressions.

Pedantic was what I was thinking. Macros aren't a worry and we usually don't lint in them (unless it's a logic bug/UB)

[epidemian]

I was going to ask "is this something that could realistically happen in the wild?". A cursory search for "if if " suggests that yes, it is indeed something that happens every now and then. So i think this lint would make sense :)

[ARandomDev99]

@rustbot claim

[lolbinarycat]

if (if a == b { b == c }
	else { a == c })
{
    a == d
} else {
    println!("True!");
}

perhaps there should be a configurable maximum level, i don't think single stacked ifs are too hard to read if you format them like a ternary.