Merge pull request #18978 from slavapestov/clean-up-conformance-subst

Clean up ProtocolConformance::subst() and fix crash exposed by coroutines work

Author: slavapestov

include/swift/AST/ProtocolConformance.h

@@ -315,13 +315,11 @@ class alignas(1 << DeclAlignInBits) ProtocolConformance {
 
   /// Substitute the conforming type and produce a ProtocolConformance that
   /// applies to the substituted type.
-  ProtocolConformance *subst(Type substType,
-                             SubstitutionMap subMap) const;
+  ProtocolConformance *subst(SubstitutionMap subMap) const;
 
   /// Substitute the conforming type and produce a ProtocolConformance that
   /// applies to the substituted type.
-  ProtocolConformance *subst(Type substType,
-                             TypeSubstitutionFn subs,
+  ProtocolConformance *subst(TypeSubstitutionFn subs,
                              LookupConformanceFn conformances) const;
 
   void dump() const;

include/swift/SIL/TypeSubstCloner.h

@@ -297,15 +297,12 @@ class TypeSubstCloner : public SILClonerWithScopes<ImplClass> {
     if (SubsMap.empty())
       return false;
 
-    auto Params = Sig->getSubstitutableParams();
-    return std::any_of(Params.begin(), Params.end(), [&](Type ParamType) {
-      // FIXME: It would be more elegant to run
-      // SubsMap.mapReplacementTypesOutOfContext() bup front, but it can assert.
-      Type Substitution = Type(ParamType).subst(SubsMap)->mapTypeOutOfContext();
-      return !Substitution->isOpenedExistential() &&
-             (Substitution->getCanonicalType() !=
-              ParamType->getCanonicalType());
-    });
+    for (auto ParamType : Sig->getSubstitutableParams()) {
+      if (!Type(ParamType).subst(SubsMap)->isEqual(ParamType))
+        return true;
+    }
+
+    return false;
   }
 
   enum { ForInlining = true };
@@ -314,7 +311,7 @@ class TypeSubstCloner : public SILClonerWithScopes<ImplClass> {
   /// \param SubsMap - the substitutions of the inlining/specialization process.
   /// \param RemappedSig - the generic signature.
   static SILFunction *remapParentFunction(FunctionBuilderTy &FuncBuilder,
-					  SILModule &M,
+                                          SILModule &M,
                                           SILFunction *ParentFunction,
                                           SubstitutionMap SubsMap,
                                           GenericSignature *RemappedSig,
@@ -325,14 +322,15 @@ class TypeSubstCloner : public SILClonerWithScopes<ImplClass> {
     if (!RemappedSig || !OriginalEnvironment)
       return ParentFunction;
 
-    if (!substitutionsChangeGenericTypeParameters(SubsMap, RemappedSig))
-      return ParentFunction;
-
     if (SubsMap.hasArchetypes())
       SubsMap = SubsMap.mapReplacementTypesOutOfContext();
 
-    // This is a bug in mapReplacementTypesOutOfContext(). Archetypes can't be
-    // mangled, only type parameters can; ignore this for now.
+    if (!substitutionsChangeGenericTypeParameters(SubsMap, RemappedSig))
+      return ParentFunction;
+
+    // Note that mapReplacementTypesOutOfContext() can't do anything for
+    // opened existentials, and since archetypes can't be mangled, ignore
+    // this case for now.
     if (SubsMap.hasArchetypes())
       return ParentFunction;
 

lib/AST/ProtocolConformance.cpp

@@ -98,22 +98,14 @@ ProtocolConformanceRef::subst(Type origType,
   if (isInvalid())
     return *this;
 
-  auto substType = origType.subst(subs, conformances,
-                                  SubstFlags::UseErrorType);
-
   // If we have a concrete conformance, we need to substitute the
   // conformance to apply to the new type.
-  if (isConcrete()) {
-    auto concrete = getConcrete();
-    if (auto classDecl = concrete->getType()->getClassOrBoundGenericClass()) {
-      // If this is a class, we need to traffic in the actual type that
-      // implements the protocol, not 'Self' and not any subclasses (with their
-      // inherited conformances).
-      substType = substType->getSuperclassForDecl(classDecl);
-    }
-    return ProtocolConformanceRef(
-      getConcrete()->subst(substType, subs, conformances));
-  }
+  if (isConcrete())
+    return ProtocolConformanceRef(getConcrete()->subst(subs, conformances));
+
+  // Otherwise, compute the substituted type.
+  auto substType = origType.subst(subs, conformances,
+                                  SubstFlags::UseErrorType);
 
   // Opened existentials trivially conform and do not need to go through
   // substitution map lookup.
@@ -1047,74 +1039,64 @@ bool ProtocolConformance::isVisibleFrom(const DeclContext *dc) const {
 }
 
 ProtocolConformance *
-ProtocolConformance::subst(Type substType,
-                           SubstitutionMap subMap) const {
-  return subst(substType,
-               QuerySubstitutionMap{subMap},
+ProtocolConformance::subst(SubstitutionMap subMap) const {
+  return subst(QuerySubstitutionMap{subMap},
                LookUpConformanceInSubstitutionMap(subMap));
 }
 
 ProtocolConformance *
-ProtocolConformance::subst(Type substType,
-                           TypeSubstitutionFn subs,
+ProtocolConformance::subst(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: {
-    if (substType->isSpecialized()) {
-      assert(getType()->isSpecialized()
-             && "substitution mapped non-specialized to specialized?!");
-      assert(getType()->getNominalOrBoundGenericNominal()
-               == substType->getNominalOrBoundGenericNominal()
-             && "substitution mapped to different nominal?!");
+    auto origType = getType();
+    if (!origType->hasTypeParameter() &&
+        !origType->hasArchetype())
+      return const_cast<ProtocolConformance *>(this);
 
-      auto subMap = SubstitutionMap::get(getGenericSignature(),
-                                         subs, conformances);
+    auto subMap = SubstitutionMap::get(getGenericSignature(),
+                                       subs, conformances);
+    auto substType = origType.subst(subMap, SubstFlags::UseErrorType);
+    if (substType->isEqual(origType))
+      return const_cast<ProtocolConformance *>(this);
 
-      return substType->getASTContext()
+    return substType->getASTContext()
         .getSpecializedConformance(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
       = cast<InheritedProtocolConformance>(this)->getInheritedConformance();
-    ProtocolConformance *newBase;
-    if (inheritedConformance->getType()->isSpecialized()) {
-      // Follow the substituted type up the superclass chain until we reach
-      // the underlying class type.
-      auto targetClass =
-        inheritedConformance->getType()->getClassOrBoundGenericClass();
-      auto superclassType = substType->getSuperclassForDecl(targetClass);
 
+    auto origType = getType();
+    if (!origType->hasTypeParameter() &&
+        !origType->hasArchetype()) {
+      return const_cast<ProtocolConformance *>(this);
+    }
+
+    auto origBaseType = inheritedConformance->getType();
+    if (origBaseType->hasTypeParameter() ||
+        origBaseType->hasArchetype()) {
       // Substitute into the superclass.
-      newBase = inheritedConformance->subst(superclassType, subs, conformances);
-    } else {
-      newBase = inheritedConformance;
+      inheritedConformance = inheritedConformance->subst(subs, conformances);
     }
 
+    auto substType = origType.subst(subs, conformances,
+                                    SubstFlags::UseErrorType);
     return substType->getASTContext()
-      .getInheritedConformance(substType, newBase);
+      .getInheritedConformance(substType, inheritedConformance);
   }
   case ProtocolConformanceKind::Specialized: {
     // Substitute the substitutions in the specialized conformance.
     auto spec = cast<SpecializedProtocolConformance>(this);
     auto genericConformance = spec->getGenericConformance();
     auto subMap = spec->getSubstitutionMap();
 
+    auto origType = getType();
+    auto substType = origType.subst(subs, conformances,
+                                    SubstFlags::UseErrorType);
     return substType->getASTContext()
       .getSpecializedConformance(substType, genericConformance,
                                  subMap.subst(subs, conformances));

lib/AST/SubstitutionMap.cpp

@@ -437,20 +437,44 @@ SubstitutionMap SubstitutionMap::subst(TypeSubstitutionFn subs,
                                        LookupConformanceFn conformances) const {
   if (empty()) return SubstitutionMap();
 
-  return get(
-    getGenericSignature(),
-    [&](SubstitutableType *type) {
-      return Type(type).subst(*this, SubstFlags::UseErrorType)
-               .subst(subs, conformances, SubstFlags::UseErrorType);
-    },
-    [&](CanType dependentType, Type replacementType,
-        ProtocolDecl *proto) ->Optional<ProtocolConformanceRef> {
-      auto conformance =
-        lookupConformance(dependentType, proto)
-          .getValueOr(ProtocolConformanceRef::forInvalid());
-      auto substType = dependentType.subst(*this, SubstFlags::UseErrorType);
-      return conformance.subst(substType, subs, conformances);
-    });
+  SmallVector<Type, 4> newSubs;
+  for (Type type : getReplacementTypes()) {
+    if (!type) {
+      // Non-canonical parameter.
+      newSubs.push_back(Type());
+      continue;
+    }
+    newSubs.push_back(type.subst(subs, conformances, SubstFlags::UseErrorType));
+  }
+
+  SmallVector<ProtocolConformanceRef, 4> newConformances;
+  auto oldConformances = getConformances();
+
+  auto *genericSig = getGenericSignature();
+  for (const auto &req : genericSig->getRequirements()) {
+    if (req.getKind() != RequirementKind::Conformance) continue;
+
+    auto conformance = oldConformances[0];
+
+    // Fast path for concrete case -- we don't need to compute substType
+    // at all.
+    if (conformance.isConcrete()) {
+      newConformances.push_back(
+        ProtocolConformanceRef(
+          conformance.getConcrete()->subst(subs, conformances)));
+    } else {
+      auto origType = req.getFirstType();
+      auto substType = origType.subst(*this, SubstFlags::UseErrorType);
+
+      newConformances.push_back(
+        conformance.subst(substType, subs, conformances));
+    }
+
+    oldConformances = oldConformances.slice(1);
+  }
+
+  assert(oldConformances.empty());
+  return SubstitutionMap(genericSig, newSubs, newConformances);
 }
 
 SubstitutionMap

lib/IRGen/GenProto.cpp

@@ -1112,8 +1112,7 @@ getWitnessTableLazyAccessFunction(IRGenModule &IGM,
 static ProtocolConformance &mapConformanceIntoContext(IRGenModule &IGM,
                                                       const ProtocolConformance &conf,
                                                       DeclContext *dc) {
-  return *conf.subst(dc->mapTypeIntoContext(conf.getType()),
-                     [&](SubstitutableType *t) -> Type {
+  return *conf.subst([&](SubstitutableType *t) -> Type {
                        return dc->mapTypeIntoContext(t);
                      },
                      LookUpConformanceInModule(IGM.getSwiftModule()));

lib/Sema/TypeCheckProtocol.cpp

@@ -3366,11 +3366,7 @@ void ConformanceChecker::ensureRequirementsAreSatisfied(
         auto concreteConformance = conformance.getConcrete();
 
         // Map the conformance.
-        auto interfaceType =
-          concreteConformance->getType()->mapTypeOutOfContext();
-
         concreteConformance = concreteConformance->subst(
-            interfaceType,
             [](SubstitutableType *type) -> Type {
               if (auto *archetypeType = type->getAs<ArchetypeType>())
                 return archetypeType->getInterfaceType();

test/SILGen/Inputs/keypaths_multi_file_b.swift

@@ -9,3 +9,24 @@ struct A {
     set {}
   }
 }
+
+final class C<T> {
+  var a = 0
+  var b = 0
+
+  subscript(x: Int) -> Int { return x }
+}
+
+class D<T> {
+  var a = 0
+  var b = 0
+
+  subscript(x: Int) -> Int { return x }
+}
+
+protocol P {
+  var a: Int { get set }
+  var b: Int { get set }
+
+  subscript(x: Int) -> Int { get set }
+}

test/SILGen/Inputs/keypaths_multi_file_c.swift

@@ -0,0 +1,11 @@
+public protocol PP {}
+public protocol QQ {}
+
+public class AA : PP {}
+public class BB : AA {}
+
+public struct DD<T: PP> {}
+
+public struct CC<T: PP> {
+  public var x: DD<T> { fatalError("death") }
+}

test/SILGen/keypaths_multi_file.swift

@@ -1,5 +1,11 @@
-// RUN: %target-swift-emit-silgen -module-name keypaths -primary-file %s %S/Inputs/keypaths_multi_file_b.swift | %FileCheck %s
-// RUN: %target-swift-emit-silgen -module-name keypaths %s -primary-file %S/Inputs/keypaths_multi_file_b.swift | %FileCheck --check-prefix=DEFINITION %s
+// RUN: %empty-directory(%t)
+
+// RUN: %target-build-swift -module-name keypaths_resilient -emit-module -Xfrontend -enable-key-path-resilience %S/Inputs/keypaths_multi_file_c.swift -emit-module-path %t/keypaths_resilient.swiftmodule
+
+// RUN: %target-swift-emit-silgen -module-name keypaths -primary-file %s %S/Inputs/keypaths_multi_file_b.swift -I %t | %FileCheck %s
+// RUN: %target-swift-emit-silgen -module-name keypaths %s -primary-file %S/Inputs/keypaths_multi_file_b.swift -I %t | %FileCheck --check-prefix=DEFINITION %s
+
+import keypaths_resilient
 
 func foo(x: Int) -> KeyPath<A, Int> {
   switch x {
@@ -18,3 +24,22 @@ func foo(x: Int) -> KeyPath<A, Int> {
 // A.subscript setter
 // CHECK-LABEL: sil hidden_external @$S8keypaths1AVyS2icis
 // DEFINITION-LABEL: sil hidden @$S8keypaths1AVyS2icis
+
+func bar<T>(_: T) {
+  _ = \C<T>.b
+  _ = \C<T>[0]
+
+  _ = \D<T>.b
+  _ = \D<T>[0]
+
+  _ = \P.b
+
+  // FIXME: crashes
+  // _ = \P[0]
+}
+
+// https://bugs.swift.org/browse/SR-8643
+class MM<T: P> : PP {}
+func foo<T3: BB, T4: MM<T3>>(t: T3, u: T4) {
+  let _ = \CC<T4>.x
+}