Merge pull request swiftlang#24431 from xedin/diag-noescape-param-assignment

[ConstraintSystem] Improve @escaping parameter diagnostics

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -3075,10 +3075,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, Type))
 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

@@ -417,6 +417,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);
@@ -441,6 +444,138 @@ 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;
+
+  auto getGenericParamType =
+      [](TypeVariableType *typeVar) -> GenericTypeParamType * {
+    auto *locator = typeVar->getImpl().getLocator();
+    if (locator->isForGenericParameter()) {
+      const auto &GP = locator->getPath().back();
+      return GP.getGenericParameter();
+    }
+    return nullptr;
+  };
+
+  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 (paramType->isTypeVariableOrMember()) {
+          auto diagnoseGenericParamFailure = [&](Type genericParam,
+                                                 GenericTypeParamDecl *decl) {
+            emitDiagnostic(anchor->getLoc(),
+                           diag::converting_noespace_param_to_generic_type,
+                           PD->getName(), genericParam);
+
+            emitDiagnostic(decl, diag::generic_parameters_always_escaping);
+          };
+
+          // 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 *DMT = paramType->getAs<DependentMemberType>()) {
+            auto baseTy = DMT->getBase()->castTo<TypeVariableType>();
+            diagnoseGenericParamFailure(resolveType(DMT),
+                                        getGenericParamType(baseTy)->getDecl());
+            return true;
+          }
+
+          auto *typeVar = paramType->getAs<TypeVariableType>();
+          if (auto *GP = getGenericParamType(typeVar)) {
+            diagnoseGenericParamFailure(GP, GP->getDecl());
+            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];
+    return param.getPlainType();
+  }
+
+  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(),

lib/Sema/ConstraintLocator.cpp

@@ -170,6 +170,12 @@ bool ConstraintLocator::isForKeyPathComponent() const {
   });
 }
 
+bool ConstraintLocator::isForGenericParameter() const {
+  auto path = getPath();
+  return !path.empty() &&
+         path.back().getKind() == ConstraintLocator::GenericParameter;
+}
+
 void ConstraintLocator::dump(SourceManager *sm) {
   dump(sm, llvm::errs());
   llvm::errs() << "\n";

lib/Sema/ConstraintLocator.h

@@ -568,6 +568,9 @@ class ConstraintLocator : public llvm::FoldingSetNode {
   /// components.
   bool isForKeyPathComponent() const;
 
+  /// Determine whether this locator points to the generic parameter.
+  bool isForGenericParameter() const;
+
   /// Produce a profile of this locator, for use in a folding set.
   static void Profile(llvm::FoldingSetNodeID &id, Expr *anchor,
                       ArrayRef<PathElement> path);

test/Constraints/function.swift

@@ -85,24 +85,43 @@ 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}}
   takesAny(other) // expected-error {{converting non-escaping value to 'Any' may allow it to escape}}
 }
+
+protocol P {
+  associatedtype U
+}
+
+func test_passing_noescape_function_to_dependent_member() {
+  struct S<T : P> { // expected-note {{generic parameters are always considered '@escaping'}}
+    func foo(_: T.U) {}
+  }
+
+  struct Q : P {
+    typealias U = () -> Int
+  }
+
+  func test(_ s: S<Q>, fn: () -> Int) {
+    s.foo(fn)
+    // expected-error@-1 {{converting non-escaping parameter 'fn' to generic parameter 'Q.U' (aka '() -> Int') 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