Don't flag if_not_else when else block is longer than then block

[nickdrozd]

Consider the following if/else block:

if !failure_condition {
    proceed_as_normal();
} else {
    handle_error();
    clean_up();
    notify();
    etc();
}

Currently, clippy will flag this block with if_not_else and suggest
that it be rewritten as

if failure_condition {
    handle_error();
    clean_up();
    notify();
    etc();
} else {
    proceed_as_normal();
}

But top-heavy blocks are hard to read, and the suggested rewrite is
worse than the original. So if_not_else should only be raised when the
else block is no longer than the then block.

[oli-obk]

we could make this lint configurable over a ratio. I think something like 1:2 would still be ok as a default

[nickdrozd]

Yeah, I was thinking that too, but I don't know about the details. It wouldn't be hard to add in something like that later.

[oli-obk]

True, can you open an issue for adding such a ratio to our config?

[nickdrozd]

Will do.

Unrelated, but could you take a look at remacs/remacs@dee6781 and see if it takes care of remacs/remacs#757?

[nickdrozd]

Number of lines is also something to consider. For instance

                if self.q != 0 {
                    if self.tail == self.tortoise {
                        return self.circular();
                    }
                } else {
                    self.n = self.n.wrapping_sub(1);
                    if self.n > 0 {
                        if self.tail == self.tortoise {
                            return self.circular();
                        }
                    } else {
                        self.max <<= 1;
                        self.q = self.max as u16;
                        self.n = self.max >> 16;
                        self.tortoise = self.tail;
                    }

There are just two statements in the else block (assuming the if counts as one), but the whole block is pretty long, and I wouldn't want to switch those.

[sanmai-NL]

I think a needless negation is a needless negation. The motivation for applying this lint is in my opinion not to make the code optimally readable according to a subjective criterion such as conditional clause length in number of lines. That would be besides the point. The real point is having a consistent pattern in conditional logic, one that uses the simplest conditional expressions possible, meaning with as few logical operators as possible. This leads me to conclude that this proposal is undesirable as is.

[oli-obk]

I think a needless negation is a needless negation.

Sure, and the default should stay exactly this way. But this is not the first time users have expressed their dissatisfaction with such a black and white rule. Making it configurable but leaving the default with the current behavior seems ok to me. Alternatively we could have a separate restriction lint that forces all if/else blocks to have the shorter one first.

[sanmai-NL]

A separate lint would be best then I think, since this proposal addresses a different use case/concern.

[nickdrozd]

The compiler is free to shuffle around blocks however it wants, so I don't see what considerations there are besides readability. That's the justification given for this lint in the first place:

Why is this bad
Negations reduce the readability of statements.

You might think that the most readable form of a conditional is the one with the fewest logical operators, and you're perfectly welcome to that choice. I prefer the smaller block to be on top. Again, it doesn't matter to the compiler either way, so this is just a matter of taste.

I have a bunch of blocks that look like the first one in my first post here. Clippy flags those and proposes a change that I think would make the code uglier and harder to read. So my current choices are to 1) disable that check, or 2) ignore the complaints and try to sift through the noise to find real violations. Neither of these is very appealing, so why not at least make it configurable?

[flip1995]

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.