[Async Refactoring] Placeholder dropped params in fallback case

Make sure to convert any references to parameters
that get dropped in the fallback case to
placeholders.

Author: hamishknight

lib/IDE/Refactoring.cpp

@@ -7184,10 +7184,9 @@ class AsyncConverter : private SourceEntityWalker {
       AllBindings.append(SuccessBindings.begin(), SuccessBindings.end());
       AllBindings.push_back(ErrParam);
 
-      // Don't do any unwrapping or placeholder replacement since all params
-      // are still valid in the fallback case
       prepareNames(ClassifiedBlock(), AllBindings, InlinePatterns);
-
+      preparePlaceholdersAndUnwraps(HandlerDesc, CallbackParams,
+                                    PlaceholderMode::FALLBACK);
       addFallbackVars(AllBindings, Blocks);
       addDo();
       addAwaitCall(CE, Blocks.SuccessBlock, SuccessBindings, InlinePatterns,
@@ -7243,7 +7242,7 @@ class AsyncConverter : private SourceEntityWalker {
 
     prepareNames(Blocks.SuccessBlock, SuccessBindings, InlinePatterns);
     preparePlaceholdersAndUnwraps(HandlerDesc, CallbackParams,
-                                  /*Success=*/true);
+                                  PlaceholderMode::SUCCESS_BLOCK);
 
     addAwaitCall(CE, Blocks.SuccessBlock, SuccessBindings, InlinePatterns,
                  HandlerDesc, /*AddDeclarations=*/true);
@@ -7260,7 +7259,7 @@ class AsyncConverter : private SourceEntityWalker {
                    ErrInlinePatterns,
                    /*AddIfMissing=*/HandlerDesc.Type != HandlerType::RESULT);
       preparePlaceholdersAndUnwraps(HandlerDesc, CallbackParams,
-                                    /*Success=*/false);
+                                    PlaceholderMode::ERROR_BLOCK);
 
       addCatch(ErrOrResultParam);
       convertNodes(Blocks.ErrorBlock.nodesToPrint());
@@ -7547,14 +7546,31 @@ class AsyncConverter : private SourceEntityWalker {
     OS << tok::l_brace;
   }
 
+  enum class PlaceholderMode {
+    SUCCESS_BLOCK, ERROR_BLOCK, FALLBACK
+  };
+
   void preparePlaceholdersAndUnwraps(AsyncHandlerDesc HandlerDesc,
                                      const ClosureCallbackParams &Params,
-                                     bool Success) {
+                                     PlaceholderMode Mode) {
+    // Params that have been dropped always need placeholdering.
+    for (auto *Param : Params.getAllParams()) {
+      if (!Params.hasBinding(Param))
+        Placeholders.insert(Param);
+    }
+    // For the fallback case, no other params need placeholdering, as they are
+    // all freely accessible in the fallback case.
+    if (Mode == PlaceholderMode::FALLBACK)
+      return;
+
     switch (HandlerDesc.Type) {
     case HandlerType::PARAMS: {
       auto *ErrParam = Params.getErrParam();
       auto SuccessParams = Params.getSuccessParams();
-      if (!Success) {
+      switch (Mode) {
+      case PlaceholderMode::FALLBACK:
+        llvm_unreachable("Already handled");
+      case PlaceholderMode::ERROR_BLOCK:
         if (ErrParam) {
           if (HandlerDesc.shouldUnwrap(ErrParam->getType())) {
             Placeholders.insert(ErrParam);
@@ -7563,7 +7579,8 @@ class AsyncConverter : private SourceEntityWalker {
           // Can't use success params in the error body
           Placeholders.insert(SuccessParams.begin(), SuccessParams.end());
         }
-      } else {
+        break;
+      case PlaceholderMode::SUCCESS_BLOCK:
         for (auto *SuccessParam : SuccessParams) {
           auto Ty = SuccessParam->getType();
           if (HandlerDesc.shouldUnwrap(Ty)) {
@@ -7573,10 +7590,6 @@ class AsyncConverter : private SourceEntityWalker {
             Placeholders.insert(SuccessParam);
           }
 
-          // If the parameter doesn't have a binding, it needs placeholdering.
-          if (!Params.hasBinding(SuccessParam))
-            Placeholders.insert(SuccessParam);
-
           // Void parameters get omitted where possible, so turn any reference
           // into a placeholder, as its usage is unlikely what the user wants.
           if (HandlerDesc.getSuccessParamAsyncReturnType(Ty)->isVoid())
@@ -7585,6 +7598,7 @@ class AsyncConverter : private SourceEntityWalker {
         // Can't use the error param in the success body
         if (ErrParam)
           Placeholders.insert(ErrParam);
+        break;
       }
       break;
     }

test/refactoring/ConvertAsync/convert_bool.swift

@@ -459,4 +459,21 @@ func testConvertBool() async throws {
   // OBJC-BOOL-WITH-ERR2-NEXT:   print("neat")
   // OBJC-BOOL-WITH-ERR2-NEXT:   print("neato")
   // OBJC-BOOL-WITH-ERR2-NEXT: }
+
+  // RUN: %refactor-check-compiles -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 -I %S/Inputs -I %t -target %target-triple %clang-importer-sdk-nosource | %FileCheck -check-prefix=OBJC-BOOL-WITH-ERR-FALLBACK %s
+  ClassWithHandlerMethods.firstBoolFlagSuccess("") { str, success, unrelated, err in
+    guard success && success == .random() else { fatalError() }
+    print("much success", unrelated, str)
+  }
+  // OBJC-BOOL-WITH-ERR-FALLBACK:      var str: String? = nil
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: var unrelated: Bool? = nil
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: var err: Error? = nil
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: do {
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT:   (str, unrelated) = try await ClassWithHandlerMethods.firstBoolFlagSuccess("")
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: } catch {
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT:   err = error
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: }
+  // OBJC-BOOL-WITH-ERR-FALLBACK-EMPTY:
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: guard <#success#> && <#success#> == .random() else { fatalError() }
+  // OBJC-BOOL-WITH-ERR-FALLBACK-NEXT: print("much success", unrelated, str)
 }