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.

---

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

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[flip1995]

Previous discussion: #3363

[nickdrozd]

The outstanding discussion issues are:

• Should the top-heaviness of if/else blocks be configurable?
• If so, what should the default be?

Note that this change does not add a new lint. Instead, it tempers an existing one. Some users might prefer the strict approach of the lint as it currently exists, but I think that kind of aggressive behavior should be opt-in, not opt-out. Readability is what matters here, not consistency.

That said, I don't care all that much about configuration options, just so long as one way or another I can get reasonable behavior out of this lint.

[flip1995]

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

cc #3363 (comment)

I think this is reasonable. Can you add a configuration for this? Since there wasn't more discussion on the reopened PR, I think we can merge a configurable version.

[sanmai-NL]

@nickdrozd, @flip1995: I don't understand why this is a new PR now. The previous discussion is relevant and is sidestepped by this new PR.

I don't recognize the issue of 'top heaviness' (some branch having a higher number of lines). It is not an issue for readability at all for me. Whether the heaviness is at the top of the bottom or in the middle, it's there anyway. If you have too much source code in your branches, refactor it to redistribute it (e.g., across helper functions).

Moreover, if it is an issue for @nickdrozd, then you should know and acknowledge that IDEs can (un)fold blocks and even (un)fold them by default or in bulk, based on your selection.

The tampering with an existing lint is exactly what makes the proposal less desirable over adding a new one. I do not like the arbitrariness introduced by making top-heaviness a factor at all. I wonder how this interacts with old macros.

@nickdrozd: is there previous work in linters for other languages that favors your change?

[flip1995]

Github seems to prevent the author of a closed PR to reopen it, when it was closed by a maintainer.

I don't think adding a configuration option is a big deal. Making a new lint for this does not seem worth to me.

Maybe we can make the configuration default so that the top-heaviness check is disabled. So that the config is opt-in.

[sanmai-NL]

@flip1995: on reopening the PR. Will you then change your standard message upon closing the PR, to only offer the opener to ask a maintainer to reopen the PR? Your current resolution broke the discussion thread.

[flip1995]

@sanmai-NL Yes, I didn't know, that the author can't reopen closed PRs. I will word it differently in the future.

[sanmai-NL]

@flip1995: I believe you only accept contributions that add value (cost vs. benefit). I'm sorry for @nickdrozd but the context hasn't changed since he stopped working on the previous PR. So far, there have been critical comments, and no counterarguments from the PR opener, community support or examples from previous work.

[flip1995]

I don't think a configuration option for this lint is too costly. It's 3 LoC testing the top-heaviness and 2 LoC for the config option. This won't have any measurable influence on performance. I don't think the cost will be too high for the benefit, that there is a configuration option.

User-facing cost would just be to have a few more lines in the documentation to read, when looking up this lint. Benefit is that everyone can decide for themselfs, if they want the top-heaviness feature enabled.

So from a cost-benefit perspective, I don't see why we shouldn't allow this contribution.

[sanmai-NL]

I generally agree on the cost, although I wouldn't add source code so easily if I had to maintain this. For me the important cost is in complexity in understanding other people's code bases with this knob enabled, and understanding Clippy itself, as you noted. The benefit is zero for me. To me this is addressing a non-issue.

[nickdrozd]

The tampering with an existing lint is exactly what makes the proposal less desirable over adding a new one.

There seems to be some confusion here. It makes no sense to suggest adding a new lint, because no new suggestions are being made. It is exactly the same suggestion as ever, only applied less aggressively. In particular, the current suggestion will not be made if the resulting code would be top-heavy and therefore ugly.

No new warnings would be emitted. It would be exactly the same warning as currently, but applied in fewer cases.

Let's be clear. There is no issue of correctness here. All the code involved is equivalent. The compiler does not care. The only consideration, as the documentation says, is readability. There is no reason to believe that readability is always, no matter what, every single time, going to be improved by switching blocks so that there is no negation in the conditional. Getting rid of those negations is a good idea generally, but obviously there are exceptions. A foolish consistency is the hobgoblin of little minds.

One problem with linters is that most users hate them. People don't like being told to rewrite their code. It is in the interest of the linter not to emit false positives, because false positives cause users to become skeptical of the linter's value. This is particularly true when it comes to functionally irrelevant opinions about style. Currently Clippy suggests replacing certain perfectly good conditional blocks with equivalent but uglier conditional blocks for no reason at all. This is a false positive, and it makes the tool less pleasant to use.

is there previous work in linters for other languages that favors your change?

I don't know about linters, but an aversion to top-heavy conditional blocks is built into Emacs Lisp at the sytactical level. An if expression consists of a test expression, followed by a single "then" expression, followed by any number of "else" expressions. The clear intent of this design is to keep the blocks from being top-heavy. This is even built into the standard indentation:

(if (not failure-condition)
    (proceed-as-normal)
  (handle-error)
  (clean-up)
  (notify)
  (etc))

If this lint were run against that block, the output would look like this:

(if (not failure-condition)
    (progn
      (handle-error)
      (clean-up)
      (notify)
      (etc))
  (proceed-as-normal))

That second block is obviously much uglier than the first one. Maybe some people like that kind of thing, but it is strange and nonstandard. I don't think Clippy should be so opinionated as to suggest changing code to look like this.

[flip1995]

Wait, I just saw, that if_not_else is a pedantic lint. I thought it would be a complexity or style lint.

The pedantic lint group is designed to include more opinionated lints, that can be enabled for specific code bases, if desired. It is not recommended to use the complete pedantic group, but selected lints from this group.

