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?

[flip1995]

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

I don't think that is a problem. If you don't see it you don't know that it is there.

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

Maybe we could still lint on multiline return expressions.

Generally I like your idea about skipping "simple" return expressions.

[xFrednet]

What do you think about using cognitive complexity to determine if a return statement is "simple"? We could simply reuse the implementation of the cognitive_complexity lint on the return statement. This would additionally allow us to add a limit configuration if there are still discussions about what is considered simple enough.

[flip1995]

Hmm, the cognitive complexity lint was abandoned for the reason that we are not able to actually decide how complex an expression is, because there are just to many factors.

I'd like to here @felix91gr opinion on reusing stuff of the cognitive complexity implementation to determine the complexity of a single expression vs. a complete function (/a bigger scope).

[felix91gr]

(Ohh. This is an interesting issue, @flip1995. Thanks for pinging me.)

Okay so the reason why I stopped pursuing the cognitive complexity lint is because the approach I was using was just too incomplete and naïve. To keep a long story short:

• The source I was using for the metric was not scientific (MY bad, and it took me ages to realize this). This makes my statements about trusting it as a source from the CS research completely unsustained.
• Measuring code complexity is way beyond the scope of what we can do with our available tools. It requires at least some approximate model of a human reader, and we're just not there yet in terms of static analysis technology. I can point to some resources where this is talked about in length if you'd like :3
• The implementation I made is, then, but the shadow of a ragdoll that resembles the idea of a good measurement of cognitive complexity. It is not representative of the actual task at hand at all.

So with the background out of the way, my thoughts are these: @xFrednet, I'd advise against it. I don't think the code I made is even remotely useful for this task. That said, open source is open source and I cannot tell you what you can do and cannot. But I think it can't be done with what I made, and you might encounter a lot of gratuitous hardship with no much reward if you try to use that implementation regardless 🙂

[xFrednet]

I wasn't aware of the discussions around the cognitive complexity lint that have already taken place. Thank you for clarifying this @flip1995 and @felix91gr. That was the main reason why I suggested it. So, back to the drawing board I guess 🙃

[cjdelisle]

Here's my example:

pub fn pc_degrade_announcement_target(ann_tar: u32, ann_age_blocks: u32) -> u32 {
    if ann_age_blocks < ANN_WAIT_PERIOD {
    } else if ann_age_blocks > 256 + ANN_WAIT_PERIOD {
    } else if ann_age_blocks == ANN_WAIT_PERIOD {
        return ann_tar;
    } else {
        let bn_ann_tar = bn_for_compact(ann_tar) << (ann_age_blocks - ANN_WAIT_PERIOD);
        if bn_ann_tar.bits() < 256 {
            let out = compact_for_bn(bn_ann_tar);
            if out <= 0x207fffff {
                return out;
            }
        }
    }
    0xffffffff
}

I use this pattern of empty blocks often because or-chains become long and unreasonable and I don't think it should be considered an error.

[camsteffen]

How about we allow

// either there is an `else` here
else if a {
    single_statement;
} else if b {
    same_single_statement;
} else // ...or there is an `else` here

[flip1995]

Yeah, I think we can allow one-line similarities. We could use our is_muliline_span (or something like that) to determine if the statement is multiple lines long. And if it is a single expr/stmt that is only one line long, just don't lint.

[jdahlstrom]

A case that I just ran into that I think should be accepted:

fn to_index(&self, x: usize, y: usize) -> Option<usize> {
    if x < 0 || x >= self.w { // ignore for now that Clippy also complains about these
        None
    } else if y < 0 || y >= self.h {         
        None
    } else {                                 
        Some(y * self.w + x)       
    }
}                                                

I think this is the clearest way to write the logic. The single-line heuristic would work great here.

[hrxi]

I also hit this. Since this is a deny-by-default lint, false positives are especially annoying.

[J-ZhengLi]

I suppose the conclusion to this discussion would be:

...only trigger this lint if the block contains actual statements. Single return statements like in the two issues would than be ignored.

...allow one-line similarities

Hopefully I didn't miss anything else, so... I'll adjusting the labels for this issue (ping me if wrong labels being added/removed)

@rustbot label -A-documentation -A-lint +I-false-positive +good-first-issue

[kevincox]

FWIW I don't think one line is a great heuristic for this lint. But it would avoid all of the false-positives that I have personally run into. I also notice that "no false positives" has been removed from the description of all do I don't have a strong argument against it. I also agree that it is an improvement over the current state. If this greatly reduces the false positive rate than a per-use opt-out becomes more palatable.

So if that is the official judgement call I'm ok with it.