Rust-clippy: if_same_then_else Should not warn for `else if` blocks.

[kevincox]

if condition1() {
  1
} else if condition2() {
  1
} else if condition3() {
  2
} else {
  3
}

There are many cases where code like this shouldn't be warned on. While you could do condition1() || condition2() often times it is more clear of the intent if they are separate. Especially if the conditions are non-trivial compared to the value being returned.

I think the right time for this warning to warn is if the last if has the same condition as the else. For example the following two cases would warn.

if condition1() {
  1
} else {
  1
}

if condition1() {
  1
} else if condition2() {
  2
} else {
  2
}

[flip1995]

IMHO this lint is good as it is. If you have a situation where separating the if conditions makes the logic more readable you should allow this lint.

From my experience this lint has in fact two purposes:

1. The one that you described, which is: "Hey, you can merge this if conditions"
2. These two if bodies are the same, but maybe that's unintended

Example for 2.:

if c1 {
    // some code
} else if c2 {
    // some other code
} else if c3 {
    // same code as 1, because copy pasted, but forget to adapt that one line
} else {
    // ...
}

If we'd change the lint in a way that only consecutive branches would get linted, the second case would never be caught. And the second case can even lead to bugs, while the first one is just code style.

---

TL;DR: I wouldn't change the lint, but add the second point to the "Why is this bad?" section of the documentation.

[kevincox]

In the example you gave I also think that it would have too many false positives. There are lots of cases where the actual value is trivial compared to the complexity of the condition so you wouldn't want to merge it. Additionally the more conditions that are between the two identical branches the more complicated the condition would have to become. The "correct" fix in the example you provided is:

If c1 || (c3 && !c2) {
  // some code
} else if c2 {
 // some other code
} else {
  // ...
}

Unless the code in the statement body is quite complicated this is much harder to understand. And if the conditions have side effects it isn't even correct.

Also I'm not proposing warning on any consecutive branches. I would only warn when the last if and the else branches are identical because in this case you could just omit the condition which will (almost) always result in simpler code.

I guess it comes down to if you want to avoid false positives or false negatives. Maybe I can propose that this is split into two lints. One will warn when two consecutive branches are identical but not be in all. Then the one that only warns for the last two branches only can be included in all. I think this is reasonable because all claims "everything that has no false positives".

[flip1995]

The "correct" fix in the example you provided is

I think you misunderstood me there. The correct fix for my example would not be to merge the branches, but to change the code in the branch of condition 3, since it is copy pasted and should do something different (this happened to me in my last bigger project and saved me a lot of time).

The problem with the suggestion is though, that the lint can't know if merging the branches is what the programmer wants or changing the code in one branch is the right thing to do. (*)

Technically this isn't a false positive, since the if bodies are actually the same, so the lint does what it's supposed to do.

---

For the merge of the two conditions in the example I provided, following should be enough:

if c1 || c3 {
    // some code
} else if c2 {
    // some other code
} else {
    // ...
}

If c2 and c3 can both be true at the same time, but when c2 is true, the code in c3 (and c1) shouldn't be executed, then the logic is probably complex enough (special case) that adding an allow is reasonable.

---

(*) Maybe also add a note to the lint for this case, additionally to the suggestion.

[kevincox]

Well I guess we can agree to disagree.

If you don't want to change it could we add another lint for the case I suggested?

To be perfectly frank the lint has far too many false positives for me to use the way it is now but if it was broken in two then at least I could catch the cases I am interested in.

[kevincox]

For the merge of the two conditions in the example I provided, following should be enough of c2 and c3 can both be true at the same time,

I agree that this works in the simple case. However if I have to sprinkle lint ignores all over my code it becomes too distracting. I see that we also have a different definition of false positive. In my mind a false positive is "The linter bothered me, but the code could not be improved". This is a very different definition then the linter not having a bug.

[flip1995]

Well I guess we can agree to disagree.

I've labeled this as C-needs-discussion, to (hopefully) get more opinions on this.

add another lint for the case I suggested?

That would be possible

However if I have to sprinkle lint ignores all over my code it becomes too distracting

If you (completely) disagree with a lint, you can write #![allow(clippy::lint)] in your crate root, to completely disable it in your crate. Clippy lints are sometimes opinionated.

[kentfredric]

I'd suggest that a user seeing this warning when doing error handling, eg:

if stituation {
   Err(...)
} else if second_situation {
   Err(...)
} else {
    Ok()
}

That this may be a good place to think about creating a new class of errors. Even though there's no bug "as-is" in the logic, adding some fidelity to the return value improves the product.

[kentfredric]

It may also be helpful to offer computed unions/intersections in the error report where possible, because mentally performing boolean operations between lots of different cases is sometimes tricky ( see the aforementioned c1 || (c3 && !c2) expression ).

You could always sit down and write a big ol' Karnaugh Map for this, but it seems viable a linter could just have the required logic in it to simplify the conditions automatically.

[wwylele]

Hi, I just want to provide a real example I encountered on this topic. I wrote a function that handles user input birthday:

