Merge pull request #18659 from slavapestov/sil-lowering-illegal-tuples

slavapestov · web-flow · commit 59eae1e1b164 · 2018-08-12T13:49:02.000-07:00
SIL: Don't try to lower various illegal tuple types
diff --git a/include/swift/AST/Types.h b/include/swift/AST/Types.h
@@ -361,8 +361,9 @@ class alignas(1 << TypeAlignInBits) TypeBase {
   );
 
   SWIFT_INLINE_BITFIELD_FULL(TupleType, TypeBase, 1+32,
-    /// Whether an element of the tuple is inout.
-    HasInOutElement : 1,
+    /// Whether an element of the tuple is inout, __shared or __owned.
+    /// Values cannot have such tuple types in the language.
+    HasElementWithOwnership : 1,
 
     : NumPadBits,
 
@@ -1876,9 +1877,9 @@ class TupleType final : public TypeBase, public llvm::FoldingSetNode,
   /// return the element index, otherwise return -1.
   int getNamedElementId(Identifier I) const;
   
-  /// Returns true if this tuple has inout elements.
-  bool hasInOutElement() const {
-    return static_cast<bool>(Bits.TupleType.HasInOutElement);
+  /// Returns true if this tuple has inout, __shared or __owned elements.
+  bool hasElementWithOwnership() const {
+    return static_cast<bool>(Bits.TupleType.HasElementWithOwnership);
   }
 
   // Implement isa/cast/dyncast/etc.
@@ -1895,9 +1896,9 @@ class TupleType final : public TypeBase, public llvm::FoldingSetNode,
 private:
   TupleType(ArrayRef<TupleTypeElt> elements, const ASTContext *CanCtx,
             RecursiveTypeProperties properties,
-            bool hasInOut)
+            bool hasElementWithOwnership)
      : TypeBase(TypeKind::Tuple, CanCtx, properties) {
-     Bits.TupleType.HasInOutElement = hasInOut;
+     Bits.TupleType.HasElementWithOwnership = hasElementWithOwnership;
      Bits.TupleType.Count = elements.size();
      std::uninitialized_copy(elements.begin(), elements.end(),
                              getTrailingObjects<TupleTypeElt>());
@@ -4980,13 +4981,13 @@ inline bool TypeBase::isTypeParameter() {
 inline bool TypeBase::isMaterializable() {
   if (hasLValueType())
     return false;
-  
+
   if (is<InOutType>())
     return false;
-  
+
   if (auto *TTy = getAs<TupleType>())
-    return !TTy->hasInOutElement();
-  
+    return !TTy->hasElementWithOwnership();
+
   return true;
 }
 
diff --git a/include/swift/SIL/TypeLowering.h b/include/swift/SIL/TypeLowering.h
@@ -41,6 +41,29 @@ namespace swift {
 
 namespace Lowering {
 
+/// Should this tuple type always be expanded into its elements, even
+/// when emitted against an opaque abstraction pattern?
+///
+/// FIXME: Remove this once function signature lowering always explodes
+/// the top-level argument list.
+inline bool shouldExpandTupleType(TupleType *type) {
+  // Tuples with inout, __shared and __owned elements cannot be lowered
+  // to SIL types.
+  if (type->hasElementWithOwnership())
+    return true;
+
+  // A one-element tuple with a vararg element is essentially
+  // equivalent to the element itself, and we also can't lower it, since
+  // that would strip off the vararg-ness and produce a non-tuple type.
+  if (type->getNumElements() == 1 &&
+      type->getElement(0).isVararg()) {
+    return true;
+  }
+
+  // Everything else is OK.
+  return false;
+}
+
 /// The default convention for handling the callee object on thick
 /// callees.
 const ParameterConvention DefaultThickCalleeConvention =
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -2974,7 +2974,7 @@ Type TupleType::get(ArrayRef<TupleTypeElt> Fields, const ASTContext &C) {
                           Fields[0].getParameterFlags());
 
   RecursiveTypeProperties properties;
-  bool hasInOut = false;
+  bool hasElementWithOwnership = false;
   for (const TupleTypeElt &Elt : Fields) {
     auto eltTy = Elt.getType();
     if (!eltTy) continue;
@@ -2984,11 +2984,13 @@ Type TupleType::get(ArrayRef<TupleTypeElt> Fields, const ASTContext &C) {
     // non-paren tuples are malformed and will be diagnosed later.
     if (auto *TTy = Elt.getType()->getAs<TupleType>()) {
       if (TTy->getNumElements() == 1)
-        hasInOut |= TTy->hasInOutElement();
+        hasElementWithOwnership |= TTy->hasElementWithOwnership();
     } else if (auto *Pty = dyn_cast<ParenType>(Elt.getType().getPointer())) {
-      hasInOut |= Pty->getParameterFlags().isInOut();
+      hasElementWithOwnership |= (Pty->getParameterFlags().getValueOwnership() !=
+                                  ValueOwnership::Default);
     } else {
-      hasInOut |= Elt.getParameterFlags().isInOut();
+      hasElementWithOwnership |= (Elt.getParameterFlags().getValueOwnership() !=
+                                  ValueOwnership::Default);
     }
   }
 
@@ -3016,7 +3018,7 @@ Type TupleType::get(ArrayRef<TupleTypeElt> Fields, const ASTContext &C) {
                          sizeof(TupleTypeElt) * Fields.size(),
                          alignof(TupleType), arena);
   auto New = new (mem) TupleType(Fields, IsCanonical ? &C : nullptr, properties,
-                                 hasInOut);
+                                 hasElementWithOwnership);
   C.getImpl().getArena(arena).TupleTypes.InsertNode(New, InsertPos);
   return New;
 }
diff --git a/lib/SIL/SILFunctionType.cpp b/lib/SIL/SILFunctionType.cpp
@@ -604,9 +604,10 @@ class DestructureInputs {
                                ParameterTypeFlags paramFlags, CanType ty) {
       CanTupleType tty = dyn_cast<TupleType>(ty);
       // If the abstraction pattern is opaque, and the tuple type is
-      // materializable -- if it doesn't contain an l-value type -- then it's
-      // a valid target for substitution and we should not expand it.
-      if (!tty || (pattern.isTypeParameter() && !tty->hasInOutElement())) {
+      // a valid target for substitution, don't expand it.
+      if (!tty ||
+          (pattern.isTypeParameter() &&
+           !shouldExpandTupleType(tty))) {
         visit(paramFlags.getValueOwnership(), /*forSelf=*/false, pattern, ty,
               silRepresentation);
         return;
diff --git a/lib/SIL/TypeLowering.cpp b/lib/SIL/TypeLowering.cpp
@@ -1335,6 +1335,10 @@ void TypeConverter::insert(TypeKey k, const TypeLowering *tl) {
 static CanTupleType getLoweredTupleType(TypeConverter &tc,
                                         AbstractionPattern origType,
                                         CanTupleType substType) {
+  // We can't lower InOutType, and we can't lower an unlabeled one
+  // element vararg tuple either, because lowering strips off flags,
+  // which would end up producing a ParenType.
+  assert(!shouldExpandTupleType(substType));
   assert(origType.matchesTuple(substType));
 
   // Does the lowered tuple type differ from the substituted type in
@@ -1347,27 +1351,32 @@ static CanTupleType getLoweredTupleType(TypeConverter &tc,
     auto origEltType = origType.getTupleElementType(i);
     auto substEltType = substType.getElementType(i);
 
-    assert(!isa<LValueType>(substEltType) &&
-           "lvalue types cannot exist in function signatures");
-    assert(!isa<InOutType>(substEltType) &&
-           "inout cannot appear in tuple element type here");
-    
-    // If the original type was an archetype, use that archetype as
-    // the original type of the element --- the actual archetype
-    // doesn't matter, just the abstraction pattern.
+    auto &substElt = substType->getElement(i);
+
+    // Make sure we don't have something non-materializable.
+    auto Flags = substElt.getParameterFlags();
+    assert(Flags.getValueOwnership() == ValueOwnership::Default);
+
     SILType silType = tc.getLoweredType(origEltType, substEltType);
     CanType loweredSubstEltType = silType.getASTType();
 
-    changed = (changed || substEltType != loweredSubstEltType);
+    changed = (changed || substEltType != loweredSubstEltType ||
+               !Flags.isNone());
 
-    auto &substElt = substType->getElement(i);
-    loweredElts.push_back(substElt.getWithType(loweredSubstEltType));
+    // Note: we drop any parameter flags such as @escaping, @autoclosure and
+    // varargs.
+    //
+    // FIXME: Replace this with an assertion that the original tuple element
+    // did not have any flags.
+    loweredElts.emplace_back(loweredSubstEltType,
+                             substElt.getName(),
+                             ParameterTypeFlags());
   }
   
   if (!changed) return substType;
 
-  // Because we're transforming an existing tuple, the weird corner
-  // case where TupleType::get does not return a TupleType can't happen.
+  // The cast should succeed, because if we end up with a one-element
+  // tuple type here, it must have a label.
   return cast<TupleType>(CanType(TupleType::get(loweredElts, tc.Context)));
 }
 
diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp
@@ -2448,10 +2448,12 @@ class ArgEmitter {
     //
     // FIXME: Once -swift-version 3 code generation goes away, we
     // can simplify this.
-    if (isUnmaterializableTupleType(substArgType)) {
-      assert(origParamType.isTypeParameter());
-      emitExpanded(std::move(arg), origParamType);
-      return;
+    if (auto substTupleType = dyn_cast<TupleType>(substArgType)) {
+      if (shouldExpandTupleType(substTupleType)) {
+        assert(origParamType.isTypeParameter());
+        emitExpanded(std::move(arg), origParamType);
+        return;
+      }
     }
 
     // Okay, everything else will be passed as a single value, one
@@ -2538,13 +2540,6 @@ class ArgEmitter {
     return (dest->isValid() ? dest : nullptr);
   }
 
-  bool isUnmaterializableTupleType(CanType type) {
-    if (auto tuple = dyn_cast<TupleType>(type))
-      if (tuple->hasInOutElement())
-        return true;
-    return false;
-  }
-
   /// Emit an argument as an expanded tuple.
   void emitExpanded(ArgumentSource &&arg, AbstractionPattern origParamType) {
     assert(!arg.isLValue() && "argument is l-value but parameter is tuple?");
diff --git a/lib/SILGen/SILGenPoly.cpp b/lib/SILGen/SILGenPoly.cpp
@@ -872,8 +872,9 @@ namespace {
         }
       }
 
-      // Special-case: tuples containing inouts.
-      if (inputTupleType && inputTupleType->hasInOutElement()) {
+      // Special-case: tuples containing inouts, __shared or __owned,
+      // and one-element vararg tuples.
+      if (inputTupleType && shouldExpandTupleType(inputTupleType)) {
         // Non-materializable tuple types cannot be bound as generic
         // arguments, so none of the remaining transformations apply.
         // Instead, the outermost tuple layer is exploded, even when
@@ -1056,8 +1057,8 @@ namespace {
                                     CanTupleType inputTupleType,
                                     AbstractionPattern outputOrigType,
                                     CanTupleType outputTupleType) {
-      assert(!inputTupleType->hasInOutElement() &&
-             !outputTupleType->hasInOutElement());
+      assert(!inputTupleType->hasElementWithOwnership() &&
+             !outputTupleType->hasElementWithOwnership());
       assert(inputTupleType->getNumElements() ==
              outputTupleType->getNumElements());
 
@@ -1149,8 +1150,8 @@ namespace {
       // when witness method thunks re-abstract a non-mutating
       // witness for a mutating requirement. The inout self is just
       // loaded to produce a value in this case.
-      assert(inputSubstType->hasInOutElement() ||
-             !outputSubstType->hasInOutElement());
+      assert(inputSubstType->hasElementWithOwnership() ||
+             !outputSubstType->hasElementWithOwnership());
       assert(inputSubstType->getNumElements() ==
              outputSubstType->getNumElements());
 
@@ -1171,8 +1172,8 @@ namespace {
                                   ManagedValue inputTupleAddr) {
       assert(inputOrigType.isTypeParameter());
       assert(outputOrigType.matchesTuple(outputSubstType));
-      assert(!inputSubstType->hasInOutElement() &&
-             !outputSubstType->hasInOutElement());
+      assert(!inputSubstType->hasElementWithOwnership() &&
+             !outputSubstType->hasElementWithOwnership());
       assert(inputSubstType->getNumElements() ==
              outputSubstType->getNumElements());
 
@@ -1218,8 +1219,8 @@ namespace {
                                  TemporaryInitialization &tupleInit) {
       assert(inputOrigType.matchesTuple(inputSubstType));
       assert(outputOrigType.matchesTuple(outputSubstType));
-      assert(!inputSubstType->hasInOutElement() &&
-             !outputSubstType->hasInOutElement());
+      assert(!inputSubstType->hasElementWithOwnership() &&
+             !outputSubstType->hasElementWithOwnership());
       assert(inputSubstType->getNumElements() ==
              outputSubstType->getNumElements());
 
diff --git a/test/SILGen/reabstract.swift b/test/SILGen/reabstract.swift
@@ -103,3 +103,13 @@ closureTakingOptional({ (_: Any) -> () in })
 // CHECK:   [[OPTADDR:%.*]] = init_existential_addr [[ANYADDR]] : $*Any, $Optional<Int>
 // CHECK:   store %0 to [trivial] [[OPTADDR]] : $*Optional<Int>
 // CHECK:   apply %1([[ANYADDR]]) : $@noescape @callee_guaranteed (@in_guaranteed Any) -> ()
+
+// Same behavior as above with other ownership qualifiers.
+func evenLessFun(_ s: __shared C, _ o: __owned C) {}
+
+// CHECK-LABEL: sil shared [transparent] [serializable] [reabstraction_thunk] @$S10reabstract1CCACIeggx_A2CytIegnir_TR : $@convention(thin) (@in_guaranteed C, @in C, @guaranteed @callee_guaranteed (@guaranteed C, @owned C) -> ()) -> @out ()
+// CHECK-LABEL: sil shared [transparent] [serializable] [reabstraction_thunk] @$S10reabstract1CCACytIegnir_A2CIeggx_TR : $@convention(thin) (@guaranteed C, @owned C, @guaranteed @callee_guaranteed (@in_guaranteed C, @in C) -> @out ()) -> ()
+func testSharedOwnedOpaque(_ s: C, o: C) {
+  let box = Box(t: evenLessFun)
+  box.t(s, o)
+}
diff --git a/test/SILGen/tuple_attribute_reabstraction.swift b/test/SILGen/tuple_attribute_reabstraction.swift
@@ -0,0 +1,24 @@
+// RUN: %target-swift-emit-silgen -enable-sil-ownership %s | %FileCheck %s
+
+public struct G<T> {
+  var t: T
+
+  public init(t: T) { self.t = t }
+}
+
+public func takesAutoclosureAndEscaping(_: @autoclosure () -> (), _: @escaping () -> ()) {}
+public func takesVarargs(_: Int...) {}
+
+public func f() {
+  _ = G(t: takesAutoclosureAndEscaping)
+  _ = G(t: takesVarargs)
+}
+
+// We shouldn't have @autoclosure and @escaping attributes in the lowered tuple type:
+
+// CHECK-LABEL: sil shared [transparent] [serializable] [reabstraction_thunk] @$SIg_Ieg_Iegyg_ytytIgnr__ytytIegnr_tytIegnr_TR : $@convention(thin) (@in_guaranteed (@noescape @callee_guaranteed (@in_guaranteed ()) -> @out (), @callee_guaranteed (@in_guaranteed ()) -> @out ()), @guaranteed @callee_guaranteed (@noescape @callee_guaranteed () -> (), @guaranteed @callee_guaranteed () -> ()) -> ()) -> @out ()
+
+// The one-element vararg tuple ([Int]...) should be exploded and not treated as opaque,
+// even though its materializable:
+
+// CHECK-LABEL: sil shared [transparent] [serializable] [reabstraction_thunk] @$SSaySiGIegg_AAytIegnr_TR : $@convention(thin) (@in_guaranteed Array<Int>, @guaranteed @callee_guaranteed (@guaranteed Array<Int>) -> ()) -> @out ()
