Merge pull request #15383 from xedin/rdar-38648760

[Sema] Improve diagnostics for non-escaping function types

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -2805,6 +2805,9 @@ ERROR(assigning_noescape_to_escaping,none,
 ERROR(general_noescape_to_escaping,none,
       "using non-escaping parameter %0 in a context expecting an @escaping closure",
       (Identifier))
+ERROR(converting_noescape_to_type,none,
+      "converting non-escaping value to %0 may allow it to escape",
+      (Type))
 
 ERROR(capture_across_type_decl,none,
       "%0 declaration cannot close over value %1 defined in outer scope",

lib/Sema/CSApply.cpp

@@ -7817,7 +7817,9 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
   // If we didn't manage to resolve directly to an expression, we don't
   // have a great diagnostic to give, so bail.
   if (!resolved || !resolved->getAnchor() ||
-      !resolved->getPath().empty())
+      (!resolved->getPath().empty() &&
+       fix.first.getKind() != FixKind::ExplicitlyEscaping &&
+       fix.first.getKind() != FixKind::ExplicitlyEscapingToAny))
     return false;
 
   Expr *affected = resolved->getAnchor();
@@ -7974,6 +7976,30 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
     }
     return false;
   }
+
+  case FixKind::ExplicitlyEscaping: {
+    auto path = locator->getPath();
+    auto *anchor = locator->getAnchor();
+
+    if (path.empty())
+      return false;
+
+    auto &last = path.back();
+    if (last.getKind() == ConstraintLocator::Archetype) {
+      auto *archetype = last.getArchetype();
+      TC.diagnose(anchor->getLoc(), diag::converting_noescape_to_type,
+                  archetype);
+      return true;
+    }
+    break;
+  }
+
+  case FixKind::ExplicitlyEscapingToAny: {
+    auto *anchor = locator->getAnchor();
+    TC.diagnose(anchor->getLoc(), diag::converting_noescape_to_type,
+                getASTContext().TheAnyType);
+    return true;
+  }
   }
 
   // FIXME: It would be really nice to emit a follow-up note showing where

lib/Sema/CSBindings.cpp

