Skip to content

Commit 025f4dd

Browse files
rintarorintaro
authored
Merge pull request #16789 from rintaro/refactoring-nested-if
[Refactoring] Re-implement "collapse nested if" action
2 parents 2403fb8 + 639fb85 commit 025f4dd

File tree

4 files changed

+95
-112
lines changed

4 files changed

+95
-112
lines changed

‎include/swift/IDE/RefactoringKinds.def

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ CURSOR_REFACTORING(LocalizeString, "Localize String", localize.string)
4444

4545
CURSOR_REFACTORING(SimplifyNumberLiteral, "Simplify Long Number Literal", simplify.long.number.literal)
4646

47-
CURSOR_REFACTORING(CollapseNestedIfExpr, "Collapse Nested If Expression", collapse.nested.if.expr)
47+
CURSOR_REFACTORING(CollapseNestedIfStmt, "Collapse Nested If Statements", collapse.nested.ifstmt)
4848

4949
CURSOR_REFACTORING(ConvertToDoCatch, "Convert To Do/Catch", convert.do.catch)
5050

‎lib/IDE/Refactoring.cpp

Lines changed: 53 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1668,109 +1668,70 @@ bool RefactoringActionReplaceBodiesWithFatalError::performChange() {
16681668
return false;
16691669
}
16701670

