Merge pull request #8937 from slavapestov/check-yourself-before-you-wreck-yourself

Fix a couple of dynamic Self bugs and a bonus protocol self-conformance crasher

Author: slavapestov

lib/AST/ProtocolConformance.cpp

@@ -103,15 +103,23 @@ ProtocolConformanceRef::subst(Type origType,
   auto substType = origType.subst(subs, conformances,
                                   SubstFlags::UseErrorType);
 
-  if (substType->isOpenedExistential())
-    return *this;
-
   // If we have a concrete conformance, we need to substitute the
   // conformance to apply to the new type.
   if (isConcrete())
     return ProtocolConformanceRef(
       getConcrete()->subst(substType, subs, conformances));
 
+  // Opened existentials trivially conform and do not need to go through
+  // substitution map lookup.
+  if (substType->isOpenedExistential())
+    return *this;
+
+  // If the substituted type is an existential, we have a self-conforming
+  // existential being substituted in place of itself. There's no
+  // conformance information in this case, so just return.
+  if (substType->isObjCExistentialType())
+    return *this;
+
   auto *proto = getRequirement();
 
   // If the original type was an archetype, check the conformance map.
@@ -154,9 +162,7 @@ ProtocolConformanceRef::subst(Type origType,
     return ProtocolConformanceRef(lookupResults.front());
   }
 
-  // FIXME: Rip this out once ConformanceAccessPaths are plumbed through
-  auto *M = proto->getParentModule();
-  return *M->lookupConformance(substType, proto, nullptr);
+  llvm_unreachable("Invalid conformance substitution");
 }
 
 Type
