[refactoring] Implement "Convert to Trailing Closure" refactoring action #12268

rintaro · 2017-10-04T16:04:19Z

As a groundwork, I modified CursorInfoResolver and ResolvedCursorInfo so that it preserves surrounding ASTNode (direct child of BraceStmt).
Also, introduced CursorInfoKind::Middle (naming TBD) which means the cursor is at middle of someting, like:

do {
  foo(x: bar, y: baz)
       ↑ here
}

As a ground work, added SourceFile to ResolvedCursorInfo struct. Also, let CursorInfoResolver always set Loc to ResolvedCursorInfo so that we can manually resolve the surrounding context later.

This refactoring action searches innermost CallExpr from the cursor position. If found, it's applicable if that CallExpr has ClosureExpr as the last argument.

rintaro · 2017-10-04T16:04:33Z

@swift-ci Please smoke test

rintaro · 2017-10-04T16:08:50Z

@nkcsgexi Could you review this?
I'm not 100% sure that this strategy is good.

rintaro · 2017-10-04T16:51:20Z

@swift-ci Please smoke test

nkcsgexi · 2017-10-05T18:33:33Z

lib/IDE/Refactoring.cpp

+  default:
+    Loc = CursorInfo.Loc;
+    break;
+  }


Can we just replace this with an early return in the second case? and use SourceLoc Loc = CursorInfo.Loc; in general. CursorInfo.TrailingExpr->getStartLoc() and CursorInfo.Loc should be equivalent.

nkcsgexi · 2017-10-05T18:44:31Z

lib/IDE/SwiftSourceDocInfo.cpp

@@ -152,6 +156,14 @@ bool CursorInfoResolver::walkToStmtPre(Stmt *S) {
  // non-implicit Stmts (fix Stmts created for lazy vars).
  if (!S->isImplicit() && !rangeContainsLoc(S->getSourceRange()))
    return false;
+
+  if (isa<BraceStmt>(S)) {
+    ContextStack.push_back(ContextNode);


We only capture explicit brace statement as the context node. I think the main benefit is we can start walk from the brace statement instead of the source file. However, walking AST is fairly efficient operation. I think we can afford to start walking from SourceFIle.

OK, I will try walking from SourceFIle.
Can I add SourceFile property to ResolvedCursorInfo then?
Should I pass SF from collectAvailableRefactorings() to isApplicable()?

That's a good idea! can we add a SoureFile field to ResolvedCursorInfo instead of passing it? I bet other refactorings may need this information too.

Actually mine needs it, too :) I've changed the isApplicable to expect ApplicabilityContext, which is a struct that contains DiagnosticEngine reference and SourceFile pointer. You could check it here: #12281. To be honest either way works, so I'm happy to change it.
One thought, though: If we're passing SourceFile to the ResolvedCursorInfo— shouldn't we pass DiagnosticEngine the same way for consistency?

If we're passing SourceFile to the ResolvedCursorInfo — shouldn't we pass DiagnosticEngine the same way for consistency?

I don't think DiagnosticEngine should be a property of ResolvedCursorInfo, whereas SourceFile is directly related to the cursor position.

Yeah, you're right 👍

nkcsgexi · 2017-10-05T18:45:45Z

lib/IDE/Refactoring.cpp

+    }
+
+  public:
+    CallExprFinder(SourceManager &SM, SourceLoc Loc) : SM(SM), Loc(Loc) {}


Once we walk the entire source file to find the call expression. We can use an existing utility in this file ContextFinder. right?

nkcsgexi · 2017-10-05T18:46:43Z

@rintaro Looks good in general. Some comments inline 😄

rintaro · 2017-10-10T20:41:49Z

@swift-ci Please smoke test

rintaro · 2017-10-10T21:20:54Z

@swift-ci Please smoke test Linux platform

rintaro · 2017-10-10T23:18:21Z

@swift-ci Please smoke test Linux platform

rintaro · 2017-10-11T02:31:10Z

@nkcsgexi
Could you re-review this?

nkcsgexi

Looks great, @rintaro some minor questions/comments are inline.

nkcsgexi · 2017-10-13T22:07:56Z

lib/IDE/Refactoring.cpp

@@ -2066,6 +2073,107 @@ bool RefactoringActionSimplifyNumberLiteral::performChange() {
  return true;
 }

+static CallExpr *findTrailingClosureTarget(SourceManager &SM,
+                                           ResolvedCursorInfo CursorInfo) {
+  if (CursorInfo.Kind == CursorInfoKind::StmtStart)


Why are we handling StmtStart specifically here?

Because StmtStart cursor position can't be a part of CallExpr.
We can bail-out early.

nkcsgexi · 2017-10-13T22:18:54Z

lib/IDE/Refactoring.cpp

+    LastArg = TE->getElements().back();
+  }
+
+  while (auto *ICE = dyn_cast<ImplicitConversionExpr>(LastArg))


Can we check isImplicit() instead, which seems to be more generic.

I think your original code makes sense, but can we add it as a member function to ImplicitConversionExpr?

isImplicit() doesn't necessary mean it's ImplicitConversionExpr.
Probably, we should add a member function to Expr, say getSingleSyntacticExpr() which dig into isImplicit() expressions using SourceEntitiyWalker(or ASTWalker). (Single because implicit TupleExpr may contain multiple syntactic expressions. Returns nullptr in such cases).

I think we can add getSyntacticSubExpr() to ImplicitConversionExpr. Adding such method to Expr seems to be an overkill because in a long term we will have libSyntax to get rid of all implicit nodes. Also, as you said, we have to return nullptr for TupleExpr which is a little ambiguous for we're not sure whether it means no explicit nodes or multiple of them. What do you think?

I agree. Thanks!

nkcsgexi · 2017-10-13T22:19:18Z