1671-
struct CollapsibleNestedIfInfo {
1672-
IfStmt *OuterIf;
1673-
IfStmt *InnerIf;
1674-
bool FinishedOuterIf;
1675-
bool FoundNonCollapsibleItem;
1676-
CollapsibleNestedIfInfo():
1677-
OuterIf(nullptr), InnerIf(nullptr),
1678-
FinishedOuterIf(false), FoundNonCollapsibleItem(false) {}
1679-
bool isValid() {
1680-
return OuterIf && InnerIf && FinishedOuterIf && !FoundNonCollapsibleItem;
1681-
}
1682-
};
1683-
1684-
static CollapsibleNestedIfInfo findCollapseNestedIfTarget(ResolvedCursorInfo CursorInfo) {
1671+
static std::pair<IfStmt *, IfStmt *>
1672+
findCollapseNestedIfTarget(ResolvedCursorInfo CursorInfo) {
16851673
if (CursorInfo.Kind != CursorInfoKind::StmtStart)
1686-
return CollapsibleNestedIfInfo();
1687-
struct IfStmtFinder: public SourceEntityWalker {
1688-
SourceLoc StartLoc;
1689-
CollapsibleNestedIfInfo IfInfo;
1690-
IfStmtFinder(SourceLoc StartLoc): StartLoc(StartLoc), IfInfo() {}
1691-
bool finishedInnerIfButNotFinishedOuterIf() {
1692-
return IfInfo.InnerIf && !IfInfo.FinishedOuterIf;
1693-
}
1694-
bool walkToStmtPre(Stmt *S) {
1695-
if (finishedInnerIfButNotFinishedOuterIf()) {
1696-
IfInfo.FoundNonCollapsibleItem = true;
1697-
return false;
1698-
}
1674+
return {};
16991675

1700-
bool StmtIsOuterIfBrace =
1701-
IfInfo.OuterIf && !IfInfo.InnerIf && S->getKind() == StmtKind::Brace;
1702-
if (StmtIsOuterIfBrace) {
1703-
return true;
1704-
}
1676+
// Ensure the statement is 'if' statement. It must not have 'else' clause.
1677+
IfStmt *OuterIf = dyn_cast<IfStmt>(CursorInfo.TrailingStmt);
1678+
if (!OuterIf)
1679+
return {};
1680+
if (OuterIf->getElseStmt())
1681+
return {};
17051682

1706-
auto *IFS = dyn_cast<IfStmt>(S);
1707-
if (!IFS) {
1708-
return false;
1709-
}
1710-
if (!IfInfo.OuterIf) {
1711-
IfInfo.OuterIf = IFS;
1712-
return true;
1713-
} else {
1714-
IfInfo.InnerIf = IFS;
1715-
return false;
1716-
}
1717-
}
1718-
bool walkToStmtPost(Stmt *S) {
1719-
assert(S != IfInfo.InnerIf && "Should not traverse inner if statement");
1720-
if (S == IfInfo.OuterIf) {
1721-
IfInfo.FinishedOuterIf = true;
1722-
}
1723-
return true;
1724-
}
1725-
bool walkToDeclPre(Decl *D, CharSourceRange Range) {
1726-
if (finishedInnerIfButNotFinishedOuterIf()) {
1727-
IfInfo.FoundNonCollapsibleItem = true;
1728-
return false;
1729-
}
1730-
return true;
1731-
}
1732-
bool walkToExprPre(Expr *E) {
1733-
if (finishedInnerIfButNotFinishedOuterIf()) {
1734-
IfInfo.FoundNonCollapsibleItem = true;
1735-
return false;
1736-
}
1737-
return true;
1738-
}
1683+
// The body must contain a sole inner 'if' statement.
1684+
auto Body = dyn_cast_or_null<BraceStmt>(OuterIf->getThenStmt());
1685+
if (!Body || Body->getNumElements() != 1)
1686+
return {};
1687+
1688+
IfStmt *InnerIf =
1689+
dyn_cast_or_null<IfStmt>(Body->getElement(0).dyn_cast<Stmt *>());
1690+
if (!InnerIf)
1691+
return {};
17391692

1740-
} Walker(CursorInfo.TrailingStmt->getStartLoc());
1741-
Walker.walk(CursorInfo.TrailingStmt);
1742-
return Walker.IfInfo;
1693+
// Inner 'if' statement also cannot have 'else' clause.
1694+
if (InnerIf->getElseStmt())
1695+
return {};
1696+
1697+
return {OuterIf, InnerIf};
17431698
}
17441699

1745-
bool RefactoringActionCollapseNestedIfExpr::
1746-
isApplicable(ResolvedCursorInfo Tok, DiagnosticEngine &Diag) {
1747-
return findCollapseNestedIfTarget(Tok).isValid();
1700+
bool RefactoringActionCollapseNestedIfStmt::
1701+
isApplicable(ResolvedCursorInfo CursorInfo, DiagnosticEngine &Diag) {
1702+
return findCollapseNestedIfTarget(CursorInfo).first;
17481703
}
17491704

1750-
bool RefactoringActionCollapseNestedIfExpr::performChange() {
1705+
bool RefactoringActionCollapseNestedIfStmt::performChange() {
17511706
auto Target = findCollapseNestedIfTarget(CursorInfo);
1752-
if (!Target.isValid())
1707+
if (!Target.first)
17531708
return true;
1754-
auto OuterIfConds = Target.OuterIf->getCond().vec();
1755-
auto InnerIfConds = Target.InnerIf->getCond().vec();
1709+
auto OuterIf = Target.first;
1710+
auto InnerIf = Target.second;
17561711

1757-
EditorConsumerInsertStream OS(EditConsumer, SM,
1758-
Lexer::getCharSourceRangeFromSourceRange(
1759-
SM, Target.OuterIf->getSourceRange()));
1760-
1761-
OS << tok::kw_if << " ";
1762-
for (auto CI = OuterIfConds.begin(); CI != OuterIfConds.end(); ++CI) {
1763-
OS << (CI != OuterIfConds.begin() ? ", " : "");
1764-
OS << Lexer::getCharSourceRangeFromSourceRange(
1765-
SM, CI->getSourceRange()).str();
1766-
}
1767-
for (auto CI = InnerIfConds.begin(); CI != InnerIfConds.end(); ++CI) {
1768-
OS << ", " << Lexer::getCharSourceRangeFromSourceRange(
1769-
SM, CI->getSourceRange()).str();
1770-
}
1771-
auto ThenStatementText = Lexer::getCharSourceRangeFromSourceRange(
1772-
SM, Target.InnerIf->getThenStmt()->getSourceRange()).str();
1773-
OS << " " << ThenStatementText;
1712+
EditorConsumerInsertStream OS(
1713+
EditConsumer, SM,
1714+
Lexer::getCharSourceRangeFromSourceRange(SM, OuterIf->getSourceRange()));
1715+
1716+
OS << tok::kw_if << " ";
1717+
1718+
// Emit conditions.
1719+
bool first = true;
1720+
for (auto &C : llvm::concat<StmtConditionElement>(OuterIf->getCond(),
1721+
InnerIf->getCond())) {
1722+
if (first)
1723+
first = false;
1724+
else
1725+
OS << ", ";
1726+
OS << Lexer::getCharSourceRangeFromSourceRange(SM, C.getSourceRange())
1727+
.str();
1728+
}
1729+
1730+
// Emit body.
1731+
OS << " ";
1732+
OS << Lexer::getCharSourceRangeFromSourceRange(
1733+
SM, InnerIf->getThenStmt()->getSourceRange())
1734+
.str();
17741735
return false;
17751736
}
17761737

‎test/refactoring/RefactoringKind/basic.swift

Lines changed: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -174,33 +174,53 @@ func testStringLiteral() -> String {
174174
return "abc"
175175
}
176176

177-
func testCollapseNestedIf() {
177+
func testCollapseNestedIf1() {
178178
let a = 3
179179
if a > 2 {
180180
if a < 10 {}
181181
}
182182
}
183183

184-
func testMultiConditionalNestedIf() {
184+
func testCollapseNestedIf2() {
185185
let a = 3
186186
if a > 2, a != 4 {
187187
if a < 10 {}
188188
}
189189
}
190190

191-
func testExtraDeclNestedIf() {
191+
func testCollapseNestedIf3() {
192192
let a = 3
193193
if a > 2 {
194194
if a < 10 {}
195195
let b = 0
196196
}
197197
}
198198

199-
func testExtraIfNestedIf() {
199+
func testCollapseNestedIf4() {
200200
let a = 3
201201
if a > 2 {
202-
if a < 10 {}
203202
let b = 0
203+
if a < 10 {}
204+
}
205+
}
206+
207+
func testCollapseNestedIf5() {
208+
let a = 3
209+
if a > 2 {
210+
if a < 10 {}
211+
} else {
212+
print("else")
213+
}
214+
}
215+
216+
func testCollapseNestedIf6() {
217+
let a = 3
218+
if a > 2 {
219+
if a < 10 {
220+
print("if")
221+
} else if a < 5 {
222+
print("else")
223+
}
204224
}
205225
}
206226

@@ -313,25 +333,27 @@ func testConvertToTernaryExpr() {
313333

314334
// RUN: %refactor -source-filename %s -pos=173:3 -end-pos=173:27| %FileCheck %s -check-prefix=CHECK-EXTRCT-METHOD
315335

316-
// RUN: %refactor -source-filename %s -pos=179:3 | %FileCheck %s -check-prefix=CHECK-COLLAPSE-NESTED-IF-EXPRESSION
317-
// RUN: %refactor -source-filename %s -pos=186:3 | %FileCheck %s -check-prefix=CHECK-COLLAPSE-NESTED-IF-EXPRESSION
336+
// RUN: %refactor -source-filename %s -pos=179:3 | %FileCheck %s -check-prefix=CHECK-COLLAPSE-NESTED-IF
337+
// RUN: %refactor -source-filename %s -pos=186:3 | %FileCheck %s -check-prefix=CHECK-COLLAPSE-NESTED-IF
318338
// RUN: %refactor -source-filename %s -pos=193:3 | %FileCheck %s -check-prefix=CHECK-NONE
319339
// RUN: %refactor -source-filename %s -pos=201:3 | %FileCheck %s -check-prefix=CHECK-NONE
340+
// RUN: %refactor -source-filename %s -pos=209:3 | %FileCheck %s -check-prefix=CHECK-NONE
341+
// RUN: %refactor -source-filename %s -pos=218:3 | %FileCheck %s -check-prefix=CHECK-NONE
320342

321-
// RUN: %refactor -source-filename %s -pos=210:11 -end-pos=210:24 | %FileCheck %s -check-prefix=CHECK-STRINGS-INTERPOLATION
322-
// RUN: %refactor -source-filename %s -pos=211:11 -end-pos=211:26 | %FileCheck %s -check-prefix=CHECK-STRINGS-INTERPOLATION
323-
// RUN: %refactor -source-filename %s -pos=212:11 -end-pos=212:21 | %FileCheck %s -check-prefix=CHECK-STRINGS-INTERPOLATION
343+
// RUN: %refactor -source-filename %s -pos=230:11 -end-pos=230:24 | %FileCheck %s -check-prefix=CHECK-STRINGS-INTERPOLATION
344+
// RUN: %refactor -source-filename %s -pos=231:11 -end-pos=231:26 | %FileCheck %s -check-prefix=CHECK-STRINGS-INTERPOLATION
345+
// RUN: %refactor -source-filename %s -pos=232:11 -end-pos=232:21 | %FileCheck %s -check-prefix=CHECK-STRINGS-INTERPOLATION
324346

325-
// RUN: %refactor -source-filename %s -pos=217:11 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
326-
// RUN: %refactor -source-filename %s -pos=217:12 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
327-
// RUN: %refactor -source-filename %s -pos=217:13 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
328-
// RUN: %refactor -source-filename %s -pos=217:14 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
347+
// RUN: %refactor -source-filename %s -pos=237:11 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
348+
// RUN: %refactor -source-filename %s -pos=237:12 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
349+
// RUN: %refactor -source-filename %s -pos=237:13 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
350+
// RUN: %refactor -source-filename %s -pos=237:14 | %FileCheck %s -check-prefix=CHECK-TRY-CATCH
329351

330-
// RUN: %refactor -source-filename %s -pos=225:3 | %FileCheck %s -check-prefix=CHECK-EXPAND-SWITCH
352+
// RUN: %refactor -source-filename %s -pos=245:3 | %FileCheck %s -check-prefix=CHECK-EXPAND-SWITCH
331353

332-
// RUN: %refactor -source-filename %s -pos=231:3 -end-pos=231:24 | %FileCheck %s -check-prefix=CHECK-EXPAND-TERNARY-EXPRESSEXPRESSION
354+
// RUN: %refactor -source-filename %s -pos=251:3 -end-pos=251:24 | %FileCheck %s -check-prefix=CHECK-EXPAND-TERNARY-EXPRESSEXPRESSION
333355

334-
// RUN: %refactor -source-filename %s -pos=237:3 -end-pos=242:4 | %FileCheck %s -check-prefix=CHECK-CONVERT-TO-TERNARY-EXPRESSEXPRESSION
356+
// RUN: %refactor -source-filename %s -pos=257:3 -end-pos=262:4 | %FileCheck %s -check-prefix=CHECK-CONVERT-TO-TERNARY-EXPRESSEXPRESSION
335357

336358
// CHECK1: Action begins
337359
// CHECK1-NEXT: Extract Method
@@ -368,7 +390,7 @@ func testConvertToTernaryExpr() {
368390

369391
// CHECK-LOCALIZE-STRING: Localize String
370392

371-
// CHECK-COLLAPSE-NESTED-IF-EXPRESSION: Collapse Nested If Expression
393+
// CHECK-COLLAPSE-NESTED-IF: Collapse Nested If Statements
372394

373395
// CHECK-STRINGS-INTERPOLATION: Convert to String Interpolation
374396

‎tools/swift-refactor/swift-refactor.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ Action(llvm::cl::desc("kind:"), llvm::cl::init(RefactoringKind::None),
4141
"expand-switch-cases", "Perform switch cases expand refactoring"),
4242
clEnumValN(RefactoringKind::LocalizeString,
4343
"localize-string", "Perform string localization refactoring"),
44-
clEnumValN(RefactoringKind::CollapseNestedIfExpr,
44+
clEnumValN(RefactoringKind::CollapseNestedIfStmt,
4545
"collapse-nested-if", "Perform collapse nested if statements"),
4646
clEnumValN(RefactoringKind::ConvertToDoCatch,
4747
"convert-to-do-catch", "Perform force try to do try catch refactoring"),

0 commit comments

Comments
 (0)