Merge pull request swiftlang#21360 from slavapestov/fix-tuple-splat-varargs

slavapestov · web-flow · commit fd0a82bd337b · 2018-12-16T13:08:59.000-05:00
SE-0110 tuple splatting should not implode argument list with varargs
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -431,16 +431,12 @@ namespace {
     /// \param sources The sources of each of the elements to be used in the
     /// resulting tuple, as provided by \c computeTupleShuffle.
     ///
-    /// \param variadicArgs The source indices that are mapped to the variadic
-    /// parameter of the resulting tuple, as provided by \c computeTupleShuffle.
-    ///
     /// \param typeFromPattern Optionally, the caller can specify the pattern
     /// from where the toType is derived, so that we can deliver better fixit.
     Expr *coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
                              TupleType *toTuple,
                              ConstraintLocatorBuilder locator,
-                             SmallVectorImpl<int> &sources,
-                             SmallVectorImpl<unsigned> &variadicArgs,
+                             SmallVectorImpl<unsigned> &sources,
                              Optional<Pattern*> typeFromPattern = None);
 
     /// Coerce a subclass, class-constrained archetype, class-constrained
@@ -2815,11 +2811,7 @@ namespace {
     }
 
     Expr *visitParenExpr(ParenExpr *expr) {
-      auto &ctx = cs.getASTContext();
-      auto pty = cs.getType(expr->getSubExpr());
-      cs.setType(expr, ParenType::get(ctx, pty->getInOutObjectType(),
-                                      ParameterTypeFlags().withInOut(pty->is<InOutType>())));
-      return expr;
+      return simplifyExprType(expr);
     }
 
     Expr *visitTupleExpr(TupleExpr *expr) {
@@ -5061,8 +5053,7 @@ static Type rebuildIdentityExprs(ConstraintSystem &cs, Expr *expr, Type type) {
 Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
                                        TupleType *toTuple,
                                        ConstraintLocatorBuilder locator,
-                                       SmallVectorImpl<int> &sources,
-                                       SmallVectorImpl<unsigned> &variadicArgs,
+                                       SmallVectorImpl<unsigned> &sources,
                                        Optional<Pattern*> typeFromPattern){
   auto &tc = cs.getTypeChecker();
 
@@ -5077,14 +5068,11 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
                                  fromTuple->getElements().size());
 
   for (unsigned i = 0, n = toTuple->getNumElements(); i != n; ++i) {
-    assert(sources[i] != TupleShuffleExpr::DefaultInitialize &&
-           sources[i] != TupleShuffleExpr::Variadic);
-
     const auto &toElt = toTuple->getElement(i);
     auto toEltType = toElt.getType();
 
     // If the source and destination index are different, we'll be shuffling.
-    if ((unsigned)sources[i] != i) {
+    if (sources[i] != i) {
       anythingShuffled = true;
     }
 
@@ -5161,11 +5149,22 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
 
     return expr;
   }
-  
+
+  // computeTupleShuffle() produces an array of unsigned (since it can only
+  // contain tuple element indices). However TupleShuffleExpr is also used
+  // for call argument lists which can contain special things like default
+  // arguments and variadics; those are presented by negative integers.
+  //
+  // FIXME: Design and implement a new AST where argument lists are
+  // separate from rvalue tuple conversions.
+  ArrayRef<unsigned> sourcesRef = sources;
+  ArrayRef<int> sourcesCast((const int *) sourcesRef.data(),
+                            sourcesRef.size());
+
   // Create the tuple shuffle.
   return
     cs.cacheType(TupleShuffleExpr::create(tc.Context,
-                                          expr, sources,
+                                          expr, sourcesCast,
                                           TupleShuffleExpr::TupleToTuple,
                                           ConcreteDeclRef(), {}, Type(), {},
                                           toSugarType));
@@ -5306,15 +5305,12 @@ Expr *ExprRewriter::coerceExistential(Expr *expr, Type toType,
   if (auto tupleType = fromType->getAs<TupleType>()) {
     if (tupleType->hasLValueType()) {
       auto toTuple = tupleType->getRValueType()->castTo<TupleType>();
-      SmallVector<int, 4> sources;
-      SmallVector<unsigned, 4> variadicArgs;
-      bool failed = computeTupleShuffle(tupleType, toTuple,
-                                        sources, variadicArgs);
+      SmallVector<unsigned, 4> sources;
+      bool failed = computeTupleShuffle(tupleType, toTuple, sources);
       assert(!failed && "Couldn't convert tuple to tuple?");
       (void)failed;
 
-      coerceTupleToTuple(expr, tupleType, toTuple, locator, sources,
-                         variadicArgs);
+      coerceTupleToTuple(expr, tupleType, toTuple, locator, sources);
     }
   }
 
@@ -6543,11 +6539,10 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
   if (auto toTuple = toType->getAs<TupleType>()) {
     // Coerce from a tuple to a tuple.
     if (auto fromTuple = fromType->getAs<TupleType>()) {
-      SmallVector<int, 4> sources;
-      SmallVector<unsigned, 4> variadicArgs;
-      if (!computeTupleShuffle(fromTuple, toTuple, sources, variadicArgs)) {
+      SmallVector<unsigned, 4> sources;
+      if (!computeTupleShuffle(fromTuple, toTuple, sources)) {
         return coerceTupleToTuple(expr, fromTuple, toTuple,
-                                  locator, sources, variadicArgs);
+                                  locator, sources);
       }
     }
   }
diff --git a/lib/Sema/CSDiag.cpp b/lib/Sema/CSDiag.cpp
@@ -1528,13 +1528,12 @@ bool FailureDiagnosis::diagnoseGeneralConversionFailure(Constraint *constraint){
         FromElts.push_back({ voidTy, fromTT->getElement(i).getName() });
       auto TEType = TupleType::get(FromElts, CS.getASTContext());
 
-      SmallVector<int, 4> sources;
-      SmallVector<unsigned, 4> variadicArgs;
+      SmallVector<unsigned, 4> sources;
       
       // If the shuffle conversion is invalid (e.g. incorrect element labels),
       // then we have a type error.
       if (computeTupleShuffle(TEType->castTo<TupleType>()->getElements(),
-                              toTT->getElements(), sources, variadicArgs)) {
+                              toTT->getElements(), sources)) {
         diagnose(anchor->getLoc(), diag::tuple_types_not_convertible,
                  fromTT, toTT)
         .highlight(anchor->getSourceRange());
@@ -7610,27 +7609,18 @@ bool FailureDiagnosis::visitTupleExpr(TupleExpr *TE) {
   if (!TEType->is<TupleType>())
     return visitExpr(TE);
 
-  SmallVector<int, 4> sources;
-  SmallVector<unsigned, 4> variadicArgs;
+  SmallVector<unsigned, 4> sources;
   
   // If the shuffle is invalid, then there is a type error.  We could diagnose
   // it specifically here, but the general logic does a fine job so we let it
   // do it.
   if (computeTupleShuffle(TEType->castTo<TupleType>()->getElements(),
-                          contextualTT->getElements(), sources, variadicArgs))
+                          contextualTT->getElements(), sources))
     return visitExpr(TE);
 
   // If we got a correct shuffle, we can perform the analysis of all of
   // the input elements, with their expected types.
   for (unsigned i = 0, e = sources.size(); i != e; ++i) {
-    // If the value is taken from a default argument, ignore it.
-    if (sources[i] == TupleShuffleExpr::DefaultInitialize ||
-        sources[i] == TupleShuffleExpr::Variadic ||
-        sources[i] == TupleShuffleExpr::CallerDefaultInitialize)
-      continue;
-    
-    assert(sources[i] >= 0 && "Unknown sources index");
-    
     // Otherwise, it must match the corresponding expected argument type.
     unsigned inArgNo = sources[i];
 
@@ -7659,27 +7649,6 @@ bool FailureDiagnosis::visitTupleExpr(TupleExpr *TE) {
     }
   }
   
-  if (!variadicArgs.empty()) {
-    Type varargsTy;
-    for (auto &elt : contextualTT->getElements()) {
-      if (elt.isVararg()) {
-        varargsTy = elt.getVarargBaseTy();
-        break;
-      }
-    }
-
-    assert(varargsTy);
-
-    for (unsigned i = 0, e = variadicArgs.size(); i != e; ++i) {
-      unsigned inArgNo = variadicArgs[i];
-
-      auto expr = typeCheckChildIndependently(
-          TE->getElement(inArgNo), varargsTy, CS.getContextualTypePurpose());
-      // If there was an error type checking this argument, then we're done.
-      if (!expr) return true;
-    }
-  }
-  
   return false;
 }
 
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -997,29 +997,12 @@ ConstraintSystem::matchTupleTypes(TupleType *tuple1, TupleType *tuple2,
   }
 
   // Compute the element shuffles for conversions.
-  SmallVector<int, 16> sources;
-  SmallVector<unsigned, 4> variadicArguments;
-  if (computeTupleShuffle(tuple1, tuple2, sources, variadicArguments))
+  SmallVector<unsigned, 16> sources;
+  if (computeTupleShuffle(tuple1, tuple2, sources))
     return getTypeMatchFailure(locator);
 
   // Check each of the elements.
-  bool hasVariadic = false;
-  unsigned variadicIdx = sources.size();
   for (unsigned idx2 = 0, n = sources.size(); idx2 != n; ++idx2) {
-    // Default-initialization always allowed for conversions.
-    if (sources[idx2] == TupleShuffleExpr::DefaultInitialize) {
-      continue;
-    }
-
-    // Variadic arguments handled below.
-    if (sources[idx2] == TupleShuffleExpr::Variadic) {
-      assert(!hasVariadic && "Multiple variadic parameters");
-      hasVariadic = true;
-      variadicIdx = idx2;
-      continue;
-    }
-
-    assert(sources[idx2] >= 0);
     unsigned idx1 = sources[idx2];
 
     // Match up the types.
@@ -1032,21 +1015,6 @@ ConstraintSystem::matchTupleTypes(TupleType *tuple1, TupleType *tuple2,
       return result;
   }
 
-  // If we have variadic arguments to check, do so now.
-  if (hasVariadic) {
-    const auto &elt2 = tuple2->getElements()[variadicIdx];
-    auto eltType2 = elt2.getVarargBaseTy();
-
-    for (unsigned idx1 : variadicArguments) {
-      auto result = matchTypes(tuple1->getElementType(idx1), eltType2, subKind,
-                         subflags,
-                         locator.withPathElement(
-                                        LocatorPathElt::getTupleElement(idx1)));
-      if (result.isFailure())
-        return result;
-    }
-  }
-
   return getTypeMatchSuccess();
 }
 
@@ -1173,6 +1141,17 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
             !params[0].isVariadic());
   };
 
