[Refactoring] Only convert calls with handlers that have completion-like names

Still convert the call if it was requested directly - only check the name
when converting a whole function. Once we have an attribute, we should
use that instead.

Author: bnbarham

include/swift/ClangImporter/ClangImporter.h

@@ -492,6 +492,9 @@ ImportDecl *createImportDecl(ASTContext &Ctx, DeclContext *DC, ClangNode ClangN,
 std::string
 getModuleCachePathFromClang(const clang::CompilerInstance &Instance);
 
+/// Whether the given parameter name identifies a completion handler.
+bool isCompletionHandlerParamName(StringRef paramName);
+
 } // end namespace swift
 
 #endif

lib/ClangImporter/ImportName.cpp

@@ -1189,8 +1189,7 @@ Optional<ForeignErrorConvention::Info> NameImporter::considerErrorImport(
   return None;
 }
 
-/// Whether the given parameter name identifies a completion handler.
-static bool isCompletionHandlerParamName(StringRef paramName) {
+bool swift::isCompletionHandlerParamName(StringRef paramName) {
   return paramName == "completionHandler" ||
       paramName == "withCompletionHandler" ||
       paramName == "completion" || paramName == "withCompletion" ||

lib/IDE/Refactoring.cpp

@@ -28,6 +28,7 @@
 #include "swift/Frontend/Frontend.h"
 #include "swift/IDE/IDERequests.h"
 #include "swift/Index/Index.h"
+#include "swift/ClangImporter/ClangImporter.h"
 #include "swift/Parse/Lexer.h"
 #include "swift/Sema/IDETypeChecking.h"
 #include "swift/Subsystems.h"
@@ -3894,12 +3895,6 @@ namespace asyncrefactorings {
 
 // TODO: Should probably split the refactorings into separate files
 
-/// Whether the given parameter name identifies a completion callback
-bool isCompletionHandlerName(StringRef Name) {
-  return Name.startswith("completion") || Name.contains("Completion") ||
-         Name.contains("Complete");
-}
-
 /// Whether the given type is the stdlib Result type
 bool isResultType(Type Ty) {
   if (!Ty)
@@ -4083,7 +4078,7 @@ struct AsyncHandlerDesc {
   HandlerType Type = HandlerType::INVALID;
   bool HasError = false;
 
-  static AsyncHandlerDesc find(const FuncDecl *FD, bool ignoreName = false) {
+  static AsyncHandlerDesc find(const FuncDecl *FD, bool ignoreName) {
     if (!FD || FD->hasAsync() || FD->hasThrows())
       return AsyncHandlerDesc();
 
@@ -4104,7 +4099,7 @@ struct AsyncHandlerDesc {
 
     // Callback must have a completion-like name (if we're not ignoring it)
     if (!ignoreName &&
-        !isCompletionHandlerName(HandlerDesc.Handler->getNameStr()))
+        !isCompletionHandlerParamName(HandlerDesc.Handler->getNameStr()))
       return AsyncHandlerDesc();
 
     // Callback must be a function type and return void. Doesn't need to have
@@ -4670,12 +4665,12 @@ struct CallbackClassifier {
   }
 };
 
-/// Builds up async-converted code for any added AST nodes.
+/// Builds up async-converted code for an AST node.
 ///
-/// Function declarations will have `async` added. If a completion handler is
-/// present, it will be removed and the return type of the function will
-/// reflect the parameters of the handler, including an added `throws` if
-/// necessary.
+/// If it is a function, its declaration will have `async` added. If a
+/// completion handler is present, it will be removed and the return type of
+/// the function will reflect the parameters of the handler, including an
+/// added `throws` if necessary.
 ///
 /// Calls to the completion handler are replaced with either a `return` or
 /// `throws` depending on the arguments.
@@ -4699,28 +4694,58 @@ struct CallbackClassifier {
 /// The fallback is generally avoided, however, since it's quite unlikely to be
 /// the code the user intended. In most cases the refactoring will continue,
 /// with any unhandled decls wrapped in placeholders instead.
-class AsyncConversionStringBuilder : private SourceEntityWalker {
+class AsyncConverter : private SourceEntityWalker {
   SourceManager &SM;
   DiagnosticEngine &DiagEngine;
+
+  // Node to convert
+  ASTNode StartNode;
+
+  // Completion handler of `StartNode` (if it's a function with an async
+  // alternative)
   const AsyncHandlerDesc &TopHandler;
   SmallString<0> Buffer;
   llvm::raw_svector_ostream OS;
 
+  // Decls where any force-unwrap of that decl should be unwrapped, eg. for a
+  // previously optional closure paramter has become a non-optional local
   llvm::DenseSet<const Decl *> Unwraps;
+  // Decls whose references should be replaced with, either because they no
+  // longer exist or are a different type. Any replaced code should ideally be
+  // handled by the refactoring properly, but that's not possible in all cases
   llvm::DenseSet<const Decl *> Placeholders;
+  // Mapping from decl -> name, used as both the name of possibly new local
+  // declarations of old parameters, as well as the replacement for any
+  // references to it
   llvm::DenseMap<const Decl *, std::string> Names;
 
+  // These are per-node (ie. are saved and restored on each convertNode call)
   SourceLoc LastAddedLoc;
   int NestedExprCount = 0;
 
 public:
-  AsyncConversionStringBuilder(SourceManager &SM, DiagnosticEngine &DiagEngine,
-                               const AsyncHandlerDesc &TopHandler)
-      : SM(SM), DiagEngine(DiagEngine), TopHandler(TopHandler), Buffer(),
-        OS(Buffer) {
+  AsyncConverter(SourceManager &SM, DiagnosticEngine &DiagEngine,
+                 ASTNode StartNode, const AsyncHandlerDesc &TopHandler)
+      : SM(SM), DiagEngine(DiagEngine), StartNode(StartNode),
+        TopHandler(TopHandler), Buffer(), OS(Buffer) {
     Placeholders.insert(TopHandler.Handler);
   }
 
+  bool convert() {
+    if (!Buffer.empty())
+      return !DiagEngine.hadAnyError();
+
+    if (auto *FD = dyn_cast_or_null<FuncDecl>(StartNode.dyn_cast<Decl *>())) {
+      addFuncDecl(FD);
+      if (FD->getBody()) {
+        convertNode(FD->getBody());
+      }
+    } else {
+      convertNode(StartNode);
+    }
+    return !DiagEngine.hadAnyError();
+  }
+
   void replace(ASTNode Node, SourceEditConsumer &EditConsumer,
                SourceLoc StartOverride = SourceLoc()) {
     SourceRange Range = Node.getSourceRange();
@@ -4739,13 +4764,7 @@ class AsyncConversionStringBuilder : private SourceEntityWalker {
     Buffer.clear();
   }
 
-  void convertFunction(const FuncDecl *FD) {
-    addFuncDecl(FD);
-    if (FD->getBody()) {
-      convertNode(FD->getBody());
-    }
-  }
-
+private:
   void convertNodes(ArrayRef<ASTNode> Nodes) {
     for (auto Node : Nodes) {
       OS << "\n";
@@ -4765,9 +4784,17 @@ class AsyncConversionStringBuilder : private SourceEntityWalker {
     addRange(LastAddedLoc, Node.getEndLoc(), /*ToEndOfToken=*/true);
   }
 
-private:
   bool walkToDeclPre(Decl *D, CharSourceRange Range) override {
-    return isa<PatternBindingDecl>(D);
+    if (isa<PatternBindingDecl>(D)) {
+      NestedExprCount++;
+      return true;
+    }
+    return false;
+  }
+
+  bool walkToDeclPost(Decl *D) override {
+    NestedExprCount--;
+    return true;
   }
 
 #define PLACEHOLDER_START "<#"
@@ -4805,9 +4832,8 @@ class AsyncConversionStringBuilder : private SourceEntityWalker {
                          [&]() { addHandlerCall(CE); });
 
       if (auto *CE = dyn_cast<CallExpr>(E)) {
-        auto HandlerDesc =
-            AsyncHandlerDesc::find(getUnderlyingFunc(CE->getFn()),
-                                   /*ignoreName=*/true);
+        auto HandlerDesc = AsyncHandlerDesc::find(
+            getUnderlyingFunc(CE->getFn()), StartNode.dyn_cast<Expr *>() == CE);
         if (HandlerDesc.isValid())
           return addCustom(CE->getStartLoc(), CE->getEndLoc().getAdvancedLoc(1),
                            [&]() { addAsyncAlternativeCall(CE, HandlerDesc); });
@@ -5259,13 +5285,11 @@ bool RefactoringActionConvertCallToAsyncAlternative::performChange() {
          "Should not run performChange when refactoring is not applicable");
 
   AsyncHandlerDesc TempDesc;
-  AsyncConversionStringBuilder Builder(SM, DiagEngine, TempDesc);
-  Builder.convertNode(CE);
-
-  if (DiagEngine.hadAnyError())
+  AsyncConverter Converter(SM, DiagEngine, CE, TempDesc);
+  if (!Converter.convert())
     return true;
 
-  Builder.replace(CE, EditConsumer);
+  Converter.replace(CE, EditConsumer);
   return false;
 }
 
@@ -5289,13 +5313,11 @@ bool RefactoringActionConvertToAsync::performChange() {
          "Should not run performChange when refactoring is not applicable");
 
   AsyncHandlerDesc TempDesc;
-  AsyncConversionStringBuilder Builder(SM, DiagEngine, TempDesc);
-  Builder.convertFunction(FD);
-
-  if (DiagEngine.hadAnyError())
+  AsyncConverter Converter(SM, DiagEngine, FD, TempDesc);
+  if (!Converter.convert())
     return true;
 
-  Builder.replace(FD, EditConsumer, FD->getSourceRangeIncludingAttrs().Start);
+  Converter.replace(FD, EditConsumer, FD->getSourceRangeIncludingAttrs().Start);
   return false;
 }
 
@@ -5328,16 +5350,14 @@ bool RefactoringActionAddAsyncAlternative::performChange() {
   assert(HandlerDesc.isValid() &&
          "Should not run performChange when refactoring is not applicable");
 
-  AsyncConversionStringBuilder Builder(SM, DiagEngine, HandlerDesc);
-  Builder.convertFunction(FD);
-
-  if (DiagEngine.hadAnyError())
+  AsyncConverter Converter(SM, DiagEngine, FD, HandlerDesc);
+  if (!Converter.convert())
     return true;
 
   EditConsumer.accept(SM, FD->getAttributeInsertionLoc(false),
                       "@available(*, deprecated, message: \"Prefer async "
                       "alternative instead\")\n");
-  Builder.insertAfter(FD, EditConsumer);
+  Converter.insertAfter(FD, EditConsumer);
 
   return false;
 }

test/refactoring/ConvertAsync/basic.swift

@@ -108,6 +108,10 @@ func genericResult<T>(completion: (T?, Error?) -> Void) where T: Numeric { }
 func genericError<E>(completion: (String?, E?) -> Void) where E: Error { }
 // GENERIC-ERROR: func genericError<E>() async throws -> String where E: Error { }
 
+// RUN: %refactor -add-async-alternative -dump-text -source-filename %s -pos=%(line+1):1 | %FileCheck -check-prefix=OTHER-NAME %s
+func otherName(execute: (String) -> Void) { }
+// OTHER-NAME: func otherName() async -> String { }
+
 struct MyStruct {
   var someVar: (Int) -> Void {
     get {
@@ -266,9 +270,13 @@ func testCalls() {
   let _: Void = simple { str in
     print("assigned")
   }
+  // CONVERT-FUNC: let _: Void = simple { str in{{$}}
+  // CONVERT-FUNC-NEXT: print("assigned"){{$}}
+  // CONVERT-FUNC-NEXT: }{{$}}
 
   // RUN: not %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3
   noParamAutoclosure(completion: print("autoclosure"))
+  // CONVERT-FUNC: noParamAutoclosure(completion: print("autoclosure")){{$}}
 
   // RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=EMPTY-CAPTURE %s
   simple { [] str in
@@ -284,5 +292,15 @@ func testCalls() {
   }
   // CAPTURE: let str = await simple(){{$}}
   // CAPTURE-NEXT: {{^}}print("closure with capture list \(anything)")
+
+  // RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=OTHER-DIRECT %s
+  otherName(execute: { str in
+    print("otherName")
+  })
+  // OTHER-DIRECT: let str = await otherName(){{$}}
+  // OTHER-DIRECT-NEXT: {{^}}print("otherName")
+  // CONVERT-FUNC: otherName(execute: { str in{{$}}
+  // CONVERT-FUNC-NEXT: print("otherName"){{$}}
+  // CONVERT-FUNC-NEXT: }){{$}}
 }
 // CONVERT-FUNC: {{^}}}