Sema: Simplify representation of vararg forwarding

VarargExpansionExpr shows up in call argument lists in synthesized
initializers and modify accessors when we need to forward arguments
to a call taking varargs.

Previously we would say that the type of VarargExpansionExpr is
$T when its subexpression type is [$T]. matchCallArguments() would
then 'collect' the single VarargExpansionExpr into a variadic
argument list with a single element, and build an ArgumentShuffleExpr
for the argument list.

In turn, SILGen would peephole vararg emission of a variadic
argument list with a single entry that happens to be a
VarargExpansionExpr, by returning the subexpression's value,
which happened to be an array of the right element type,
instead of building a new array containing the elements of the
variadic argument list.

This was all too complicated. Instead, let's say that the type of
a VarargExpansionExpr is [$T], except that when it appears in a
TupleExpr, the variadic bit of the corresponding element is set.

Then, matchCallArguments() needs to support a case where both
the parameter and argument list have a matching vararg element.
In this case, instead of collecting multiple arguments into a
single variadic argument list, we treat the variadic argument like
an ordinary parameter, bypassing construction of the
ArgumentShuffleExpr altogether.

Finally, SILGen now needs to be able to emit a VarargExpansionExpr
in ordinary rvalue position, since it now appears as a child of a
TupleExpr; it can do this by simply emitting the sub-expression
to produce an array value.

Author: slavapestov

lib/SILGen/ASTVisitor.h

