Merge pull request #23907 from slavapestov/noescape-parameter-call-restriction

Re-implement "no-escape parameter call restriction" as SIL pass

Author: slavapestov

include/swift/AST/DiagnosticsSIL.def

@@ -137,6 +137,12 @@ NOTE(value_captured_here,none,"captured here", ())
 NOTE(value_captured_transitively,none,
      "captured indirectly by this call", ())
 
+ERROR(err_noescape_param_call,none,
+      "passing a %select{|closure which captures a }1non-escaping function "
+      "parameter %0 to a call to a non-escaping function parameter can allow "
+      "re-entrant modification of a variable",
+      (DeclName, unsigned))
+
 // Definite initialization diagnostics.
 NOTE(variable_defined_here,none,
      "%select{variable|constant}0 defined here", (bool))

include/swift/AST/DiagnosticsSema.def

@@ -289,11 +289,6 @@ ERROR(incorrect_explicit_closure_result,none,
       "declared closure result %0 is incompatible with contextual type %1",
       (Type, Type))
 
-ERROR(err_noescape_param_call,none,
-      "passing a %select{|closure which captures a }1non-escaping function "
-      "parameter %0 to a call to a non-escaping function parameter can allow "
-      "re-entrant modification of a variable",
-      (DeclName, unsigned))
 ERROR(cannot_call_function_value,none,
       "cannot invoke value of function type with argument list '%0'",
       (StringRef))

lib/SILOptimizer/Mandatory/DiagnoseInvalidEscapingCaptures.cpp

@@ -329,11 +329,72 @@ static void checkPartialApply(ASTContext &Context, DeclContext *DC,
   }
 }
 