Clippy has lint groups which are more or less opinionated.

• perf and correctness should only contain lints, that aren't opinionated
• complexity lints make the code easier to read and shouldn't be that opinionated
• style lints can be opinionated and it is recommended to just allow them if a user doesn't like them. But we try to only include style lints that are generally agreed upon by the community
• pedantic as stated above: these lints can be really opinionated. So if you like one of those lints, enable it specifically. If not, just don't enable them
• restriction lints restrict the language and only make sense in a few, specific code bases.

One problem with linters is that most users hate them. People don't like being told to rewrite their code.

But that is exactly the point of a linter: The machine tells you to rewrite your code, to adhere to idiomatic/company/code base standards, and that no human has to do this in a code review. If a user hates a linter, than the linter is either bad or misused. I personally don't think that Clippy is a bad linter (obviously 😄)

---

Since this is a pedantic lint, I agree with @sanmai-NL that a configuration option has no real benefit here. If you want this lint (prefer easy to read conditions over order of blocks), enable it. If not, don't.

This lint is not enabled by default, and nowhere in the documentation is it recommended to enable it.

---

I will leave this PR open a bit longer, to allow more discussion about this.

[flip1995]

Because we already talked about cost-benefit:

The cost for this configuration option would be way higher, since this:

if !a {
    println!("xyz");
} else {
    let _ = if {
        // ...100 lines
    };
}

would still lint with this implementation. So we would need to implement a line counter. Or something that calculates complexity. But detecting complexity is a different story. Long story short: we can't do this (yet?, probably?, ...? #3793 (comment))

So what @sanmai-NL said, the cost for a working implementation for this would be too high for the benefit that it would give for a pedantic lint IMO.

[ebroto]

So, I've been following the discussion and would like to share my thoughts.

@flip1995 I think @nickdrozd stated that they dislike the lint when it results in a top-heavy if-else, not that they dislike it in all cases, so I interpret this PR as a possible enhancement to the existing lint.

I think the default behavior should not change though, and "top-heaviness" evaluation should be opt-in, for deference to the current users that like the lint the way it is, like @sanmai-NL.

About the cost, couldn't we use the span of the if/else blocks to determine the number of lines? If it's not that simple, then it's probably not worth it as already stated.

[flip1995]

About the cost, couldn't we use the span of the if/else blocks to determine the number of lines?

Yeah, but should comments count? What if the if branch is shorter, but way harder to read (more indentations, ...)? Which is the happy path and should it be first? ...?

Number of lines is IMHO not a good heuristic for ordering blocks. Neither is the number of statements (especially how statements are defined in the rust compiler).

Here comes the maintenance overhead in play, @sanmai-NL correctly mentioned. If we start now with the simple statement comparison, the next contribution to this could be to change this to number of lines, but then someone else thinks comments shouldn't count and changes it again. And on it goes.

I don't think, that we should add an opinionated config option to an opinionated lint (which is marked as opinionated as it is in the pedantic group).

when it results in a top-heavy if-else, not that they dislike it in all cases

Then this lint should just be #[allow]ed in those cases. It is, after all, a pedantic lint.

[ebroto]

Yeah, but should comments count? What if the if branch is shorter, but way harder to read (more indentations, ...)? Which is the happy path and should it be first? ...?
...the next contribution to this could be to change this to number of lines, but then someone else thinks comments shouldn't count and changes it again. And on it goes.

Hmm, I think you're right, this has many more dimensions of opinionated that what has been discussed and could be a never-ending battle.

Then this lint should just be #[allow]ed in those cases. It is, after all, a pedantic lint.

Fair point. I thought clippy could accomodate more opinions, but failed to see the maintenance cost.

[nickdrozd]

I deal with enough of this kind of bullshit at work, so I'm closing this.

For future reference, here is a summary of the discussion thread:

0. The existing lint behavior is overly aggressive, and it sometimes makes suggestions that obviously make the code uglier.

1. I propose a minor modification to the behavior of the check so that it won't suggest changing the code in certain cases.

2. Some loudmouth announces that he sees no problem with the ugly code, and demands that the lint's dogmatic existing behavior be unchanged.

3. Despite evincing a clear lack of taste when it comes to code formatting, the loudmouth's extreme opinion succeeds in shifting the Overton window, and it is proposed that the dogmatic existing behavior remain the default and that the moderate new behavior be relegated to a config option.

4. The loudmouth then turns around and argues that the maintenance costs incurred by adding a new config option outweigh the benefit of making the moderate linting behavior available at all, and that therefore the whole thing should be scrapped.

[flip1995]

Please stay civil and lower the aggressive tone!

@nickdrozd I think every point you made was addressed:

0. It is a pedantic and therefore opinionated lint, and aggressive by design (README.md: "lints which are rather strict, off by default"). I was only in favor of this config option, because I thought it was a warn-by-default lint.

1. To get this to work, like you desire would be more than a minor modification and is potentially again really opinionated

2. He listed multiple arguments, why "the ugly code" is no concern for him

3. "lack of taste when it comes to code formatting" is an entirely subjective opinion

4. You're concern was about complexity of the code. Solely the line number doesn't has to influence how difficult it is to read the code. I listed other factors, that should be taken into account above. If we want to take this into account it will definitely lead to bikeshedding which always leads to maintenance cost.

That being said, I don't like this lint myself for the same reasons as you (and some other, e.g. happy path). The solution for me is to just not enable this lint. There's a reason, why pedantic lints are allow-by-default.