Merge pull request #37174 from hamishknight/off-the-chain

[Refactoring] Unwrap optional chains in async transform

lib/IDE/Refactoring.cpp

@@ -4762,8 +4762,9 @@ class AsyncConverter : private SourceEntityWalker {
   SmallString<0> Buffer;
   llvm::raw_svector_ostream OS;
 
-  // Decls where any force-unwrap of that decl should be unwrapped, eg. for a
+  // Decls where any force unwrap or optional chain of that decl should be
-  // previously optional closure paramter has become a non-optional local
+  // elided, e.g for a previously optional closure parameter that 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
@@ -4874,11 +4875,19 @@ private:
                                OS << PLACEHOLDER_END;
                            });
       }
-    } else if (auto *FTE = dyn_cast<ForceValueExpr>(E)) {
+    } else if (isa<ForceValueExpr>(E) || isa<BindOptionalExpr>(E)) {
-      if (auto *D = FTE->getReferencedDecl().getDecl()) {
+      // Remove a force unwrap or optional chain of a returned success value,
+      // as it will no longer be optional. For force unwraps, this is always a
+      // valid transform. For optional chains, it is a locally valid transform
+      // within the optional chain e.g foo?.x -> foo.x, but may change the type
+      // of the overall chain, which could cause errors elsewhere in the code.
+      // However this is generally more useful to the user than just leaving
+      // 'foo' as a placeholder. Note this is only the case when no other
+      // optionals are involved in the chain, e.g foo?.x?.y -> foo.x?.y is
+      // completely valid.
+      if (auto *D = E->getReferencedDecl().getDecl()) {
         if (Unwraps.count(D))
-          return addCustom(FTE->getStartLoc(),
+          return addCustom(E->getStartLoc(), E->getEndLoc().getAdvancedLoc(1),
-                           FTE->getEndLoc().getAdvancedLoc(1),
                            [&]() { OS << newNameFor(D, true); });
       }
     } else if (NestedExprCount == 0) {

test/refactoring/ConvertAsync/convert_params_single.swift

@@ -406,3 +406,35 @@ withError { res, err in
   print("got result \(res!)")
   print("after")
 }
+
+// RUN: %refactor -convert-call-to-async-alternative -dump-text -source-filename %s -pos=%(line+1):3 | %FileCheck -check-prefix=UNWRAPPING %s
+withError { str, err in
+  print("before")
+  guard err == nil else { return }
+  _ = str!.count
+  _ = str?.count
+  _ = str!.count.bitWidth
+  _ = str?.count.bitWidth
+  _ = (str?.count.bitWidth)!
+  _ = str!.first?.isWhitespace
+  _ = str?.first?.isWhitespace
+  _ = (str?.first?.isWhitespace)!
+  print("after")
+}
+// UNWRAPPING:      let str = try await withError()
+// UNWRAPPING-NEXT: print("before")
+// UNWRAPPING-NEXT: _ = str.count
+// UNWRAPPING-NEXT: _ = str.count
+// UNWRAPPING-NEXT: _ = str.count.bitWidth
+// UNWRAPPING-NEXT: _ = str.count.bitWidth
+
+// Note this transform results in invalid code as str.count.bitWidth is no
+// longer optional, but arguably it's more useful than leaving str as a
+// placeholder. In general, the tranform we perform here is locally valid
+// within the optional chain, but may change the type of the overall chain.
+// UNWRAPPING-NEXT: _ = (str.count.bitWidth)!
+
+// UNWRAPPING-NEXT: _ = str.first?.isWhitespace
+// UNWRAPPING-NEXT: _ = str.first?.isWhitespace
+// UNWRAPPING-NEXT: _ = (str.first?.isWhitespace)!
+// UNWRAPPING-NEXT: print("after")