SR-5739 Add refactoring action to collapse nested if

[willtellis]

This implements a refactoring action in the IDE to collapse nested if statements. It handles collapsing only a single outer if statement with single inner if statement. Each if statement may contain one or more conditionals.

For example

if a > 2 {
  if a < 10 {}
}

would be refactored to

if a > 2, a < 10 {}

To do:

• Filter out statements where the outer if statement contains anything besides the inner if
• Handle collapsing if let statements
• Add tests

Addresses SR-5739.

[willtellis]

@harlanhaskins Thanks for offering to help me with this.

[harlanhaskins]

Nitpick: LLVM style prefers early returns, rather than nesting the body of the function. This would be better expressed by:

if (S->getKind() != StmtKind::If) {
  return false;
}

[harlanhaskins]

Nitpick: LLVM style uses 2 spaces for indentation.

[CodaFi]

Could also dyn_cast first

auto *IFS = dyn_cast<IfStmt>(S);
if (!IFS) return false;
/**/ 

[harlanhaskins]

This condition is the same as the outer condition, you can remove this if check

[harlanhaskins]

Wrote a couple of nitpicks, but I think this is definitely in the right track! Thanks for tackling this and thanks for contributing! 🎉

[nkcsgexi]

@willtellis Awesome work so far! Thank you for contributing to Swift refactoring.

[harlanhaskins]

Nitpick: 80 characters per line, please

[nkcsgexi]

can we add a test case to ensure such refactoring kind doesn't show up in a multi-statement brace statement?
e.g

if a > 2 { // not here.
  if a < 5 {
  }
  if a < 10 {
  }
}

[nkcsgexi]

oh, I saw your listed to do above. Please ignore this.

[mgrebenets]

Does this mean that refactoring will support only 2 if statements initially?

[nkcsgexi]

I'm personally ok with collapsing only 2 if statements at one refactoring operation, since the users can always do more by invoking the tool multiple times.

[mgrebenets]

While thinking on how multiple if-statement conditions could be combined together, I have re-discovered once again that in C-based languages comma is an operator and it's computed left-to-right, which means for boolean conditions it's same as &&.

So if we have

if a > 1 {
  if a < 1 || a > 3 {
  }
}

instead of if (a > 1) && (a < 1 || a > 3) we can just do if a > 1, a < 1 || a > 3.
It may look a bit off, but when we have optional binding or pattern matching inside if statements, then , works for all cases:

if let a = a, a > 1 {
  if let b = b, b < 3 || b > 5 {
    if d == false {
    }
  }
}

with , combining conditions is very easy:

if let a = a, a > 1, let b = b, b < 3 || b > 5, d == false {
}

It will also work for pattern matches:

    if case let .one = enum1, a > 1 {
        if case let .two = enum2, b < 5 {
            print("hhh'")
        }
    }

will become:

    if case let .one = enum1, a > 1, case let .two = enum2, b < 5 {
        print("hhh'")
    }

[willtellis]

I was planning on implementing combining of conditionals for if let statements using this approach next. If we prefer combining conditionals using commas for all cases, it certainly simplifies the logic. I believe it also makes it so we can easily combine if statements with more than one conditional. Thoughts? Should I change direction and pursue this approach? I don't think it will be a big change from what I already have.

[willtellis]

Actually I just went ahead and did it. Let me know what you think!

[willtellis]

I'm still dealing with one small issue. How can I apply proper indentation to my collapsed if statement? Currently,

if a > 2 {
  if a < 10 {
  }
}

becomes

if a > 2, a < 10 {
  }

[harlanhaskins]

@nkcsgexi might have a better answer, but what if you used the column of the inner if?

This will break if, say, the code is

if x { if y { } }

But that might be a case we don’t care too much about...

[nkcsgexi]

@willtellis Source editor will take care of indentation after applying a refactoring, so here we need to only worry about generating the correct refactoring content.

[willtellis]

@nkcsgexi That's good to know, thanks! In that case, I think this PR is ready for approval. Do we need to run these smoke tests next?

[harlanhaskins]

@swift-ci please smoke test

[nkcsgexi]

@swift-ci Please test Linux platform

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 15a6c60

[nkcsgexi]

@willtellis Merged!

[harlanhaskins]

Thanks for the great contribution, @willtellis!

[willtellis]

Thanks for all the help!