+  auto canImplodeParams = [&](ArrayRef<AnyFunctionType::Param> params) {
+    if (params.size() == 1)
+      return false;
+
+    for (auto param : params)
+      if (param.isVariadic() || param.isInOut())
+        return false;
+
+    return true;
+  };
+
   auto implodeParams = [&](SmallVectorImpl<AnyFunctionType::Param> &params) {
     auto input = AnyFunctionType::composeInput(getASTContext(), params,
                                                /*canonicalVararg=*/false);
@@ -1193,21 +1172,18 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
 
     if (last != path.rend()) {
       if (last->getKind() == ConstraintLocator::ApplyArgToParam) {
-        if (isSingleParam(func2Params)) {
-          if (!isSingleParam(func1Params)) {
-            implodeParams(func1Params);
-          }
-        } else if (getASTContext().isSwiftVersionAtLeast(4)
-                   && !getASTContext().isSwiftVersionAtLeast(5)
-                   && !isSingleParam(func2Params)) {
+        if (isSingleParam(func2Params) &&
+            canImplodeParams(func1Params)) {
+          implodeParams(func1Params);
+        } else if (!getASTContext().isSwiftVersionAtLeast(5) &&
+                   isSingleParam(func1Params) &&
+                   canImplodeParams(func2Params)) {
           auto *simplified = locator.trySimplifyToExpr();
           // We somehow let tuple unsplatting function conversions
           // through in some cases in Swift 4, so let's let that
           // continue to work, but only for Swift 4.
           if (simplified && isa<DeclRefExpr>(simplified)) {
-            if (isSingleParam(func1Params)) {
-              implodeParams(func2Params);
-            }
+            implodeParams(func2Params);
           }
         }
       }
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
@@ -3309,25 +3309,17 @@ class ConstraintSystem {
 /// \param sources Will be populated with information about the source of each
 /// of the elements for the result tuple. The indices into this array are the
 /// indices of the tuple type we're converting to, while the values are
-/// either one of the \c TupleShuffleExpr constants or are an index into the
-/// source tuple.
-///
-/// \param variadicArgs Will be populated with all of the variadic arguments
-/// that will be placed into the variadic tuple element (i.e., at the index
-/// \c where \c consumed[i] is \c TupleShuffleExpr::Variadic). The values
-/// are indices into the source tuple.
+/// an index into the source tuple.
 ///
 /// \returns true if no tuple conversion is possible, false otherwise.
 bool computeTupleShuffle(ArrayRef<TupleTypeElt> fromTuple,
                          ArrayRef<TupleTypeElt> toTuple,
-                         SmallVectorImpl<int> &sources,
-                         SmallVectorImpl<unsigned> &variadicArgs);
+                         SmallVectorImpl<unsigned> &sources);
 static inline bool computeTupleShuffle(TupleType *fromTuple,
                                        TupleType *toTuple,
-                                       SmallVectorImpl<int> &sources,
-                                       SmallVectorImpl<unsigned> &variadicArgs){
+                                       SmallVectorImpl<unsigned> &sources){
   return computeTupleShuffle(fromTuple->getElements(), toTuple->getElements(),
-                             sources, variadicArgs);
+                             sources);
 }
 
 /// Describes the arguments to which a parameter binds.
diff --git a/lib/Sema/TypeCheckConstraints.cpp b/lib/Sema/TypeCheckConstraints.cpp
@@ -100,13 +100,11 @@ void *operator new(size_t bytes, ConstraintSystem& cs,
 
 bool constraints::computeTupleShuffle(ArrayRef<TupleTypeElt> fromTuple,
                                       ArrayRef<TupleTypeElt> toTuple,
-                                      SmallVectorImpl<int> &sources,
-                                      SmallVectorImpl<unsigned> &variadicArgs) {
-  const int unassigned = -3;
+                                      SmallVectorImpl<unsigned> &sources) {
+  const unsigned unassigned = -1;
   
   SmallVector<bool, 4> consumed(fromTuple.size(), false);
   sources.clear();
-  variadicArgs.clear();
   sources.assign(toTuple.size(), unassigned);
 
   // Match up any named elements.
@@ -150,35 +148,14 @@ bool constraints::computeTupleShuffle(ArrayRef<TupleTypeElt> fromTuple,
     if (sources[i] != unassigned)
       continue;
 
-    const auto &elt2 = toTuple[i];
-
-    // Variadic tuple elements match the rest of the input elements.
-    if (elt2.isVararg()) {
-      // Collect the remaining (unnamed) inputs.
-      while (fromNext != fromLast) {
-        // Labeled elements can't be adopted into varargs even if
-        // they're non-mandatory.  There isn't a really strong reason
-        // for this, though.
-        if (fromTuple[fromNext].hasName())
-          return true;
-
-        variadicArgs.push_back(fromNext);
-        consumed[fromNext] = true;
-        skipToNextAvailableInput();
-      }
-      sources[i] = TupleShuffleExpr::Variadic;
-      
-      // Keep looking at subsequent arguments.  Non-variadic arguments may
-      // follow the variadic one.
-      continue;
-    }
-
     // If there aren't any more inputs, we are done.
     if (fromNext == fromLast) {
       return true;
     }
 
     // Otherwise, assign this input to the next output element.
+    const auto &elt2 = toTuple[i];
+    assert(!elt2.isVararg());
 
     // Fail if the input element is named and we're trying to match it with
     // something with a different label.
diff --git a/test/Compatibility/tuple_arguments_4.swift b/test/Compatibility/tuple_arguments_4.swift
diff --git a/test/Constraints/tuple_arguments.swift b/test/Constraints/tuple_arguments.swift