lib/IDE/Refactoring.cpp

+  if (Finder.getContexts().empty()
+      || !Finder.getContexts().back().is<Expr*>())
+    return nullptr;
+  CallExpr *CE = dyn_cast<CallExpr>(Finder.getContexts().back().get<Expr*>());


can we use cast here? because we're sure it'll succeed.

You are right! 👍

nkcsgexi · 2017-10-13T22:23:21Z

lib/IDE/Refactoring.cpp

+    if (NumArgs > 1)
+      PrevArg = TE->getElement(NumArgs - 2);
+  }
+  while (auto *ICE = dyn_cast<ImplicitConversionExpr>(ClosureArg))


Same here; isImplicit() seems to be more appropriate.

nkcsgexi · 2017-10-13T22:24:27Z

lib/IDE/Refactoring.cpp

+  while (auto *ICE = dyn_cast<ImplicitConversionExpr>(ClosureArg))
+    ClosureArg = ICE->getSubExpr();
+
+  if (!isa<ClosureExpr>(ClosureArg) || LPLoc.isInvalid() || RPLoc.isInvalid())


Why do we check !isa<ClosureExpr>(ClosureArg)? it seems to me we are sure the closure is present.

nkcsgexi · 2017-10-13T22:27:49Z

lib/IDE/Refactoring.cpp

+      SM,
+      Lexer::getLocForEndOfToken(SM, ClosureArg->getEndLoc()),
+      Lexer::getLocForEndOfToken(SM, RPLoc));
+  EditConsumer.accept(SM, PostRange, "");


could we add a remove function to SourceEditConsumer and use it here?

rintaro · 2017-10-16T07:42:41Z

@swift-ci Please smoke test

nkcsgexi · 2017-10-16T20:07:03Z

Looks great! Merging.

nkcsgexi · 2017-10-16T20:08:54Z

@rintaro LGTM, feel free to merge!

rintaro · 2017-10-16T20:57:04Z

Thank you @nkcsgexi !

rintaro force-pushed the refactoring-trailingclosure branch from a2308f5 to 8d05ecb Compare October 4, 2017 16:50

nkcsgexi self-requested a review October 5, 2017 00:20

nkcsgexi reviewed Oct 5, 2017

View reviewed changes

Kacper20 mentioned this pull request Oct 10, 2017

[Refactoring] SR-6051 Expansion of switch statement missing cases #12281

Merged

rintaro force-pushed the refactoring-trailingclosure branch 3 times, most recently from 784277c to 32c01dd Compare October 10, 2017 20:40

nkcsgexi reviewed Oct 13, 2017

View reviewed changes

[IDE] Add SourceFile property to ResolvedCursorInfo

53e5145

rintaro force-pushed the refactoring-trailingclosure branch from 32c01dd to fe21f47 Compare October 16, 2017 07:25

[refactoring] Implement "Convert to Trailing Closure" refactoring action

a57199c

rintaro force-pushed the refactoring-trailingclosure branch from fe21f47 to a57199c Compare October 16, 2017 07:42

nkcsgexi approved these changes Oct 16, 2017

View reviewed changes

rintaro merged commit 0b34078 into apple:master Oct 16, 2017

rintaro deleted the refactoring-trailingclosure branch October 16, 2017 20:57

rintaro mannequin mentioned this pull request Apr 3, 2018

[SR-5738] Refactoring action to convert to trailing closure #48308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactoring] Implement "Convert to Trailing Closure" refactoring action #12268

[refactoring] Implement "Convert to Trailing Closure" refactoring action #12268

rintaro commented Oct 4, 2017 •

edited

rintaro commented Oct 4, 2017

rintaro commented Oct 4, 2017

rintaro commented Oct 4, 2017

nkcsgexi Oct 5, 2017

nkcsgexi Oct 5, 2017

rintaro Oct 6, 2017

nkcsgexi Oct 6, 2017 •

edited

Kacper20 Oct 7, 2017

rintaro Oct 9, 2017

Kacper20 Oct 9, 2017

nkcsgexi Oct 5, 2017

nkcsgexi commented Oct 5, 2017

rintaro commented Oct 10, 2017

rintaro commented Oct 10, 2017

rintaro commented Oct 10, 2017

rintaro commented Oct 11, 2017

nkcsgexi left a comment

nkcsgexi Oct 13, 2017

rintaro Oct 14, 2017

nkcsgexi Oct 13, 2017

nkcsgexi Oct 13, 2017

rintaro Oct 14, 2017

nkcsgexi Oct 14, 2017

rintaro Oct 16, 2017

nkcsgexi Oct 13, 2017

rintaro Oct 14, 2017

nkcsgexi Oct 13, 2017

nkcsgexi Oct 13, 2017

nkcsgexi Oct 13, 2017

rintaro commented Oct 16, 2017

nkcsgexi commented Oct 16, 2017

nkcsgexi commented Oct 16, 2017

rintaro commented Oct 16, 2017

[refactoring] Implement "Convert to Trailing Closure" refactoring action #12268

[refactoring] Implement "Convert to Trailing Closure" refactoring action #12268

Conversation

rintaro commented Oct 4, 2017 • edited

rintaro commented Oct 4, 2017

rintaro commented Oct 4, 2017

rintaro commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcsgexi Oct 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcsgexi commented Oct 5, 2017

rintaro commented Oct 10, 2017

rintaro commented Oct 10, 2017

rintaro commented Oct 10, 2017

rintaro commented Oct 11, 2017

nkcsgexi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro commented Oct 16, 2017

nkcsgexi commented Oct 16, 2017

nkcsgexi commented Oct 16, 2017

rintaro commented Oct 16, 2017

rintaro commented Oct 4, 2017 •

edited

nkcsgexi Oct 6, 2017 •

edited