Implement the restriction on calls to non-escaping function parameter…

include/swift/AST/DiagnosticsSema.def

@@ -267,6 +267,18 @@ ERROR(incorrect_explicit_closure_result,none,
       "declared closure result %0 is incompatible with contextual type %1",
       (Type, Type))
 
+// FIXME: make this an error when we've fixed the Dispatch problem.
+WARNING(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))
+WARNING(warn_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 and will be illegal in Swift 4",
+      (DeclName, unsigned))
+
 ERROR(cannot_call_function_value,none,
       "cannot invoke value of function type with argument list '%0'",
       (StringRef))

lib/Sema/MiscDiagnostics.cpp

@@ -210,7 +210,7 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
         }
 
         // Verify noescape parameter uses.
-        checkNoEscapeParameterUse(DRE, nullptr);
+        checkNoEscapeParameterUse(DRE, nullptr, OperandKind::None);
 
         // Verify warn_unqualified_access uses.
         checkUnqualifiedAccessUse(DRE);
@@ -239,7 +239,7 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
       if (auto MakeEsc = dyn_cast<MakeTemporarilyEscapableExpr>(E)) {
         if (auto DRE =
               dyn_cast<DeclRefExpr>(MakeEsc->getNonescapingClosureValue()))
-          checkNoEscapeParameterUse(DRE, MakeEsc);
+          checkNoEscapeParameterUse(DRE, MakeEsc, OperandKind::MakeEscapable);
       }
 
       // Check function calls, looking through implicit conversions on the
@@ -263,33 +263,11 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
         while (auto ignoredBase = dyn_cast<DotSyntaxBaseIgnoredExpr>(Base))
           Base = ignoredBase->getRHS();
         if (auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
-          checkNoEscapeParameterUse(DRE, Call);
+          checkNoEscapeParameterUse(DRE, Call, OperandKind::Callee);
           checkForSuspiciousBitCasts(DRE, Call);
         }
 
-        auto *Arg = Call->getArg();
-
-        // The argument could be shuffled if it includes default arguments,
-        // label differences, or other exciting things like that.
-        if (auto *TSE = dyn_cast<TupleShuffleExpr>(Arg))
-          Arg = TSE->getSubExpr();
-
-        // The argument is either a ParenExpr or TupleExpr.
-        ArrayRef<Expr*> arguments;
-        SmallVector<Expr *, 1> Scratch;
-        if (auto *TE = dyn_cast<TupleExpr>(Arg))
-          arguments = TE->getElements();
-        else if (auto *PE = dyn_cast<ParenExpr>(Arg)) {
-          Scratch.push_back(PE->getSubExpr());
-          arguments = makeArrayRef(Scratch);
-        }
-        else {
-          Scratch.push_back(Call->getArg());
-          arguments = makeArrayRef(Scratch);
-        }
-
-        // Check each argument.
-        for (auto arg : arguments) {
+        visitArguments(Call, [&](Expr *arg) {
           // InOutExpr's are allowed in argument lists directly.
           if (auto *IOE = dyn_cast<InOutExpr>(arg)) {
             if (isa<CallExpr>(Call))
@@ -307,18 +285,12 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
                 AcceptableInOutExprs.insert(IOE);
           }
 
-          while (1) {
-            if (auto conv = dyn_cast<ImplicitConversionExpr>(arg))
-              arg = conv->getSubExpr();
-            else if (auto *PE = dyn_cast<ParenExpr>(arg))
-              arg = PE->getSubExpr();
-            else
-              break;
-          }
+          // Also give special treatment to noescape function arguments.
+          arg = lookThroughArgument(arg);
 
           if (auto *DRE = dyn_cast<DeclRefExpr>(arg))
-            checkNoEscapeParameterUse(DRE, Call);
-        }
+            checkNoEscapeParameterUse(DRE, Call, OperandKind::Argument);
+        });
       }
 
       // If we have an assignment expression, scout ahead for acceptable _'s.
@@ -359,6 +331,38 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
       return { true, E };
     }
 