@@ -277,7 +277,7 @@ ConstraintSystem::getPotentialBindingForRelationalConstraint(
   // should be allowed to escape. As a result we allow anything
   // passed in to escape.
   if (auto *fnTy = type->getAs<AnyFunctionType>())
-    if (typeVar->getImpl().getArchetype())
+    if (typeVar->getImpl().getArchetype() && !shouldAttemptFixes())
       type = fnTy->withExtInfo(fnTy->getExtInfo().withNoEscape(false));
 
   // Check whether we can perform this binding.

lib/Sema/CSSimplify.cpp

@@ -672,9 +672,16 @@ matchCallArguments(ConstraintSystem &cs, ConstraintKind kind,
 
     // Disallow assignment of noescape function to parameter of type
     // Any. Allowing this would allow these functions to escape.
-    if (auto *fnTy = argType->getAs<AnyFunctionType>())
-      if (fnTy->isNoEscape())
+    if (auto *fnTy = argType->getAs<AnyFunctionType>()) {
+      if (fnTy->isNoEscape()) {
+        // Allow assigned of 'no-escape' function with recorded fix.
+        if (cs.shouldAttemptFixes() &&
+            !cs.recordFix(FixKind::ExplicitlyEscaping, locator))
+          return cs.getTypeMatchSuccess();
+
         return cs.getTypeMatchFailure(locator);
+      }
+    }
 
     return cs.getTypeMatchSuccess();
   }
@@ -1317,6 +1324,10 @@ ConstraintSystem::matchExistentialTypes(Type type1, Type type2,
     if (!fnTy || !fnTy->isNoEscape())
       return getTypeMatchSuccess();
 
+    if (shouldAttemptFixes() &&
+        !recordFix(FixKind::ExplicitlyEscapingToAny, locator))
+      return getTypeMatchSuccess();
+
     return getTypeMatchFailure(locator);
   }
 
@@ -1490,10 +1501,18 @@ ConstraintSystem::matchTypesBindTypeVar(
   // Disallow bindings of noescape functions to type variables that
   // represent an opened archetype. If we allowed this it would allow
   // the noescape function to potentially escape.
-  if (auto *fnTy = type->getAs<AnyFunctionType>())
-    if (fnTy->isNoEscape())
-      if (typeVar->getImpl().getArchetype())
+  if (auto *fnTy = type->getAs<AnyFunctionType>()) {
+    if (fnTy->isNoEscape() && typeVar->getImpl().getArchetype()) {
+      if (shouldAttemptFixes()) {
+        if (recordFix(FixKind::ExplicitlyEscaping, locator))
+          return getTypeMatchFailure(locator);
+
+        // Allow no-escape function to be bound with recorded fix.
+      } else {
         return getTypeMatchFailure(locator);
+      }
+    }
+  }
 
   // Check whether the type variable must be bound to a materializable
   // type.
@@ -2011,9 +2030,20 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
       // Penalize conversions to Any, and disallow conversions of
       // noescape functions to Any.
       if (kind >= ConstraintKind::Conversion && type2->isAny()) {
-        if (auto *fnTy = type1->getAs<AnyFunctionType>())
-          if (fnTy->isNoEscape())
-            return getTypeMatchFailure(locator);
+        if (auto *fnTy = type1->getAs<AnyFunctionType>()) {
+          if (fnTy->isNoEscape()) {
+            if (shouldAttemptFixes()) {
+              if (recordFix(FixKind::ExplicitlyEscapingToAny, locator))
+                return getTypeMatchFailure(locator);
+
+              // Allow 'no-escape' functions to be converted to 'Any'
+              // with a recorded fix that helps us to properly diagnose
+              // such situations.
+            } else {
+              return getTypeMatchFailure(locator);
+            }
+          }
+        }
 
         increaseScore(ScoreKind::SK_EmptyExistentialConversion);
       }
@@ -4757,7 +4787,17 @@ bool ConstraintSystem::recordFix(Fix fix, ConstraintLocatorBuilder locator) {
   if (worseThanBestSolution())
     return true;
 
-  Fixes.push_back({fix, getConstraintLocator(locator)});
+  auto *loc = getConstraintLocator(locator);
+  auto existingFix =
+      llvm::find_if(Fixes, [&](std::pair<Fix, ConstraintLocator *> &e) {
+        // If we already have a fix like this recorded, let's not do it again,
+        // this situation might happen when the same fix kind is applicable to
+        // different overload choices.
+        return e.first == fix && e.second == loc;
+      });
+
+  if (existingFix == Fixes.end())
+    Fixes.push_back({fix, loc});
 
   return false;
 }
@@ -4802,6 +4842,8 @@ ConstraintSystem::simplifyFixConstraint(Fix fix, Type type1, Type type2,
     return result;
   }
 
+  case FixKind::ExplicitlyEscaping:
+  case FixKind::ExplicitlyEscapingToAny:
   case FixKind::CoerceToCheckedCast:
     llvm_unreachable("handled elsewhere");
   }

lib/Sema/Constraint.cpp

@@ -504,6 +504,9 @@ StringRef Fix::getName(FixKind kind) {
     return "fix: add address-of";
   case FixKind::CoerceToCheckedCast:
     return "fix: as to as!";
+  case FixKind::ExplicitlyEscaping:
+  case FixKind::ExplicitlyEscapingToAny:
+    return "fix: add @escaping";
   }
 
   llvm_unreachable("Unhandled FixKind in switch.");

lib/Sema/Constraint.h

@@ -232,7 +232,7 @@ enum RememberChoice_t : bool {
 enum class FixKind : uint8_t {
   /// Introduce a '!' to force an optional unwrap.
   ForceOptional,
-    
+
   /// Introduce a '?.' to begin optional chaining.
   OptionalChaining,
 
@@ -241,9 +241,14 @@ enum class FixKind : uint8_t {
 
   /// Introduce a '&' to take the address of an lvalue.
   AddressOf,
-  
+
   /// Replace a coercion ('as') with a forced checked cast ('as!').
   CoerceToCheckedCast,
+
+  /// Mark function type as explicitly '@escaping'.
+  ExplicitlyEscaping,
+  /// Mark function type as explicitly '@escaping' to be convertable to 'Any'.
+  ExplicitlyEscapingToAny,
 };
 
 /// Describes a fix that can be applied to a constraint before visiting it.
@@ -279,6 +284,8 @@ class Fix {
   LLVM_ATTRIBUTE_DEPRECATED(void dump(ConstraintSystem *cs) const 
                               LLVM_ATTRIBUTE_USED,
                             "only for use within the debugger");
+
+  bool operator==(Fix const &b) { return Kind == b.Kind && Data == b.Data; }
 };
 
 

test/Constraints/function.swift

@@ -86,8 +86,6 @@ sr590((1, 2))
 
 // SR-2657: Poor diagnostics when function arguments should be '@escaping'.
 private class SR2657BlockClass<T> {
-  // expected-note@-1 {{'T' declared as parameter to type 'SR2657BlockClass'}}
-  // expected-note@-2 {{'T' declared as parameter to type 'SR2657BlockClass'}}
   let f: T
   init(f: T) { self.f = f }
 }
@@ -96,17 +94,15 @@ func takesAny(_: Any) {}
 
 func foo(block: () -> (), other: () -> Int) { // expected-note 2 {{parameter 'block' is implicitly non-escaping}}
   let _ = SR2657BlockClass(f: block)
-  // expected-error@-1 {{generic parameter 'T' could not be inferred}}
-  // expected-note@-2 {{explicitly specify the generic arguments to fix this issue}}
+  // expected-error@-1 {{converting non-escaping value to 'T' may allow it to escape}}
   let _ = SR2657BlockClass<()->()>(f: block)
   // expected-error@-1 {{passing non-escaping parameter 'block' to function expecting an @escaping closure}}
   let _: SR2657BlockClass<()->()> = SR2657BlockClass(f: block)
-  // expected-error@-1 {{generic parameter 'T' could not be inferred}}
-  // expected-note@-2 {{explicitly specify the generic arguments to fix this issue}}
+  // expected-error@-1 {{converting non-escaping value to 'T' may allow it to escape}}
   let _: SR2657BlockClass<()->()> = SR2657BlockClass<()->()>(f: block)
   // expected-error@-1 {{passing non-escaping parameter 'block' to function expecting an @escaping closure}}
-  _ = SR2657BlockClass<Any>(f: block)  // expected-error{{function produces expected type '()'; did you mean to call it with '()'?}}
-  _ = SR2657BlockClass<Any>(f: other) // expected-error{{function produces expected type 'Int'; did you mean to call it with '()'?}}
-  takesAny(block)  // expected-error{{function produces expected type '()'; did you mean to call it with '()'?}}
-  takesAny(other) // expected-error{{function produces expected type 'Int'; did you mean to call it with '()'?}}
+  _ = SR2657BlockClass<Any>(f: block)  // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}
+  _ = SR2657BlockClass<Any>(f: other) // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}
+  takesAny(block)  // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}
+  takesAny(other) // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}
 }

test/Constraints/function_conversion.swift

@@ -49,7 +49,7 @@ func takesAny(_ f: Any)  {}
 
 func twoFns(_ f: (Int) -> Int, _ g: @escaping (Int) -> Int) {
   // expected-note@-1 {{parameter 'f' is implicitly non-escaping}}
-  takesAny(f) // expected-error {{argument type '(Int) -> Int' does not conform to expected type 'Any'}}
+  takesAny(f) // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}
   takesAny(g)
   var h = g
   h = f // expected-error {{assigning non-escaping parameter 'f' to an @escaping closure}}
@@ -63,4 +63,4 @@ var escapingParam: (@escaping (Int) -> Int) -> () = consumeEscaping
 noEscapeParam = escapingParam // expected-error {{cannot assign value of type '(@escaping (Int) -> Int) -> ()' to type '((Int) -> Int) -> ()}}
 
 escapingParam = takesAny
-noEscapeParam = takesAny // expected-error {{cannot assign value of type '(Any) -> ()' to type '((Int) -> Int) -> ()'}}
+noEscapeParam = takesAny // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}

test/Constraints/suspicious_bit_casts.swift

@@ -2,7 +2,7 @@
 
 func escapeByBitCast(f: () -> ()) -> () -> () {
   return unsafeBitCast(f, to: (() -> ()).self)
-  // expected-error@-1 {{generic parameter 'T' could not be inferred}}
+  // expected-error@-1 {{converting non-escaping value to 'T' may allow it to escape}}
 }
 
 func changeFnRep(f: @escaping () -> ()) -> @convention(block) () -> () {

test/attr/attr_noescape.swift

@@ -13,7 +13,6 @@ func takesGenericClosure<T>(_ a : Int, _ fn : @noescape () -> T) {} // expected-
 var globalAny: Any = 0
 
 func assignToGlobal<T>(_ t: T) {
-  // expected-note@-1 {{in call to function 'assignToGlobal'}}
   globalAny = t
 }
 
@@ -56,15 +55,14 @@ func takesNoEscapeClosure(_ fn : () -> Int) {
   takesGenericClosure(4, fn)       // ok
   takesGenericClosure(4) { fn() }  // ok.
 
-  _ = [fn] // expected-error {{type of expression is ambiguous without more context}}
+  _ = [fn] // expected-error {{converting non-escaping value to 'Element' may allow it to escape}}
   _ = [doesEscape(fn)] // expected-error {{'(() -> Int) -> ()' is not convertible to '(@escaping () -> Int) -> ()'}}
-  _ = [1 : fn] // expected-error {{type of expression is ambiguous without more context}}
+  _ = [1 : fn] // expected-error {{converting non-escaping value to 'Value' may allow it to escape}}
   _ = [1 : doesEscape(fn)] // expected-error {{passing non-escaping parameter 'fn' to function expecting an @escaping closure}}
   _ = "\(doesEscape(fn))" // expected-error {{passing non-escaping parameter 'fn' to function expecting an @escaping closure}}
   _ = "\(takesArray([fn]))" // expected-error {{using non-escaping parameter 'fn' in a context expecting an @escaping closure}}
 
-  // FIXME: Generate a more specific error about the non-escaping parameter 'fn'
-  assignToGlobal(fn) // expected-error {{generic parameter 'T' could not be inferred}}
+  assignToGlobal(fn) // expected-error {{converting non-escaping value to 'T' may allow it to escape}}
 }
 
 class SomeClass {