[SR-5744] Refactoring action to convert if-let to guard-let and vice versa #24566

Regno · 2019-05-07T09:00:31Z

Implement action to convert from:

func foo(idxOpt: Int?) {
  if let idx = idxOpt {
    print(idx)
  }
}

to

func foo(idxOpt: Int?) {
  guard let idx = idxOpt else {
    return
  }
  print(idx)
}

and vice-versa

Resolves new feature request SR-5744.
Refactoring action to convert if-let to guard-let and vice versa

Feedback is very much appreciated!

Regno · 2019-05-07T09:01:59Z

@swift-ci test

theblixguy · 2019-05-07T10:43:40Z

You can't invoke CI yourself, you need commit access for it.

theblixguy · 2019-05-07T10:43:45Z

cc @akyrtzi

nkcsgexi

The implementation looks great overall👍! I have only several nitpicks inline.

nkcsgexi · 2019-07-08T18:02:28Z

lib/IDE/Refactoring.cpp

+
+  if (Info.ContainedNodes.size() == 1) {
+    if (auto S = Info.ContainedNodes[0].dyn_cast<Stmt*>()) {
+        If = dyn_cast<IfStmt>(S);


We should always use 2-spaces indentation. Could you fix other indentation issues as well in this change?

nkcsgexi · 2019-07-08T18:03:44Z

lib/IDE/Refactoring.cpp

+
+  if (CondList.size() != 1)
+    return true; // abort
+


We can assume performChange() is called only when isApplicable returns true. So it seems the above logics are unnecessary.

nkcsgexi · 2019-07-08T18:05:04Z

lib/IDE/Refactoring.cpp

+    auto Body = dyn_cast_or_null<BraceStmt>(If->getThenStmt());
+
+    if (!Body)
+        return true; // abort


Could you move the ThenStmt check to the isApplicable function too? so we don't need to check it here and surprising the users by aborting.

Moved to the isApplicable.

nkcsgexi · 2019-07-08T18:06:34Z

lib/IDE/Refactoring.cpp

+
+    // Get if-let else body.
+    auto ElseBody = dyn_cast_or_null<BraceStmt>(If->getElseStmt());
+    if (ElseBody) {


These two statements can be merged as if (auto *ElseBody = dyn_cast_or_null<BraceStmt>(If->getElseStmt())) {}

nkcsgexi · 2019-07-08T18:09:00Z

lib/IDE/Refactoring.cpp

+    auto E = CondList[0];
+    auto P = E.getPatternOrNull();
+    if (!P)
+        return true; // abort


Similarly, can we move these condition checkings into isApplicable function and only check it there?

nkcsgexi · 2019-07-08T18:13:49Z

lib/IDE/Refactoring.cpp

+    auto ReplaceRange = RangeInfo.ContentRange;
+    EditConsumer.accept(SM, ReplaceRange, DeclBuffer.str());
+
+	return false;


Indentation issue here and all other places.

nkcsgexi · 2019-07-10T22:14:35Z

@swift-ci please smoke test

nkcsgexi · 2019-07-10T23:09:18Z

@swift-ci please stress test

Regno · 2019-07-11T10:59:05Z

Also I fixed similar issues at this PR: Refactoring action to convert to computed property.

nkcsgexi · 2019-07-11T18:31:50Z

All tests passed. Congratulations for your first Swift contribution!🚢

akyrtzi requested a review from nkcsgexi May 14, 2019 00:37

nkcsgexi reviewed Jul 8, 2019

View reviewed changes

Власов Антон Вячеславович added 2 commits July 9, 2019 16:21

SR-5744 Refactoring action to convert if-let to guard-let and vice versa

8acd794

SR-5744 Fixs from comments to pull request

f119f21

nkcsgexi merged commit d0f0069 into swiftlang:master Jul 11, 2019

swift-ci mentioned this pull request May 7, 2019

[SR-5744] Refactoring action to convert if-let to guard-let and vice versa #48314

Closed

[SR-5744] Refactoring action to convert if-let to guard-let and vice versa #24566

[SR-5744] Refactoring action to convert if-let to guard-let and vice versa #24566

Uh oh!

Conversation

Regno commented May 7, 2019

Uh oh!

Regno commented May 7, 2019

Uh oh!

theblixguy commented May 7, 2019

Uh oh!

theblixguy commented May 7, 2019

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkcsgexi commented Jul 10, 2019

Uh oh!

nkcsgexi commented Jul 10, 2019

Uh oh!

Regno commented Jul 11, 2019

Uh oh!

nkcsgexi commented Jul 11, 2019

Uh oh!

Uh oh!