[ConstraintSystem] Improve @escaping parameter diagnostics

Detect difference in escapiness while matching function types
in the solver and record a fix that suggests to add @escaping
attribute where appropriate.

Also emit a tailored diagnostic when non-escaping parameter
type is used as a type of a generic parameter.

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -3073,10 +3073,15 @@ NOTE(silence_debug_description_in_interpolation_segment_call,none,
 NOTE(noescape_parameter,none,
     "parameter %0 is implicitly non-escaping",
      (Identifier))
+NOTE(generic_parameters_always_escaping,none,
+     "generic parameters are always considered '@escaping'", ())
 
 ERROR(passing_noescape_to_escaping,none,
       "passing non-escaping parameter %0 to function expecting an @escaping closure",
       (Identifier))
+ERROR(converting_noespace_param_to_generic_type,none,
+      "converting non-escaping parameter %0 to generic parameter %1 may allow it to escape",
+      (Identifier, Identifier))
 ERROR(assigning_noescape_to_escaping,none,
       "assigning non-escaping parameter %0 to an @escaping closure",
       (Identifier))

lib/Sema/CSDiag.cpp

@@ -173,17 +173,26 @@ void constraints::simplifyLocator(Expr *&anchor,
       continue;
 
     case ConstraintLocator::NamedTupleElement:
-    case ConstraintLocator::TupleElement:
+    case ConstraintLocator::TupleElement: {
       // Extract tuple element.
+      unsigned index = path[0].getValue();
       if (auto tupleExpr = dyn_cast<TupleExpr>(anchor)) {
-        unsigned index = path[0].getValue();
         if (index < tupleExpr->getNumElements()) {
           anchor = tupleExpr->getElement(index);
           path = path.slice(1);
           continue;
         }
       }
+
+      if (auto *CE = dyn_cast<CollectionExpr>(anchor)) {
+        if (index < CE->getNumElements()) {
+          anchor = CE->getElement(index);
+          path = path.slice(1);
+          continue;
+        }
+      }
       break;
+    }
 
     case ConstraintLocator::ApplyArgToParam:
       // Extract tuple element.
@@ -1994,34 +2003,9 @@ bool FailureDiagnosis::diagnoseNonEscapingParameterToEscaping(
   if (!srcFT || !dstFT || !srcFT->isNoEscape() || dstFT->isNoEscape())
     return false;
 
-  // Pick a specific diagnostic for the specific use
-  auto paramDecl = cast<ParamDecl>(declRef->getDecl());
-  switch (dstPurpose) {
-  case CTP_CallArgument:
-    CS.TC.diagnose(declRef->getLoc(), diag::passing_noescape_to_escaping,
-                   paramDecl->getName());
-    break;
-  case CTP_AssignSource:
-    CS.TC.diagnose(declRef->getLoc(), diag::assigning_noescape_to_escaping,
-                   paramDecl->getName());
-    break;
-
-  default:
-    CS.TC.diagnose(declRef->getLoc(), diag::general_noescape_to_escaping,
-                   paramDecl->getName());
-    break;
-  }
-
-  // Give a note and fixit
-  InFlightDiagnostic note = CS.TC.diagnose(
-      paramDecl->getLoc(), diag::noescape_parameter, paramDecl->getName());
-
-  if (!paramDecl->isAutoClosure()) {
-    note.fixItInsert(paramDecl->getTypeLoc().getSourceRange().Start,
-                     "@escaping ");
-  } // TODO: add in a fixit for autoclosure
-
-  return true;
+  NoEscapeFuncToTypeConversionFailure failure(
+      expr, CS, CS.getConstraintLocator(expr), dstType);
+  return failure.diagnoseAsError();
 }
 
 bool FailureDiagnosis::diagnoseContextualConversionError(

lib/Sema/CSDiagnostics.cpp

@@ -426,6 +426,9 @@ bool LabelingFailure::diagnoseAsError() {
 bool NoEscapeFuncToTypeConversionFailure::diagnoseAsError() {
   auto *anchor = getAnchor();
 
+  if (diagnoseParameterUse())
+    return true;
+
   if (ConvertTo) {
     emitDiagnostic(anchor->getLoc(), diag::converting_noescape_to_type,
                    ConvertTo);
@@ -450,6 +453,123 @@ bool NoEscapeFuncToTypeConversionFailure::diagnoseAsError() {
   return true;
 }
 
+bool NoEscapeFuncToTypeConversionFailure::diagnoseParameterUse() const {
+  // If the other side is not a function, we have common case diagnostics
+  // which handle function-to-type conversion diagnostics.
+  if (!ConvertTo || !ConvertTo->is<FunctionType>())
+    return false;
+
+  auto *anchor = getAnchor();
+  auto *locator = getLocator();
+  auto diagnostic = diag::general_noescape_to_escaping;
+
+  ParamDecl *PD = nullptr;
+  if (auto *DRE = dyn_cast<DeclRefExpr>(anchor)) {
+    PD = dyn_cast<ParamDecl>(DRE->getDecl());
+
+    // If anchor is not a parameter declaration there
+    // is no need to dig up more information.
+    if (!PD)
+      return false;
+
+    // Let's check whether this is a function parameter passed
+    // as an argument to another function which accepts @escaping
+    // function at that position.
+    auto path = locator->getPath();
+    if (!path.empty() &&
+        (path.back().getKind() == ConstraintLocator::ApplyArgToParam)) {
+      if (auto paramType =
+              getParameterTypeFor(getRawAnchor(), path.back().getValue2())) {
+        // If this is a situation when non-escaping parameter is passed
+        // to the argument which represents generic parameter, there is
+        // a tailored diagnostic for that.
+        if (auto *GP = paramType->getAs<GenericTypeParamType>()) {
+          emitDiagnostic(anchor->getLoc(),
+                         diag::converting_noespace_param_to_generic_type,
+                         PD->getName(), GP->getName());
+
+          emitDiagnostic(GP->getDecl(),
+                         diag::generic_parameters_always_escaping);
+          return true;
+        }
+      }
+
+      // If there are no generic parameters involved, this could
+      // only mean that parameter is expecting @escaping function type.
+      diagnostic = diag::passing_noescape_to_escaping;
+    }
+  } else if (auto *AE = dyn_cast<AssignExpr>(getRawAnchor())) {
+    if (auto *DRE = dyn_cast<DeclRefExpr>(AE->getSrc())) {
+      PD = dyn_cast<ParamDecl>(DRE->getDecl());
+      diagnostic = diag::assigning_noescape_to_escaping;
+    }
+  }
+
+  if (!PD)
+    return false;
+
+  emitDiagnostic(anchor->getLoc(), diagnostic, PD->getName());
+
+  // Give a note and fix-it
+  auto note =
+      emitDiagnostic(PD->getLoc(), diag::noescape_parameter, PD->getName());
+
+  if (!PD->isAutoClosure()) {
+    note.fixItInsert(PD->getTypeLoc().getSourceRange().Start, "@escaping ");
+  } // TODO: add in a fixit for autoclosure
+
+  return true;
+}
+
+Type NoEscapeFuncToTypeConversionFailure::getParameterTypeFor(
+    Expr *expr, unsigned paramIdx) const {
+  auto &cs = getConstraintSystem();
+  ConstraintLocator *locator = nullptr;
+
+  if (auto *call = dyn_cast<CallExpr>(expr)) {
+    auto *fnExpr = call->getFn();
+    if (isa<UnresolvedDotExpr>(fnExpr)) {
+      locator = cs.getConstraintLocator(fnExpr, ConstraintLocator::Member);
+    } else if (isa<UnresolvedMemberExpr>(fnExpr)) {
+      locator =
+          cs.getConstraintLocator(fnExpr, ConstraintLocator::UnresolvedMember);
+    } else if (auto *TE = dyn_cast<TypeExpr>(fnExpr)) {
+      locator = cs.getConstraintLocator(call,
+                                        {ConstraintLocator::ApplyFunction,
+                                         ConstraintLocator::ConstructorMember},
+                                        /*summaryFlags=*/0);
+    } else {
+      locator = cs.getConstraintLocator(fnExpr);
+    }
+  } else if (auto *SE = dyn_cast<SubscriptExpr>(expr)) {
+    locator = cs.getConstraintLocator(SE, ConstraintLocator::SubscriptMember);
+  }
+
+  if (!locator)
+    return Type();
+
+  auto choice = getOverloadChoiceIfAvailable(locator);
+  if (!choice)
+    return Type();
+
+  if (auto *fnType = choice->openedType->getAs<FunctionType>()) {
+    const auto &param = fnType->getParams()[paramIdx];
+
+    auto paramType = param.getPlainType();
+    if (auto *typeVar = paramType->getAs<TypeVariableType>()) {
+      auto *locator = typeVar->getImpl().getLocator();
+      if (locator->isForGenericParameter()) {
+        const auto &GP = locator->getPath().back();
+        return GP.getGenericParameter();
+      }
+    }
+
+    return paramType;
+  }
+
+  return Type();
+}
+
 bool MissingForcedDowncastFailure::diagnoseAsError() {
   if (hasComplexLocator())
     return false;

lib/Sema/CSDiagnostics.h

@@ -466,6 +466,16 @@ class NoEscapeFuncToTypeConversionFailure final : public FailureDiagnostic {
       : FailureDiagnostic(expr, cs, locator), ConvertTo(toType) {}
 
   bool diagnoseAsError() override;
+
+private:
+  /// Emit tailored diagnostics for no-escape parameter conversions e.g.
+  /// passing such parameter as an @escaping argument, or trying to
+  /// assign it to a variable which expects @escaping function.
+  bool diagnoseParameterUse() const;
+
+  /// Retrieve a type of the parameter at give index for call or
+  /// subscript invocation represented by given expression node.
+  Type getParameterTypeFor(Expr *expr, unsigned paramIdx) const;
 };
 
 class MissingForcedDowncastFailure final : public FailureDiagnostic {

lib/Sema/CSSimplify.cpp

@@ -1320,8 +1320,16 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
   // A non-@noescape function type can be a subtype of a @noescape function
   // type.
   if (func1->isNoEscape() != func2->isNoEscape() &&
-      (func1->isNoEscape() || kind < ConstraintKind::Subtype))
-    return getTypeMatchFailure(locator);
+      (func1->isNoEscape() || kind < ConstraintKind::Subtype)) {
+    if (!shouldAttemptFixes())
+      return getTypeMatchFailure(locator);
+
+    auto *fix = MarkExplicitlyEscaping::create(
+        *this, getConstraintLocator(locator), func2);
+
+    if (recordFix(fix))
+      return getTypeMatchFailure(locator);
+  }
 
   if (matchFunctionRepresentations(func1->getExtInfo().getRepresentation(),
                                    func2->getExtInfo().getRepresentation(),

test/Constraints/function.swift

@@ -85,22 +85,22 @@ sr590(())
 sr590((1, 2))
 
 // SR-2657: Poor diagnostics when function arguments should be '@escaping'.
-private class SR2657BlockClass<T> {
+private class SR2657BlockClass<T> { // expected-note 3 {{generic parameters are always considered '@escaping'}}
   let f: T
   init(f: T) { self.f = f }
 }
 
 func takesAny(_: Any) {}
 
-func foo(block: () -> (), other: () -> Int) { // expected-note 2 {{parameter 'block' is implicitly non-escaping}}
+func foo(block: () -> (), other: () -> Int) {
   let _ = SR2657BlockClass(f: block)
   // 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}}
+  // expected-error@-1 {{converting non-escaping parameter 'block' to generic parameter 'T' may allow it to escape}}
   let _: SR2657BlockClass<()->()> = SR2657BlockClass(f: block)
-  // expected-error@-1 {{converting non-escaping value to 'T' may allow it to escape}}
+  // expected-error@-1 {{converting non-escaping parameter 'block' to generic parameter '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}}
+  // expected-error@-1 {{converting non-escaping parameter 'block' to generic parameter 'T' may allow it to escape}}
   _ = 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}}

test/Constraints/function_conversion.swift

@@ -59,7 +59,7 @@ takesAny(consumeEscaping)
 
 var noEscapeParam: ((Int) -> Int) -> () = consumeNoEscape
 var escapingParam: (@escaping (Int) -> Int) -> () = consumeEscaping
-noEscapeParam = escapingParam // expected-error {{cannot assign value of type '(@escaping (Int) -> Int) -> ()' to type '((Int) -> Int) -> ()}}
+noEscapeParam = escapingParam // expected-error {{converting non-escaping value to '(Int) -> Int' may allow it to escape}}
 
 escapingParam = takesAny
 noEscapeParam = takesAny // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}

test/attr/attr_autoclosure.swift

@@ -188,6 +188,7 @@ func rdar_20591571() {
 
   func id<T>(_: T) -> T {}
   func same<T>(_: T, _: T) {}
+  // expected-note@-1 2 {{generic parameters are always considered '@escaping'}}
 
   func takesAnAutoclosure(_ fn: @autoclosure () -> Int, _ efn: @escaping @autoclosure () -> Int) {
     // These are OK -- they count as non-escaping uses
@@ -198,8 +199,8 @@ func rdar_20591571() {
     let _ = efn
 
     _ = id(fn)          // expected-error {{converting non-escaping value to 'T' may allow it to escape}}
-    _ = same(fn, { 3 }) // expected-error {{converting non-escaping value to 'T' may allow it to escape}}
-    _ = same({ 3 }, fn) // expected-error {{converting non-escaping value to 'T' may allow it to escape}}
+    _ = same(fn, { 3 }) // expected-error {{converting non-escaping parameter 'fn' to generic parameter 'T' may allow it to escape}}
+    _ = same({ 3 }, fn) // expected-error {{converting non-escaping parameter 'fn' to generic parameter 'T' may allow it to escape}}
 
     withoutActuallyEscaping(fn) { _ in }              // Ok
     withoutActuallyEscaping(fn) { (_: () -> Int) in } // Ok

test/attr/attr_escaping.swift

@@ -39,6 +39,11 @@ struct StoresClosure {
     return [fn] // expected-error{{using non-escaping parameter 'fn' in a context expecting an @escaping closure}}
   }
 
+  func dictPack(_ fn: () -> Int) -> [String: () -> Int] {
+    // expected-note@-1{{parameter 'fn' is implicitly non-escaping}} {{23-23=@escaping }}
+    return ["ultimate answer": fn] // expected-error{{using non-escaping parameter 'fn' in a context expecting an @escaping closure}}
+  }
+
   func arrayPack(_ fn: @escaping () -> Int, _ fn2 : () -> Int) -> [() -> Int] {
     // expected-note@-1{{parameter 'fn2' is implicitly non-escaping}} {{53-53=@escaping }}
 
@@ -147,7 +152,8 @@ class FooClass {
   }
   var computedEscaping : (@escaping ()->Int)->Void {
     get { return stored! }
-    set(newValue) { stored = newValue } // expected-error{{cannot assign value of type '(@escaping () -> Int) -> Void' to type 'Optional<(() -> Int) -> Void>'}}
+    set(newValue) { stored = newValue } // expected-error{{assigning non-escaping parameter 'newValue' to an @escaping closure}}
+    // expected-note@-1 {{parameter 'newValue' is implicitly non-escaping}}
   }
 }
 

test/attr/attr_noescape.swift

@@ -203,9 +203,9 @@ func overloadedEach<P: P2, T>(_ source: P, _ transform: @escaping (P.Element) ->
 struct S : P2 {
   typealias Element = Int
   func each(_ transform: @noescape (Int) -> ()) { // expected-error{{unknown attribute 'noescape'}}
-    overloadedEach(self, // expected-error {{cannot invoke 'overloadedEach' with an argument list of type '(S, (Int) -> (), Int)'}}
-    // expected-note @-1 {{overloads for 'overloadedEach' exist with these partially matching parameter lists: (O, @escaping (O.Element) -> (), T), (P, @escaping (P.Element) -> (), T)}}
-                   transform, 1)
+                                                  // expected-note@-1 {{parameter 'transform' is implicitly non-escaping}}
+    overloadedEach(self, transform, 1)
+    // expected-error@-1 {{passing non-escaping parameter 'transform' to function expecting an @escaping closure}}
   }
 }