@@ -788,11 +794,16 @@ ProtocolConformance *
 ProtocolConformance::subst(Type substType,
                            TypeSubstitutionFn subs,
                            LookupConformanceFn conformances) const {
+  // ModuleDecl::lookupConformance() strips off dynamic Self, so
+  // we should do the same here.
+  if (auto selfType = substType->getAs<DynamicSelfType>())
+    substType = selfType->getSelfType();
+
   if (getType()->isEqual(substType))
     return const_cast<ProtocolConformance *>(this);
 
   switch (getKind()) {
-  case ProtocolConformanceKind::Normal:
+  case ProtocolConformanceKind::Normal: {
     if (substType->isSpecialized()) {
       assert(getType()->isSpecialized()
              && "substitution mapped non-specialized to specialized?!");
@@ -831,10 +842,11 @@ ProtocolConformance::subst(Type substType,
                                    const_cast<ProtocolConformance *>(this),
                                    subMap);
     }
+
     assert(substType->isEqual(getType())
            && "substitution changed non-specialized type?!");
     return const_cast<ProtocolConformance *>(this);
-      
+  }
   case ProtocolConformanceKind::Inherited: {
     // Substitute the base.
     auto inheritedConformance

lib/AST/SubstitutionMap.cpp

@@ -124,9 +124,8 @@ SubstitutionMap::lookupConformance(CanType type, ProtocolDecl *proto) const {
   if (!genericSig->conformsToProtocol(type, proto, mod))
     return None;
 
-  auto canonType = genericSig->getCanonicalTypeInContext(type, mod);
   auto accessPath =
-    genericSig->getConformanceAccessPath(canonType, proto, mod);
+    genericSig->getConformanceAccessPath(type, proto, mod);
 
   // Fall through because we cannot yet evaluate an access path.
   Optional<ProtocolConformanceRef> conformance;
@@ -143,8 +142,21 @@ SubstitutionMap::lookupConformance(CanType type, ProtocolDecl *proto) const {
     // If we've hit an abstract conformance, everything from here on out is
     // abstract.
     // FIXME: This may not always be true, but it holds for now.
-    if (conformance->isAbstract())
+    if (conformance->isAbstract()) {
+      // FIXME: Rip this out once we can get a concrete conformance from
+      // an archetype.
+      auto *M = proto->getParentModule();
+      auto substType = type.subst(*this);
+      if (substType &&
+          !substType->is<ArchetypeType>() &&
+          !substType->isTypeParameter() &&
+          !substType->isExistentialType()) {
+        auto lazyResolver = M->getASTContext().getLazyResolver();
+        return *M->lookupConformance(substType, proto, lazyResolver);
+      }
+
       return ProtocolConformanceRef(proto);
+    }
 
     // For the second step, we're looking into the requirement signature for
     // this protocol.
@@ -153,7 +165,7 @@ SubstitutionMap::lookupConformance(CanType type, ProtocolDecl *proto) const {
 
     // If we haven't set the signature conformances yet, force the issue now.
     if (normal->getSignatureConformances().empty()) {
-      auto lazyResolver = canonType->getASTContext().getLazyResolver();
+      auto lazyResolver = type->getASTContext().getLazyResolver();
       lazyResolver->resolveTypeWitness(normal, nullptr);
 
       // Error case: the conformance is broken, so we cannot handle this

lib/AST/Type.cpp

@@ -2946,17 +2946,7 @@ Optional<ProtocolConformanceRef>
 LookUpConformanceInSubstitutionMap::operator()(CanType dependentType,
                                        Type conformingReplacementType,
                                        ProtocolType *conformedProtocol) const {
-  auto result = Subs.lookupConformance(dependentType, conformedProtocol->getDecl());
-  if ((result && result->isConcrete()) ||
-      conformingReplacementType->hasError() ||
-      conformingReplacementType->isTypeParameter())
-    return result;
-
-  // FIXME: Rip this out once ConformanceAccessPaths are plumbed through
-  auto *M = conformedProtocol->getDecl()->getParentModule();
-  return M->lookupConformance(conformingReplacementType,
-                              conformedProtocol->getDecl(),
-                              nullptr);
+  return Subs.lookupConformance(dependentType, conformedProtocol->getDecl());
 }
 
 Optional<ProtocolConformanceRef>

lib/SILOptimizer/IPO/ClosureSpecializer.cpp

@@ -771,6 +771,15 @@ void SILClosureSpecializerTransform::gatherCallSites(
             !ApplyCallee->hasValidLinkageForFragileInline())
           continue;
 
+        // If the callee uses a dynamic Self, we cannot specialize it,
+        // since the resulting specialization might longer has 'self' as the
+        // last parameter.
+        //
+        // We could fix this by inserting new arguments more carefully, or
+        // changing how we model dynamic Self altogether.
+        if (mayBindDynamicSelf(ApplyCallee))
+          return;
+
         // Ok, we know that we can perform the optimization but not whether or
         // not the optimization is profitable. Find the index of the argument
         // corresponding to our partial apply.

stdlib/public/core/KeyPath.swift

@@ -114,13 +114,11 @@ public class AnyKeyPath: Hashable, _AppendKeyPath {
     _abstract()
   }
 
-  // FIXME: This should return Self, but that breaks the closure specializer.
-  // rdar://problem/31725007
   public // @testable
   static func _create(
     capacityInBytes bytes: Int,
     initializedBy body: (UnsafeMutableRawBufferPointer) -> Void
-  ) -> AnyKeyPath {
+  ) -> Self {
     _sanityCheck(bytes > 0 && bytes % 4 == 0,
                  "capacity must be multiple of 4 bytes")
     let result = Builtin.allocWithTailElems_1(self, (bytes/4)._builtinWordValue,

test/SILOptimizer/closure_specialize_dynamic_self.swift

@@ -0,0 +1,23 @@
+// RUN: %target-swift-frontend -emit-sil -O -primary-file %s
+
+// Just make sure we skip the optimization and not crash here.
+//
+// Eventually, we can make this work.
+//
+// <rdar://problem/31725007>
+
+class Foo {
+  required init() {}
+
+  static func foo(_ f: () -> ()) -> Self {
+    f()
+    return self.init()
+  }
+}
+
+class Bar: Foo {}
+
+func closures(_ x: String) {
+  print(Foo.foo { _ = x })
+  print(Bar.foo { _ = x })
+}

test/SILOptimizer/specialize_dynamic_self.swift

@@ -0,0 +1,29 @@
+// RUN: %target-swift-frontend -emit-sil -O -primary-file %s | %FileCheck %s
+
+protocol P {}
+
+extension P {
+  @_semantics("optimize.sil.never") func method1() {}
+
+  @inline(__always) func method2() { method1() }
+}
+
+class C<T> : P {
+  // CHECK-LABEL: sil shared [always_inline] @_T023specialize_dynamic_self1CC11returnsSelfACyxGXDyFSi_Tg5 : $@convention(method) (@guaranteed C<Int>) -> @owned C<Int>
+  // CHECK: [[RESULT:%.*]] = alloc_stack $C<Int>
+  // CHECK: [[FN:%.*]] = function_ref @_T023specialize_dynamic_self1PPAAE7method1yyF : $@convention(method) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> ()
+  // CHECK: apply [[FN]]<@dynamic_self C<Int>>([[RESULT]]) : $@convention(method) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> ()
+  // CHECK: return %0 : $C<Int>
+  @inline(__always)
+  final func returnsSelf() -> Self {
+    method2()
+    return self
+  }
+}
+
+// CHECK-LABEL: sil hidden [thunk] [always_inline] @_T023specialize_dynamic_self8usesCIntyAA1CCySiG1c_tF : $@convention(thin) (@owned C<Int>) -> () {
+// CHECK: function_ref @_T023specialize_dynamic_self1CC11returnsSelfACyxGXDyFSi_Tg5 : $@convention(method) (@guaranteed C<Int>) -> @owned C<Int>
+// CHECK: return
+func usesCInt(c: C<Int>) {
+  _ = c.returnsSelf()
+}

test/SILOptimizer/specialize_self_conforming.swift

@@ -0,0 +1,24 @@
+// RUN: %target-swift-frontend -emit-sil -O -primary-file %s | %FileCheck %s
+
+// REQUIRES: objc_interop
+
+import Foundation
+
+@objc protocol P {}
+
+@_semantics("optimize.sil.never")
+func takesP<T : P>(_: T) {}
+
+@inline(__always)
+func callsTakesP<T : P>(_ t: T) {
+  takesP(t)
+}
+
+// CHECK-LABEL: sil hidden @_T026specialize_self_conforming16callsTakesPWithPyAA1P_pF : $@convention(thin) (@owned P) -> () {
+// CHECK: [[FN:%.*]] = function_ref @_T026specialize_self_conforming6takesPyxAA1PRzlF : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@owned τ_0_0) -> ()
+// CHECK: apply [[FN]]<P>(%0) : $@convention(thin) <τ_0_0 where τ_0_0 : P> (@owned τ_0_0) -> ()
+// CHECK: return
+
+func callsTakesPWithP(_ p: P) {
+  callsTakesP(p)
+}

validation-test/compiler_crashers_2/0091-rdar30168645.swift

@@ -0,0 +1,5 @@
+// RUN: not --crash %target-swift-frontend -emit-ir -primary-file %s
+
+class Base<T: AnyObject> {}
+
+class Derived: Base<Derived> {}

validation-test/execution/crashers/rdar18157434.swift

@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: %target-build-swift %s -o %t/a.out
+// RUN: not --crash %target-run %t/a.out
+
+// REQUIRES: executable_test
+// REQUIRES: OS=macosx
+
+// "cyclic metadata dependency detected, aborting"
+
+struct X<T> {
+  enum S {
+  case some(T), none
+  }
+  
+  init() { a = .none }
+  var a: S
+}
+
+X<()>()
+