+static void checkApply(ASTContext &Context, FullApplySite site) {
+  auto isNoEscapeParam = [&](SILValue value) -> const ParamDecl * {
+    // If the value is an escaping, do not enforce any restrictions.
+    if (!value->getType().getASTType()->isNoEscape())
+      return nullptr;
+
+    // If the value is not a function parameter, do not enforce any restrictions.
+    return getParamDeclFromOperand(value);
+  };
+
+  // If the callee is not a no-escape parameter, there is nothing to check.
+  auto callee = site.getCalleeOrigin();
+  if (!isNoEscapeParam(callee))
+    return;
+
+  // See if any of our arguments are noescape parameters, or closures capturing
+  // noescape parameters.
+  SmallVector<std::pair<SILValue, bool>, 4> args;
+  llvm::SmallDenseSet<SILValue, 4> visited;
+  auto arglistInsert = [&](SILValue arg, bool capture) {
+    if (visited.insert(arg).second)
+      args.emplace_back(arg, capture);
+  };
+
+  for (auto arg : site.getArguments())
+    arglistInsert(arg, /*capture=*/false);
+
+  while (!args.empty()) {
+    auto pair = args.pop_back_val();
+    auto arg = pair.first;
+    bool capture = pair.second;
+
+    if (auto *CI = dyn_cast<ConversionInst>(arg)) {
+      arglistInsert(CI->getConverted(), /*capture=*/false);
+      continue;
+    }
+
+    // If one of our call arguments is a noescape parameter, diagnose the
+    // violation.
+    if (auto *param = isNoEscapeParam(arg)) {
+      diagnose(Context, site.getLoc(), diag::err_noescape_param_call,
+               param->getName(), capture);
+      return;
+    }
+
+    // If one of our call arguments is a closure, recursively visit all of
+    // the closure's captures.
+    if (auto *PAI = dyn_cast<PartialApplyInst>(arg)) {
+      ApplySite site(PAI);
+      for (auto arg : site.getArguments())
+        arglistInsert(arg, /*capture=*/true);
+      continue;
+    }
+  }
+}
+
 static void checkForViolationsAtInstruction(ASTContext &Context,
                                             DeclContext *DC,
                                             SILInstruction *I) {
   if (auto *PAI = dyn_cast<PartialApplyInst>(I))
     checkPartialApply(Context, DC, PAI);
+
+  if (isa<ApplyInst>(I) || isa<TryApplyInst>(I)) {
+    FullApplySite site(I);
+    checkApply(Context, site);
+  }
 }
 
 static void checkEscapingCaptures(SILFunction *F) {

lib/Sema/MiscDiagnostics.cpp

@@ -179,10 +179,6 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
 
         ConcreteDeclRef callee;
         if (auto *calleeDRE = dyn_cast<DeclRefExpr>(base)) {
-          // This only cares about declarations of noescape function type.
-          auto AFT = calleeDRE->getType()->getAs<FunctionType>();
-          if (AFT && AFT->isNoEscape())
-            checkNoEscapeParameterCall(Call);
           checkForSuspiciousBitCasts(calleeDRE, Call);
           callee = calleeDRE->getDeclRef();
 
@@ -416,104 +412,6 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
       TC.diagnose(E->getStartLoc(), diag::value_of_module_type);
     }
 
-    class NoEscapeArgument {
-      llvm::PointerIntPair<ParamDecl*, 1, bool> ParamAndIsCapture;
-    public:
-      NoEscapeArgument() {}
-      NoEscapeArgument(ParamDecl *param, bool isCapture)
-          : ParamAndIsCapture(param, isCapture) {
-        assert(param);
-      }
-
-      explicit operator bool() const {
-        return ParamAndIsCapture.getPointer() != nullptr;
-      }
-
-      ParamDecl *getDecl() const { return ParamAndIsCapture.getPointer(); }
-      bool isDeclACapture() const { return ParamAndIsCapture.getInt(); }
-
-      static NoEscapeArgument find(TypeChecker &tc, ValueDecl *decl,
-                                   bool isCapture) {
-        if (auto param = dyn_cast<ParamDecl>(decl)) {
-          if (auto fnType =
-                param->getInterfaceType()->getAs<AnyFunctionType>()) {
-            if (fnType->isNoEscape())
-              return { param, isCapture };
-          }
-          return {};
-        }
-
-        if (auto fn = dyn_cast<AbstractFunctionDecl>(decl)) {
-          if (fn->getDeclContext()->isLocalContext()) {
-            return findInCaptures(tc, fn);
-          }
-          return {};
-        }
-
-        // FIXME: captures of computed local vars?  Can these be non-escaping?
-        return {};
-      }
-
-      static NoEscapeArgument findInCaptures(TypeChecker &tc,
-                                             AnyFunctionRef fn) {
-        // Ensure we have accurate capture information for the function.
-        tc.computeCaptures(fn);
-
-        for (const auto &capture : fn.getCaptureInfo().getCaptures()) {
-          if (capture.isDynamicSelfMetadata()) continue;
-          if (auto param = find(tc, capture.getDecl(), true))
-            return param;
-        }
-        return {};
-      }
-    };
-
-    /// Enforce the exclusivity rule against calling a non-escaping
-    /// function parameter with another non-escaping function parameter
-    /// as an argument.
-    void checkNoEscapeParameterCall(ApplyExpr *apply) {
-      NoEscapeArgument noescapeArgument;
-      Expr *problematicArg = nullptr;
-
-      visitArguments(apply, [&](unsigned argIndex, Expr *arg) {
-        // Just find the first problematic argument.
-        if (noescapeArgument) return;
-
-        // Remember the expression which used the argument.
-        problematicArg = arg;
-
-        // Look through the same set of nodes that we look through when
-        // checking for no-escape functions.
-        arg = lookThroughArgument(arg);
-
-        // If the argument isn't noescape, ignore it.
-        auto fnType = arg->getType()->getAs<AnyFunctionType>();
-        if (!fnType || !fnType->isNoEscape())
-          return;
-
-        // Okay, it should be a closure or a decl ref.
-        if (auto declRef = dyn_cast<DeclRefExpr>(arg)) {
-          noescapeArgument =
-            NoEscapeArgument::find(TC, declRef->getDecl(), false);
-        } else if (auto closure = dyn_cast<AbstractClosureExpr>(arg)) {
-          noescapeArgument =
-            NoEscapeArgument::findInCaptures(TC, closure);
-        } else {
-          // This can happen with withoutActuallyEscaping.
-          assert(isa<OpaqueValueExpr>(arg) &&
-                 "unexpected expression yielding noescape closure");
-        }
-      });
-
-      if (!noescapeArgument) return;
-
-      TC.diagnose(apply->getLoc(),
-                  diag::err_noescape_param_call,
-                  noescapeArgument.getDecl()->getName(),
-                  noescapeArgument.isDeclACapture())
-        .highlight(problematicArg->getSourceRange());
-    }
-
     // Diagnose metatype values that don't appear as part of a property,
     // method, or constructor reference.
     void checkUseOfMetaTypeName(Expr *E) {

test/SILOptimizer/noescape_param_exclusivity.swift

@@ -0,0 +1,48 @@
+// RUN: %target-swift-frontend -emit-sil %s -verify
+
+func test0(a: (() -> ()) -> (), b: () -> ()) {
+  a(b) // expected-error {{passing a non-escaping function parameter 'b' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
+}
+
+func test0(fn: (() -> ()) -> ()) {
+  fn { fn {} } // expected-error {{passing a closure which captures a non-escaping function parameter 'fn' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
+}
+
+func test1(fn: (() -> ()) -> ()) {
+  func foo() {
+    fn { fn {} } // expected-error {{passing a closure which captures a non-escaping function parameter 'fn' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
+  }
+}
+
+func test2(x: inout Int, fn: (() -> ()) -> ()) {
+  func foo(myfn: () -> ()) {
+    x += 1
+    myfn()
+  }
+
+  // Make sure we only complain about calls to noescape parameters.
+  foo { fn {} }
+}
+
+func test3(fn: (() -> ()) -> ()) {
+  { myfn in myfn { fn {} } }(fn) // expected-error {{passing a closure which captures a non-escaping function parameter 'fn' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
+}
+
+func test4(fn: (() -> ()) -> ()) {
+  func foo() {
+    fn {}
+  }
+
+  fn(foo) // expected-error {{passing a closure which captures a non-escaping function parameter 'fn' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
+}
+
+// rdar://problem/34496304
+func test5(outer: (() throws -> Int) throws -> Int) throws -> Int {
+  func descend(_ inner: (() throws -> Int) throws -> Int) throws -> Int {
+    return try inner { // expected-error {{passing a closure which captures a non-escaping function parameter 'inner' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
+      try descend(inner)
+    }
+  }
+
+  return try descend(outer)
+}

test/attr/attr_noescape.swift

@@ -303,7 +303,6 @@ func doThing4(_ completion: @escaping CompletionHandler) {
 func apply<T, U>(_ f: @noescape (T) -> U, g: @noescape (@noescape (T) -> U) -> U) -> U { 
   // expected-error@-1 6{{unknown attribute 'noescape'}}
   return g(f)
-  // expected-error@-1 {{passing a non-escaping function parameter 'f' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
 }
 
 // <rdar://problem/19997577> @noescape cannot be applied to locals, leading to duplication of code