Merge pull request swiftlang#39193 from hamishknight/less-toil-more-tuple

hamishknight · web-flow · commit ff92d93a88a6 · 2021-09-10T21:31:11.000+01:00
[AST] Enforce that TupleExprs and ParenExprs dont have param flags
diff --git a/include/swift/AST/Types.h b/include/swift/AST/Types.h
@@ -3005,12 +3005,11 @@ class AnyFunctionType : public TypeBase {
 
 public:
   /// Take an array of parameters and turn it into a tuple or paren type.
+  ///
+  /// \param wantParamFlags Whether to preserve the parameter flags from the
+  /// given set of parameters.
   static Type composeTuple(ASTContext &ctx, ArrayRef<Param> params,
-                           bool canonicalVararg);
-  static Type composeTuple(ASTContext &ctx, CanParamArrayRef params,
-                           bool canonicalVararg) {
-    return composeTuple(ctx, params.getOriginalArray(), canonicalVararg);
-  }
+                           bool wantParamFlags = true);
 
   /// Given two arrays of parameters determine if they are equal in their
   /// canonicalized form. Internal labels and type sugar is *not* taken into
diff --git a/lib/APIDigester/ModuleAnalyzerNodes.cpp b/lib/APIDigester/ModuleAnalyzerNodes.cpp
@@ -1519,8 +1519,7 @@ SwiftDeclCollector::constructTypeNode(Type T, TypeInitInfo Info) {
     Root->addChild(constructTypeNode(Fun->getResult()));
 
     auto Input = AnyFunctionType::composeTuple(Fun->getASTContext(),
-                                               Fun->getParams(),
-                                               /*canonicalVararg=*/false);
+                                               Fun->getParams());
     Root->addChild(constructTypeNode(Input));
     return Root;
   }
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -3457,12 +3457,12 @@ Type AnyFunctionType::Param::getParameterType(bool forCanonical,
 }
 
 Type AnyFunctionType::composeTuple(ASTContext &ctx, ArrayRef<Param> params,
-                                   bool canonicalVararg) {
+                                   bool wantParamFlags) {
   SmallVector<TupleTypeElt, 4> elements;
   for (const auto &param : params) {
-    Type eltType = param.getParameterType(canonicalVararg, &ctx);
-    elements.emplace_back(eltType, param.getLabel(),
-                          param.getParameterFlags());
+    auto flags = wantParamFlags ? param.getParameterFlags()
+                                : ParameterTypeFlags();
+    elements.emplace_back(param.getParameterType(), param.getLabel(), flags);
   }
   return TupleType::get(elements, ctx);
 }
diff --git a/lib/AST/ASTVerifier.cpp b/lib/AST/ASTVerifier.cpp
@@ -1136,6 +1136,7 @@ class Verifier : public ASTWalker {
     }
 
     void verifyChecked(TupleExpr *E) {
+      PrettyStackTraceExpr debugStack(Ctx, "verifying TupleExpr", E);
       const TupleType *exprTy = E->getType()->castTo<TupleType>();
       for_each(exprTy->getElements().begin(), exprTy->getElements().end(),
                E->getElements().begin(),
@@ -1148,8 +1149,14 @@ class Verifier : public ASTWalker {
           Out << elt->getType() << "\n";
           abort();
         }
+        if (!field.getParameterFlags().isNone()) {
+          Out << "TupleExpr has non-empty parameter flags?\n";
+          Out << "sub expr: \n";
+          elt->dump(Out);
+          Out << "\n";
+          abort();
+        }
       });
-      // FIXME: Check all the variadic elements.
       verifyCheckedBase(E);
     }
 