@@ -57,7 +57,8 @@ class ASTVisitor : public swift::ASTVisitor<ImplClass,
   }
 
   ExprRetTy visitVarargExpansionExpr(VarargExpansionExpr *E, Args... AA) {
-    llvm_unreachable("vararg expansion should not appear in this position");
+    return static_cast<ImplClass*>(this)->visit(E->getSubExpr(),
+                                                std::forward<Args>(AA)...);
   }
 
   ExprRetTy visitIdentityExpr(IdentityExpr *E, Args...AA) {

lib/SILGen/SILGenApply.cpp

@@ -2599,27 +2599,25 @@ namespace {
 /// arguments are going into a varargs array.
 struct ArgSpecialDest {
   VarargsInfo *SharedInfo;
-  unsigned Index : 31;
-  unsigned IsExpansion : 1;
+  unsigned Index : 32;
   CleanupHandle Cleanup;
 
   ArgSpecialDest() : SharedInfo(nullptr) {}
-  explicit ArgSpecialDest(VarargsInfo &info, unsigned index, bool isExpansion)
-    : SharedInfo(&info), Index(index), IsExpansion(isExpansion) {}
+  explicit ArgSpecialDest(VarargsInfo &info, unsigned index)
+    : SharedInfo(&info), Index(index) {}
 
   // Reference semantics: need to preserve the cleanup handle.
   ArgSpecialDest(const ArgSpecialDest &) = delete;
   ArgSpecialDest &operator=(const ArgSpecialDest &) = delete;
   ArgSpecialDest(ArgSpecialDest &&other)
     : SharedInfo(other.SharedInfo), Index(other.Index),
-      IsExpansion(other.IsExpansion), Cleanup(other.Cleanup) {
+      Cleanup(other.Cleanup) {
     other.SharedInfo = nullptr;
   }
   ArgSpecialDest &operator=(ArgSpecialDest &&other) {
     assert(!isValid() && "overwriting valid special destination!");
     SharedInfo = other.SharedInfo;
     Index = other.Index;
-    IsExpansion = other.IsExpansion;
     Cleanup = other.Cleanup;
     other.SharedInfo = nullptr;
     return *this;
@@ -2638,14 +2636,6 @@ struct ArgSpecialDest {
             SILType loweredSubstParamType) {
     assert(isValid() && "filling an invalid destination");
 
-    if (IsExpansion) {
-      auto expr = std::move(arg).asKnownExpr()->getSemanticsProvidingExpr();
-      auto array = cast<VarargExpansionExpr>(expr)->getSubExpr();
-      SharedInfo->setExpansion(Index, SGF.emitRValueAsSingleValue(array));
-      Cleanup = CleanupHandle::invalid();
-      return;
-    }
-
     SILLocation loc = arg.getLocation();
     auto destAddr = SharedInfo->getBaseAddress();
     if (Index != 0) {
@@ -3473,9 +3463,8 @@ struct ElementExtent {
   ClaimedParamsRef Params;
   /// The destination index, if any.
   /// This is set in the first pass.
-  unsigned DestIndex : 29;
+  unsigned DestIndex : 30;
   unsigned HasDestIndex : 1;
-  unsigned IsVarargExpansion : 1;
 #ifndef NDEBUG
   unsigned Used : 1;
 #endif
@@ -3520,7 +3509,6 @@ class ArgumentShuffleEmitter {
   /// Extents of the inner elements.
   SmallVector<ElementExtent, 8> innerExtents;
   Optional<VarargsInfo> varargsInfo;
-  SmallVector<unsigned, 4> varargExpansions;
   SILParameterInfo variadicParamInfo; // innerExtents will point at this
   Optional<SmallVector<ArgSpecialDest, 8>> innerSpecialDests;
 
@@ -3575,18 +3563,6 @@ class ArgumentShuffleEmitter {
     assert(!isResultScalar || index == 0);
     return origParamType.getTupleElementType(index);
   }
-
-  VarargExpansionExpr *getVarargExpansion(unsigned innerIndex) {
-    Expr *expr = inner->getSemanticsProvidingExpr();
-    if (cast<ArgumentShuffleExpr>(outer)->isSourceScalar()) {
-      assert(innerIndex == 0);
-    } else {
-      auto tuple = dyn_cast<TupleExpr>(expr);
-      if (!tuple) return nullptr;
-      expr = tuple->getElement(innerIndex)->getSemanticsProvidingExpr();
-    }
-    return dyn_cast<VarargExpansionExpr>(expr);
-  }
 };
 
 } // end anonymous namespace
@@ -3635,22 +3611,10 @@ void ArgumentShuffleEmitter::constructInnerTupleTypeInfo(ArgEmitter &parent) {
       unsigned numVarargs = variadicArgs.size();
       assert(canVarargsArrayType == substEltType);
 
-      // Check for vararg expansions, since their presence changes our
-      // emission strategy.
-      {
-        for (auto i : indices(variadicArgs)) {
-          unsigned innerIndex = variadicArgs[i];
-          if (getVarargExpansion(innerIndex)) {
-            varargExpansions.push_back(i);
-          }
-        }
-      }
-
       // If we don't have any vararg expansions, eagerly emit into
       // the array value.
       varargsInfo.emplace(emitBeginVarargs(parent.SGF, outer, varargsEltType,
-                                           canVarargsArrayType, numVarargs,
-                                           varargExpansions));
+                                           canVarargsArrayType, numVarargs));
 
       // If we have any varargs, we'll need to actually initialize
       // the array buffer.
@@ -3679,12 +3643,9 @@ void ArgumentShuffleEmitter::constructInnerTupleTypeInfo(ArgEmitter &parent) {
           innerExtents[innerIndex].Used = true;
 #endif
 
-          auto expansion = getVarargExpansion(innerIndex);
-
           // Set the destination index.
           innerExtents[innerIndex].HasDestIndex = true;
           innerExtents[innerIndex].DestIndex = i++;
-          innerExtents[innerIndex].IsVarargExpansion = (expansion != nullptr);
 
           // Use the singleton param info we prepared before.
           innerExtents[innerIndex].Params =
@@ -3714,8 +3675,7 @@ void ArgumentShuffleEmitter::flattenPatternFromInnerExtendIntoInnerParams(
       if (extent.HasDestIndex) {
         assert(extent.Params.size() == 1);
         innerSpecialDests->push_back(
-            ArgSpecialDest(*varargsInfo, extent.DestIndex,
-                           extent.IsVarargExpansion));
+            ArgSpecialDest(*varargsInfo, extent.DestIndex));
 
         // Otherwise, fill in with the appropriate number of invalid
         // special dests.

lib/SILGen/SILGenExpr.cpp

@@ -1916,20 +1916,11 @@ RValue RValueEmitter::visitCoerceExpr(CoerceExpr *E, SGFContext C) {
 
 VarargsInfo Lowering::emitBeginVarargs(SILGenFunction &SGF, SILLocation loc,
                                        CanType baseTy, CanType arrayTy,
-                                       unsigned numElements,
-                                       ArrayRef<unsigned> expansionIndices) {
+                                       unsigned numElements) {
   // Reabstract the base type against the array element type.
   auto baseAbstraction = AbstractionPattern::getOpaque();
   auto &baseTL = SGF.getTypeLowering(baseAbstraction, baseTy);
 
-  if (!expansionIndices.empty()) {
-    // An assertion is okay here for now because this is only in generated code.
-    assert(numElements == 1 &&
-           "expansion that is not the only variadic argument is unsupported");
-    return VarargsInfo(ManagedValue(), CleanupHandle::invalid(), SILValue(),
-                       baseTL, baseAbstraction, /*expansion peephole*/ true);
-  }
-
   // Allocate the array.
   SILValue numEltsVal = SGF.B.createIntegerLiteral(loc,
                              SILType::getBuiltinWordType(SGF.getASTContext()),
@@ -1955,18 +1946,11 @@ VarargsInfo Lowering::emitBeginVarargs(SILGenFunction &SGF, SILLocation loc,
     /*isStrict*/ true,
     /*isInvariant*/ false);
 
-  return VarargsInfo(array, abortCleanup, basePtr, baseTL, baseAbstraction,
-                     /*expansion peephole*/ false);
+  return VarargsInfo(array, abortCleanup, basePtr, baseTL, baseAbstraction);
 }
 
 ManagedValue Lowering::emitEndVarargs(SILGenFunction &SGF, SILLocation loc,
                                       VarargsInfo &&varargs) {
-  if (varargs.isExpansionPeephole()) {
-    auto result = varargs.getArray();
-    assert(result);
-    return result;
-  }
-
   // Kill the abort cleanup.
   SGF.Cleanups.setCleanupState(varargs.getAbortCleanup(), CleanupState::Dead);
 
@@ -3610,7 +3594,7 @@ RValue RValueEmitter::visitArrayExpr(ArrayExpr *E, SGFContext C) {
   CanType arrayTy = ArraySliceType::get(elementType)->getCanonicalType();
   VarargsInfo varargsInfo =
       emitBeginVarargs(SGF, loc, elementType, arrayTy,
-                       E->getNumElements(), {});
+                       E->getNumElements());
 
   // Cleanups for any elements that have been initialized so far.
   SmallVector<CleanupHandle, 8> cleanups;

lib/SILGen/Varargs.h

@@ -35,23 +35,12 @@ class VarargsInfo {
   SILValue BaseAddress;
   AbstractionPattern BasePattern;
   const TypeLowering &BaseTL;
-  bool IsExpansionPeephole = false;
 public:
   VarargsInfo(ManagedValue array, CleanupHandle abortCleanup,
               SILValue baseAddress, const TypeLowering &baseTL,
-              AbstractionPattern basePattern, bool isExpansionPeephole)
+              AbstractionPattern basePattern)
     : Array(array), AbortCleanup(abortCleanup),
-      BaseAddress(baseAddress), BasePattern(basePattern), BaseTL(baseTL),
-      IsExpansionPeephole(isExpansionPeephole) {}
-
-  void setExpansion(unsigned index, ManagedValue expansion) {
-    assert(IsExpansionPeephole);
-    assert(index == 0 && "non-initial index for peephole?");
-    assert(!Array && "array already filled");
-    Array = expansion;
-  }
-
-  bool isExpansionPeephole() const { return IsExpansionPeephole; }
+      BaseAddress(baseAddress), BasePattern(basePattern), BaseTL(baseTL) {}
 
   /// Return the array value.  emitEndVarargs() is really the only
   /// function that should be accessing this directly.
@@ -75,8 +64,7 @@ class VarargsInfo {
 /// Begin a varargs emission sequence.
 VarargsInfo emitBeginVarargs(SILGenFunction &SGF, SILLocation loc,
                              CanType baseTy, CanType arrayTy,
-                             unsigned numElements,
-                             ArrayRef<unsigned> expansions);
+                             unsigned numElements);
 
 /// Successfully end a varargs emission sequence.
 ManagedValue emitEndVarargs(SILGenFunction &SGF, SILLocation loc,

lib/Sema/CSApply.cpp

@@ -2945,11 +2945,7 @@ namespace {
     Expr *visitVarargExpansionExpr(VarargExpansionExpr *expr) {
       simplifyExprType(expr);
 
-      auto elementTy = cs.getType(expr);
-      auto arrayTy =
-        cs.getTypeChecker().getArraySliceType(expr->getLoc(), elementTy);
-      if (!arrayTy) return expr;
-
+      auto arrayTy = cs.getType(expr);
       expr->setSubExpr(coerceToType(expr->getSubExpr(), arrayTy,
                                     cs.getConstraintLocator(expr)));
       return expr;

lib/Sema/CSGen.cpp

@@ -1718,7 +1718,9 @@ namespace {
       for (unsigned i = 0, n = expr->getNumElements(); i != n; ++i) {
         auto *elt = expr->getElement(i);
         auto ty = CS.getType(elt);
-        auto flags = ParameterTypeFlags().withInOut(elt->isSemanticallyInOutExpr());
+        auto flags = ParameterTypeFlags()
+            .withInOut(elt->isSemanticallyInOutExpr())
+            .withVariadic(isa<VarargExpansionExpr>(elt));
         elements.push_back(TupleTypeElt(ty->getInOutObjectType(),
                                         expr->getElementName(i), flags));
       }
@@ -2438,12 +2440,7 @@ namespace {
       CS.addConstraint(ConstraintKind::Conversion,
                        CS.getType(expr->getSubExpr()), array,
                        CS.getConstraintLocator(expr));
-
-      // The apparent type of the expression is the element type, as far as
-      // the type-checker is concerned.  When this becomes a real feature,
-      // we should syntactically restrict these expressions to only appear
-      // in specific positions.
-      return element;
+      return array;
     }
 
     Type visitDynamicTypeExpr(DynamicTypeExpr *expr) {

lib/Sema/CSSimplify.cpp

@@ -390,6 +390,13 @@ matchCallArguments(ArrayRef<AnyFunctionType::Param> args,
       // Record the first argument for the variadic.
       parameterBindings[paramIdx].push_back(*claimed);
 
+      // If the argument is itself variadic, we're forwarding varargs
+      // with a VarargExpansionExpr; don't collect any more arguments.
+      if (args[*claimed].isVariadic()) {
+        skipClaimedArgs();
+        return;
+      }
+
       auto currentNextArgIdx = nextArgIdx;
       {
         nextArgIdx = *claimed;

lib/Sema/CodeSynthesis.cpp

@@ -457,7 +457,8 @@ static Expr *buildArgumentForwardingExpr(ArrayRef<ParamDecl*> params,
   }
 
   // A single unlabeled value is not a tuple.
-  if (args.size() == 1 && labels[0].empty()) {
+  if (args.size() == 1 && labels[0].empty() &&
+      !isa<VarargExpansionExpr>(args[0])) {
     return new (ctx) ParenExpr(SourceLoc(), args[0], SourceLoc(),
                                /*hasTrailingClosure=*/false);
   }