// User has already selected a month and also selected a day just now
// This functions adjust the month selection in case the combination is invalid
// There is also a special valid combination where month=0 and day=0, 
//  which means "Not provided"
fn adjust_birthmonth(day: u16, month: &mut u16) {
    if day == 0 {
        // month and day must be both zero for "not provided" combination
        *month = 0;
    } else if *month == 0 {
        // if themonth is not provided but the day is, pick a valid month instead
        *month = 1; 
    } else if day == 30 && *month == 2 {
        // There is no Feb 30. Change to Jan instead
        *month = 1;
        // ^ clippy lint this on if_same_then_else. 
        // Technically these two blocks are indeed mergable, 
        // but their meanings are pretty different. 
        // So I really want to keep them separate to make the logic clear. 
    } else if day == 31 {
        if let 2|4|6|9|12 = *month { 
            // There is no 31 for these months. Change to a valid month instead
            *month -= 1;
        }
    }
}

(I admit the overall UX design on this is not ideal, but that is a different topic)
This is my first encounter on a "deny" level lint that I don't agree with. I agree on the general usefulness of this lint, but it can conflict with some cases like this.

[flip1995]

I agree that it makes sense to keep the arms separated here. But I think that in this case you should just allow this lint.

I guess adding another lint or making this lint configurable is the way to go for this issue.

[nagisa]

In answer to @montrivo here, in particular this following excerpt of their comment:

This is not a false positive. The lint is designed so.

The lint description says this:

Why is this bad
This is probably a copy & paste error.

I would argue that if the branches have wildly different bodies (when you take the comments into consideration), then it cannot possibly be a copy&paste error. And so, the lint’s design or implementation does not appear to be serving its initial purpose. Or that its purpose is misrepresented.

Sure, you can allow the lint, but remember that this lint is also deny-by-default. I wouldn’t mind this issue as much if it was just a warn-by-default.

[kevincox]

I think you misunderstood me there. The correct fix for my example would not be to merge the branches, but to change the code in the branch of condition 3

Ha, I missed this remark. If I am reading this correctly you are asking me to change my correct code to incorrect code because the linter doesn't like it? My point is that this lint is flagging perfectly correct and readable code and incorrectly suggesting that it can be improved. This is what I call a false-positive and seems to be the thing that isn't supposed to exist in all according to the docs.

Another possible fix is to update the docs of all to better describe the types of lints that it contains.

[paridhi7]

Hi @kevincox I'm new to open source and would love to get started. Can I work on this issue?

[kevincox]

It's not my project. You would need to contact the maintainers. Hopefully they will see your interest and drop some starters here.

Of course it seems that maintainers think this is working as intended ao I'm not sure they are interested in a patch at this point.

[ebroto]

@paridhi7 this is indeed not a good first issue, the label was added when the fix was thought to be just a documentation change. I think you could start with #5849, I will leave some mentoring instructions there in case you wish to tackle it. Don't hesitate to ask for support there or in Discord (#clippy channel or private message) if you get stuck.

About this issue, just to give another opinion, I believe we should change the lint to be warn-by-default, as it has been shown that folks consider this useful sometimes, so it does not fit the correctness category. Mistakes would still be caught by default. Besides that, this pattern is not so frequent so I think allowing the lint when it pops up makes sense.

It would be fun to try to implement @nagisa's suggestion :), but I would consider that heuristic experimental and not a candidate for a warn-by-default lint.

[flip1995]

Oh I thought I replied to this: I definitely see now that having this as a correctness lint might be the wrong categorization. What we might want to do is to just allow this lint, if there are comments in the arm bodies. But a good first step would be to downgrade this lint to style or complexity. I would vote for style since merging two arms is not always less complex.

[xFrednet]

This lint is in my opinion there to avoid code duplication. Both this and issue #6285 trigger the lint as the statements and return of the blocks are the same. However, I agree that both are not direct cases of code duplication because they don't contain any logic and just a simple single expression.

I would therefore suggest to only trigger this lint if the block contains actual statements. Single return statements like in the two issues would than be ignored. The lint help message could than suggest to either combine the conditions or to move the duplicate code into a local function.

Some examples that are collapsed to keep it readable

1. Single return statements would not be linted:

   if x == 9 {
       true
   } else x == 8 {
       true
   } else {
       false
   }
   

2. Statements with logic would be linted

   if x == 9 {
       let y = x * 2 - 9;
       println!("logic");
       y != 17
   } else if x == 8 {
       let y = x * 2 - 9;
       println!("logic");
       y != 17
   } else {
       false
   }
   

   The help would either suggest to combine the conditions (i.) or to move the logic into a local (ii.):

   1. Combine the conditions like this:

      if x == 9 || x == 8 {
          let y = x * 2 - 9;
          println!("logic");
          y != 17
      } else {
          false
      }
      

   2. Move the logic into a local function

      // Note you can define functions inside functions. I just learned that 1 week ago ^^
      fn do_logic(x: i32) -> bool {
          let y = x * 2 - 9;
          println!("logic");
          y != 17
      }
      
      if x == 9 {
          do_logic(x)
      } else if x == 8 {
          do_logic(x)
      } else {
          false
      }
      

      This would not be linted as all blocks only contain a return value and no additional logic.

---

There are two drawbacks that I see with this implementation:

1. This could confuse users of clippy why some blocks get linted and some don't

2. Users could have a long duplicate return statement that wouldn't be linted.
   

   
   

   Example of a case that wouldn't be linted

   if x == 9 {
       (x * 19 + 17) << 4 + (y & 0x00ff) ^ 13
   } else {
       (x * 19 + 17) << 4 + (y & 0x00ff) ^ 13
   }
   

---

What are your thoughts about this option?