@@ -1970,10 +1977,15 @@ class Verifier : public ASTWalker {
 
     void verifyChecked(ParenExpr *E) {
       PrettyStackTraceExpr debugStack(Ctx, "verifying ParenExpr", E);
-      if (!isa<ParenType>(E->getType().getPointer())) {
+      auto ty = dyn_cast<ParenType>(E->getType().getPointer());
+      if (!ty) {
         Out << "ParenExpr not of ParenType\n";
         abort();
       }
+      if (!ty->getParameterFlags().isNone()) {
+        Out << "ParenExpr has non-empty parameter flags?\n";
+        abort();
+      }
       verifyCheckedBase(E);
     }
 
diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp
@@ -4023,8 +4023,7 @@ RValue CallEmission::applyEnumElementConstructor(SGFContext C) {
                                 std::move(*callSite).forward());
 
     auto payloadTy = AnyFunctionType::composeTuple(SGF.getASTContext(),
-                                                  resultFnType.getParams(),
-                                                  /*canonicalVararg*/ true);
+                                                   resultFnType->getParams());
     auto arg = RValue(SGF, argVals, payloadTy->getCanonicalType());
     payload = ArgumentSource(uncurriedLoc, std::move(arg));
     formalResultType = cast<FunctionType>(formalResultType).getResult();
diff --git a/lib/SILGen/SILGenExpr.cpp b/lib/SILGen/SILGenExpr.cpp
@@ -2668,8 +2668,7 @@ loadIndexValuesForKeyPathComponent(SILGenFunction &SGF, SILLocation loc,
 
   auto indexLoweredTy =
     SGF.getLoweredType(
-      AnyFunctionType::composeTuple(SGF.getASTContext(), indexParams,
-                                    /*canonicalVararg=*/false));
+      AnyFunctionType::composeTuple(SGF.getASTContext(), indexParams));
 
   auto addr = SGF.B.createPointerToAddress(loc, pointer,
                                            indexLoweredTy.getAddressType(),
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -2002,8 +2002,7 @@ static bool fixMissingArguments(ConstraintSystem &cs, ASTNode anchor,
   // synthesized arguments to it.
   if (argumentTuple) {
     cs.addConstraint(ConstraintKind::Bind, *argumentTuple,
-                     FunctionType::composeTuple(ctx, args,
-                                                /*canonicalVararg=*/false),
+                     FunctionType::composeTuple(ctx, args),
                      cs.getConstraintLocator(anchor));
   }
 
@@ -2206,21 +2205,39 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
   // take multiple arguments to be passed as an argument in places
   // that expect a function that takes a single tuple (of the same
   // arity);
-  auto canImplodeParams = [&](ArrayRef<AnyFunctionType::Param> params) {
+  auto canImplodeParams = [&](ArrayRef<AnyFunctionType::Param> params,
+                              const FunctionType *destFn) {
     if (params.size() == 1)
       return false;
 
-    for (auto param : params)
-      if (param.isVariadic() || param.isInOut() || param.isAutoClosure())
-        return false;
+    // We do not support imploding into a @differentiable function.
+    if (destFn->isDifferentiable())
+      return false;
 
+    for (auto &param : params) {
+      // We generally cannot handle parameter flags, though we can carve out an
+      // exception for ownership flags such as __owned, which we can thunk, and
+      // flags that can freely dropped from a function type such as
+      // @_nonEphemeral. Note that @noDerivative can also be freely dropped, as
+      // we've already ensured that the destination function is not
+      // @differentiable.
+      auto flags = param.getParameterFlags();
+      flags = flags.withValueOwnership(
+          param.isInOut() ? ValueOwnership::InOut : ValueOwnership::Default);
+      flags = flags.withNonEphemeral(false)
+                   .withNoDerivative(false);
+      if (!flags.isNone())
+        return false;
+    }
     return true;
   };
 
   auto implodeParams = [&](SmallVectorImpl<AnyFunctionType::Param> &params) {
+    // Form an imploded tuple type, dropping the parameter flags as although
+    // canImplodeParams makes sure we're not dealing with vargs, inout, etc,
+    // we may still have e.g ownership flags left over, which we can drop.
     auto input = AnyFunctionType::composeTuple(getASTContext(), params,
-                                               /*canonicalVararg=*/false);
-
+                                               /*wantParamFlags*/ false);
     params.clear();
     // If fixes are disabled let's do an easy thing and implode
     // tuple directly into parameters list.
@@ -2255,12 +2272,12 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
     if (last != path.rend()) {
       if (last->getKind() == ConstraintLocator::ApplyArgToParam) {
         if (isSingleTupleParam(ctx, func2Params) &&
-            canImplodeParams(func1Params)) {
+            canImplodeParams(func1Params, /*destFn*/ func2)) {
           implodeParams(func1Params);
           increaseScore(SK_FunctionConversion);
         } else if (!ctx.isSwiftVersionAtLeast(5) &&
                    isSingleTupleParam(ctx, func1Params) &&
-                   canImplodeParams(func2Params)) {
+                   canImplodeParams(func2Params,  /*destFn*/ func1)) {
           auto *simplified = locator.trySimplifyToExpr();
           // We somehow let tuple unsplatting function conversions
           // through in some cases in Swift 4, so let's let that
@@ -2296,11 +2313,11 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
         // 2. `case .bar(let tuple) = e` allows to match multiple
         //    parameters with a single tuple argument.
         if (isSingleTupleParam(ctx, func1Params) &&
-            canImplodeParams(func2Params)) {
+            canImplodeParams(func2Params, /*destFn*/ func1)) {
           implodeParams(func2Params);
           increaseScore(SK_FunctionConversion);
         } else if (isSingleTupleParam(ctx, func2Params) &&
-                   canImplodeParams(func1Params)) {
+                   canImplodeParams(func1Params, /*destFn*/ func2)) {
           implodeParams(func1Params);
           increaseScore(SK_FunctionConversion);
         }
@@ -2311,7 +2328,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
       auto *anchor = locator.trySimplifyToExpr();
       if (isa_and_nonnull<ClosureExpr>(anchor) &&
           isSingleTupleParam(ctx, func2Params) &&
-          canImplodeParams(func1Params)) {
+          canImplodeParams(func1Params, /*destFn*/ func2)) {
         auto *fix = AllowClosureParamDestructuring::create(
             *this, func2, getConstraintLocator(anchor));
         if (recordFix(fix))
@@ -6073,8 +6090,7 @@ ConstraintSystem::simplifyConstructionConstraint(
 
     // Tuple construction is simply tuple conversion.
     Type argType = AnyFunctionType::composeTuple(getASTContext(),
-                                                 fnType->getParams(),
-                                                 /*canonicalVararg=*/false);
+                                                 fnType->getParams());
     Type resultType = fnType->getResult();
 
     ConstraintLocatorBuilder builder(locator);
@@ -7006,8 +7022,7 @@ ConstraintSystem::simplifyFunctionComponentConstraint(
 
     if (kind == ConstraintKind::FunctionInput) {
       type = AnyFunctionType::composeTuple(getASTContext(),
-                                           funcTy->getParams(),
-                                           /*canonicalVararg=*/false);
+                                           funcTy->getParams());
       locKind = ConstraintLocator::FunctionArgument;
     } else if (kind == ConstraintKind::FunctionResult) {
       type = funcTy->getResult();
diff --git a/lib/Sema/TypeCheckDeclOverride.cpp b/lib/Sema/TypeCheckDeclOverride.cpp
@@ -482,7 +482,7 @@ static bool noteFixableMismatchedTypes(ValueDecl *decl, const ValueDecl *base) {
     auto *fnType = baseTy->getAs<AnyFunctionType>();
     baseTy = fnType->getResult();
     Type argTy = FunctionType::composeTuple(
-        ctx, baseTy->getAs<AnyFunctionType>()->getParams(), false);
+        ctx, baseTy->getAs<AnyFunctionType>()->getParams());
     auto diagKind = diag::override_type_mismatch_with_fixits_init;
     unsigned numArgs = baseInit->getParameters()->size();
     return computeFixitsForOverridenDeclaration(
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -2164,8 +2164,7 @@ static Type getTypeForDisplay(ModuleDecl *module, ValueDecl *decl) {
     return AnyFunctionType::composeTuple(module->getASTContext(),
                                          ctor->getMethodInterfaceType()
                                              ->castTo<FunctionType>()
-                                             ->getParams(),
-                                         /*canonicalVararg=*/false);
+                                             ->getParams());
   }
 
   Type type = decl->getInterfaceType();
diff --git a/test/AutoDiff/Sema/differentiable_attr_type_checking.swift b/test/AutoDiff/Sema/differentiable_attr_type_checking.swift
@@ -726,3 +726,24 @@ struct Accessors: Differentiable {
 // expected-error @+1 {{cannot differentiate functions returning opaque result types}}
 @differentiable(reverse)
 func opaqueResult(_ x: Float) -> some Differentiable { x }
+
+// Test the function tupling conversion with @differentiable.
+func tuplify<Ts, U>(_ fn: @escaping (Ts) -> U) -> (Ts) -> U { fn }
+func tuplifyDifferentiable<Ts : Differentiable, U>(_ fn: @escaping @differentiable(reverse) (Ts) -> U) -> @differentiable(reverse) (Ts) -> U { fn }
+
+func testTupling(withoutNoDerivative: @escaping @differentiable(reverse) (Float, Float) -> Float,
+                 withNoDerivative: @escaping @differentiable(reverse) (Float, @noDerivative Float) -> Float) {
+  // We support tupling of differentiable functions as long as they drop @differentiable.
+  let _: ((Float, Float)) -> Float = tuplify(withoutNoDerivative)
+  let fn1 = tuplify(withoutNoDerivative)
+  _ = fn1((0, 0))
+
+  // In this case we also drop @noDerivative.
+  let _: ((Float, Float)) -> Float = tuplify(withNoDerivative)
+  let fn2 = tuplify(withNoDerivative)
+  _ = fn2((0, 0))
+
+  // We do not support tupling into an @differentiable function.
+  let _ = tuplifyDifferentiable(withoutNoDerivative) // expected-error {{cannot convert value of type '@differentiable(reverse) (Float, Float) -> Float' to expected argument type '@differentiable(reverse) (Float) -> Float'}}
+  let _ = tuplifyDifferentiable(withNoDerivative) // expected-error {{cannot convert value of type '@differentiable(reverse) (Float, @noDerivative Float) -> Float' to expected argument type '@differentiable(reverse) (Float) -> Float'}}
+}
diff --git a/test/Concurrency/isolated_parameters.swift b/test/Concurrency/isolated_parameters.swift
@@ -102,3 +102,13 @@ actor TestActor {
 
 func redecl(_: TestActor) { } // expected-note{{'redecl' previously declared here}}
 func redecl(_: isolated TestActor) { } // expected-error{{invalid redeclaration of 'redecl'}}
+
+func tuplify<Ts>(_ fn: (Ts) -> Void) {} // expected-note {{in call to function 'tuplify'}}
+
+@available(SwiftStdlib 5.5, *)
+func testTuplingIsolated(_ a: isolated A, _ b: isolated A) {
+  // FIXME: We shouldn't duplicate the "cannot convert value of type" diagnostic (SR-15179)
+  tuplify(testTuplingIsolated)
+  // expected-error@-1 {{generic parameter 'Ts' could not be inferred}}
+  // expected-error@-2 2{{cannot convert value of type '(isolated A, isolated A) -> ()' to expected argument type '(Ts) -> Void'}}
+}
diff --git a/test/Constraints/function.swift b/test/Constraints/function.swift
@@ -250,3 +250,16 @@ func test_passing_noescape_function_ref_to_generic_parameter() {
 func SR14784<T>(_ fs: () -> T..., a _ : Int) -> T {
   fs.first! // expected-error{{function produces expected type 'T'; did you mean to call it with '()'?}} {{11-11=()}}
 }
+
+func tuplify<Ts>(_ fn: (Ts) -> Void) {}
+
+func testInvalidTupleImplosions() {
+  func takesVargs(_ x: Int, _ y: String...) {}
+  tuplify(takesVargs) // expected-error {{cannot convert value of type '(Int, String...) -> ()' to expected argument type '(Int) -> Void'}}
+
+  func takesAutoclosure(_ x: @autoclosure () -> Int, y: String) {}
+  tuplify(takesAutoclosure) // expected-error {{cannot convert value of type '(@autoclosure () -> Int, String) -> ()' to expected argument type '(@escaping () -> Int) -> Void'}}
+
+  func takesInout(_ x: Int, _ y: inout String) {}
+  tuplify(takesInout) // expected-error {{cannot convert value of type '(Int, inout String) -> ()' to expected argument type '(Int) -> Void'}}
+}
diff --git a/test/Sema/diag_non_ephemeral.swift b/test/Sema/diag_non_ephemeral.swift
@@ -525,3 +525,16 @@ func ambiguous_fn(_ ptr: UnsafeRawPointer) {} // expected-note {{found this cand
 func test_ambiguity_with_function_instead_of_argument(_ x: inout Int) {
   ambiguous_fn(&x) // expected-error {{ambiguous use of 'ambiguous_fn'}}
 }
+
+func tuplify<Ts>(_ fn: @escaping (Ts) -> Void) -> (Ts) -> Void { fn }
+
+func testTuplingNonEphemeral(_ ptr: UnsafePointer<Int>) {
+  // Make sure we drop @_nonEphemeral when imploding params. This is to ensure
+  // we don't accidently break any potentially valid code.
+  let fn = tuplify(takesTwoPointers)
+  fn((ptr, ptr))
+
+  // Note we can't perform X-to-pointer conversions in this case even if we
+  // wanted to.
+  fn(([1], ptr)) // expected-error {{tuple type '([Int], UnsafePointer<Int>)' is not convertible to tuple type '(UnsafePointer<Int>, UnsafePointer<Int>)'}}
+}

Original file line number	Diff line number	Diff line change
`@@ -1519,8 +1519,7 @@ SwiftDeclCollector::constructTypeNode(Type T, TypeInitInfo Info) {`
`1519`	`1519`	`Root->addChild(constructTypeNode(Fun->getResult()));`
`1520`	`1520`
`1521`	`1521`	`auto Input = AnyFunctionType::composeTuple(Fun->getASTContext(),`
`1522`		`- Fun->getParams(),`
`1523`		`- /canonicalVararg=/false);`
	`1522`	`+ Fun->getParams());`
`1524`	`1523`	`Root->addChild(constructTypeNode(Input));`
`1525`	`1524`	`return Root;`
`1526`	`1525`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3457,12 +3457,12 @@ Type AnyFunctionType::Param::getParameterType(bool forCanonical,`
`3457`	`3457`	`}`
`3458`	`3458`
`3459`	`3459`	`Type AnyFunctionType::composeTuple(ASTContext &ctx, ArrayRef<Param> params,`
`3460`		`- bool canonicalVararg) {`
	`3460`	`+ bool wantParamFlags) {`
`3461`	`3461`	`SmallVector<TupleTypeElt, 4> elements;`
`3462`	`3462`	`for (const auto &param : params) {`
`3463`		`- Type eltType = param.getParameterType(canonicalVararg, &ctx);`
`3464`		`- elements.emplace_back(eltType, param.getLabel(),`
`3465`		`- param.getParameterFlags());`
	`3463`	`+ auto flags = wantParamFlags ? param.getParameterFlags()`
	`3464`	`+ : ParameterTypeFlags();`
	`3465`	`+ elements.emplace_back(param.getParameterType(), param.getLabel(), flags);`
`3466`	`3466`	`}`
`3467`	`3467`	`return TupleType::get(elements, ctx);`
`3468`	`3468`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2164,8 +2164,7 @@ static Type getTypeForDisplay(ModuleDecl module, ValueDecl decl) {`
`2164`	`2164`	`return AnyFunctionType::composeTuple(module->getASTContext(),`
`2165`	`2165`	`ctor->getMethodInterfaceType()`
`2166`	`2166`	`->castTo<FunctionType>()`
`2167`		`- ->getParams(),`
`2168`		`- /canonicalVararg=/false);`
	`2167`	`+ ->getParams());`
`2169`	`2168`	`}`
`2170`	`2169`
`2171`	`2170`	`Type type = decl->getInterfaceType();`