+    static void visitArguments(ApplyExpr *apply,
+                               llvm::function_ref<void(Expr*)> fn) {
+      auto *arg = apply->getArg();
+
+      // The argument could be shuffled if it includes default arguments,
+      // label differences, or other exciting things like that.
+      if (auto *TSE = dyn_cast<TupleShuffleExpr>(arg))
+        arg = TSE->getSubExpr();
+
+      // The argument is either a ParenExpr or TupleExpr.
+      if (auto *TE = dyn_cast<TupleExpr>(arg)) {
+        for (auto elt : TE->getElements())
+          fn(elt);
+      } else if (auto *PE = dyn_cast<ParenExpr>(arg)) {
+        fn(PE->getSubExpr());
+      } else {
+        fn(arg);
+      }
+    }
+
+    static Expr *lookThroughArgument(Expr *arg) {
+      while (1) {
+        if (auto conv = dyn_cast<ImplicitConversionExpr>(arg))
+          arg = conv->getSubExpr();
+        else if (auto *PE = dyn_cast<ParenExpr>(arg))
+          arg = PE->getSubExpr();
+        else
+          break;
+      }
+      return arg;
+    }
+
     Expr *walkToExprPost(Expr *E) override {
       checkInvalidPartialApplication(E);
       return E;
@@ -446,9 +450,118 @@ 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, [&](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;
+
+      // In Swift 3, this is just a warning.
+      TC.diagnose(apply->getLoc(),
+                  TC.Context.isSwiftVersion3()
+                    ? diag::warn_noescape_param_call
+                    : diag::err_noescape_param_call,
+                  noescapeArgument.getDecl()->getName(),
+                  noescapeArgument.isDeclACapture())
+        .highlight(problematicArg->getSourceRange());
+    }
+
+    enum class OperandKind {
+      None,
+      Callee,
+      Argument,
+      MakeEscapable,
+    };
+
     /// The DRE argument is a reference to a noescape parameter.  Verify that
     /// its uses are ok.
-    void checkNoEscapeParameterUse(DeclRefExpr *DRE, Expr *ParentExpr=nullptr) {
+    void checkNoEscapeParameterUse(DeclRefExpr *DRE, Expr *parent,
+                                   OperandKind useKind) {
       // This only cares about declarations of noescape function type.
       auto AFT = DRE->getDecl()->getInterfaceType()->getAs<AnyFunctionType>();
       if (!AFT || !AFT->isNoEscape())
@@ -463,10 +576,15 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
       // either as the callee or as an argument (in which case, the typechecker
       // validates that the noescape bit didn't get stripped off), or as
       // a special case, in the binding of a withoutActuallyEscaping block.
-      if (ParentExpr
-          && (isa<ApplyExpr>(ParentExpr) // param()
-              || isa<MakeTemporarilyEscapableExpr>(ParentExpr)))
-        return;
+      if (parent) {
+        if (auto apply = dyn_cast<ApplyExpr>(parent)) {
+          if (isa<ParamDecl>(DRE->getDecl()) && useKind == OperandKind::Callee)
+            checkNoEscapeParameterCall(apply);
+          return;
+        } else if (isa<MakeTemporarilyEscapableExpr>(parent)) {
+          return;
+        }
+      }
 
       TC.diagnose(DRE->getStartLoc(), diag::invalid_noescape_use,
                   cast<VarDecl>(DRE->getDecl())->getName(),

test/SILOptimizer/access_enforcement_noescape.swift

@@ -26,12 +26,6 @@ func doTwo(_: ()->(), _: ()->()) {}
 // Helper
 func doOneInout(_: ()->(), _: inout Int) {}
 
-// FIXME: statically prohibit a call to a non-escaping closure
-// parameter using another non-escaping closure parameter as an argument.
-func reentrantNoescape(fn: (() -> ()) -> ()) {
-  fn { fn {} }
-}
-
 // Error: Cannot capture nonescaping closure.
 // func reentrantCapturedNoescape(fn: (() -> ()) -> ()) {
 //   let c = { fn {} }

test/attr/attr_noescape.swift

@@ -304,6 +304,7 @@ func apply<T, U>(_ f: @noescape (T) -> U, g: @noescape (@noescape (T) -> U) -> U
   // expected-warning@-2{{@noescape is the default and is deprecated}} {{46-56=}}
   // expected-warning@-3{{@noescape is the default and is deprecated}} {{57-66=}}
   return g(f)
+  // expected-warning@-1{{passing a non-escaping function parameter 'f' to a call to a non-escaping function parameter}}
 }
 
 // <rdar://problem/19997577> @noescape cannot be applied to locals, leading to duplication of code

test/expr/postfix/call/noescape-param-exclusivity-swift3.swift

@@ -0,0 +1,5 @@
+// RUN: %target-typecheck-verify-swift -swift-version 3
+
+func foo(fn: (() -> ()) -> ()) {
+  fn { fn {} } // expected-warning {{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 and will be illegal in Swift 4}}
+}

test/expr/postfix/call/noescape-param-exclusivity.swift

@@ -0,0 +1,40 @@
+// RUN: %target-typecheck-verify-swift -swift-version 4
+
+// FIXME: make these errors
+
+func test0(fn: (() -> ()) -> ()) {
+  fn { fn {} } // expected-warning {{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: (() -> ()) -> ()) { // expected-note {{parameter 'fn' is implicitly non-escaping}}
+  // TODO: infer that this function is noescape from its captures
+  func foo() {
+    fn { fn {} } // expected-warning {{can allow re-entrant modification}}
+    // expected-error@-1 {{declaration closing over non-escaping parameter 'fn' may allow it to escape}}
+  }
+}
+
+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-warning {{can allow re-entrant modification}}
+}
+
+func test4(fn: (() -> ()) -> ()) { // expected-note {{parameter 'fn' is implicitly non-escaping}}
+  // TODO: infer that this function is noescape from its captures
+  func foo() {
+    fn {}
+    // expected-error@-1 {{declaration closing over non-escaping parameter 'fn' may allow it to escape}}
+    // FIXME: if the above is ever not an error, we should diagnose at the call below
+  }
+
+  fn(foo)
+}