Rationalize Implicit Member Synthesis Somewhat

Codable's deep magic currently forces conformance checks in the middle
of name lookup in order to inject CodingKeys into lookup results.  This
is compounded by the fact that this lookup fixup is occuring
incrementally, meaning depending on order of requirements being looked
up, Decl::getMembers() will give you a different answer.

Compounding this, NameLookup relied on the LazyResolver to formalize
this layering violation, and relied on implicit laziness to guard
against re-entrancy.

The approach is multi-pronged:
1) Shift the layering violation into the request evaluator
2) Spell out the kinds of resolution we support explicitly (make them
easier to find and kill)
3) Remove the LazyResolver entrypoint this was relying on
4) Split off the property wrappers part into its own utility

Author: CodaFi

include/swift/AST/ASTTypeIDZone.def

@@ -18,6 +18,7 @@
 SWIFT_TYPEID(AncestryFlags)
 SWIFT_TYPEID(CtorInitializerKind)
 SWIFT_TYPEID(GenericSignature)
+SWIFT_TYPEID(ImplicitMemberAction)
 SWIFT_TYPEID(ParamSpecifier)
 SWIFT_TYPEID(PropertyWrapperBackingPropertyInfo)
 SWIFT_TYPEID(PropertyWrapperTypeInfo)

include/swift/AST/ASTTypeIDs.h

@@ -55,6 +55,7 @@ class TypeAliasDecl;
 class Type;
 struct TypePair;
 enum class AncestryFlags : uint8_t;
+enum class ImplicitMemberAction : uint8_t;
 
 // Define the AST type zone (zone 1)
 #define SWIFT_TYPEID_ZONE AST

include/swift/AST/Decl.h

@@ -477,13 +477,16 @@ class alignas(1 << DeclAlignInBits) Decl {
     IsDebuggerAlias : 1
   );
 
-  SWIFT_INLINE_BITFIELD(NominalTypeDecl, GenericTypeDecl, 1+1,
+  SWIFT_INLINE_BITFIELD(NominalTypeDecl, GenericTypeDecl, 1+1+1,
     /// Whether we have already added implicitly-defined initializers
     /// to this declaration.
     AddedImplicitInitializers : 1,
 
     /// Whether there is are lazily-loaded conformances for this nominal type.
-    HasLazyConformances : 1
+    HasLazyConformances : 1,
+
+    /// Whether this nominal type is having its semantic members resolved.
+    IsComputingSemanticMembers : 1
   );
 
   SWIFT_INLINE_BITFIELD_FULL(ProtocolDecl, NominalTypeDecl, 1+1+1+1+1+1+1+2+1+1+8+16,
@@ -3328,6 +3331,7 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
     Bits.NominalTypeDecl.AddedImplicitInitializers = false;
     ExtensionGeneration = 0;
     Bits.NominalTypeDecl.HasLazyConformances = false;
+    Bits.NominalTypeDecl.IsComputingSemanticMembers = false;
   }
 
   friend class ProtocolType;
@@ -3475,6 +3479,8 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
   /// declaration, or \c nullptr if it doesn't have one.
   ConstructorDecl *getDefaultInitializer() const;
 
+  void synthesizeSemanticMembersIfNeeded(DeclName member);
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) {
     return D->getKind() >= DeclKind::First_NominalTypeDecl &&

include/swift/AST/LazyResolver.h

@@ -52,9 +52,6 @@ class LazyResolver {
   /// the given protocol conformance.
   virtual void resolveWitness(const NormalProtocolConformance *conformance,
                               ValueDecl *requirement) = 0;
-
-  /// Resolve an implicitly-generated member with the given name.
-  virtual void resolveImplicitMember(NominalTypeDecl *nominal, DeclName member) = 0;
 };
 
 class LazyMemberLoader;

include/swift/AST/TypeCheckRequests.h

@@ -1668,6 +1668,35 @@ class InheritsSuperclassInitializersRequest
   void cacheResult(bool value) const;
 };
 
+// The actions this request takes are all huge layering violations.
+//
+// Please do not add any more.
+enum class ImplicitMemberAction : uint8_t {
+  ResolveImplicitInit,
+  ResolveCodingKeys,
+  ResolveEncodable,
+  ResolveDecodable,
+};
+
+class ResolveImplicitMemberRequest
+    : public SimpleRequest<ResolveImplicitMemberRequest,
+                           bool(NominalTypeDecl *, ImplicitMemberAction),
+                           CacheKind::Uncached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  // Evaluation.
+  llvm::Expected<bool> evaluate(Evaluator &evaluator, NominalTypeDecl *NTD,
+                                ImplicitMemberAction action) const;
+
+public:
+  // Separate caching.
+  bool isCached() const { return true; }
+};
+
 // Allow AnyValue to compare two Type values, even though Type doesn't
 // support ==.
 template<>
@@ -1689,6 +1718,7 @@ AnyValue::Holder<GenericSignature>::equals(const HolderBase &other) const {
 
 void simple_display(llvm::raw_ostream &out, Type value);
 void simple_display(llvm::raw_ostream &out, const TypeRepr *TyR);
+void simple_display(llvm::raw_ostream &out, ImplicitMemberAction action);
 
 #define SWIFT_TYPEID_ZONE TypeChecker
 #define SWIFT_TYPEID_HEADER "swift/AST/TypeCheckerTypeIDZone.def"

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -174,6 +174,9 @@ SWIFT_REQUEST(TypeChecker, HasUserDefinedDesignatedInitRequest,
               bool(NominalTypeDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, HasMemberwiseInitRequest,
               bool(StructDecl *), Cached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, ResolveImplicitMemberRequest,
+              bool(NominalTypeDecl *, ImplicitMemberAction), Uncached,
+              NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, SynthesizeMemberwiseInitRequest,
               ConstructorDecl *(NominalTypeDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, HasDefaultInitRequest,

lib/AST/Decl.cpp

@@ -3956,6 +3956,48 @@ ConstructorDecl *NominalTypeDecl::getDefaultInitializer() const {
                            SynthesizeDefaultInitRequest{mutableThis}, nullptr);
 }
 
+void NominalTypeDecl::synthesizeSemanticMembersIfNeeded(DeclName member) {
+  // Silently break cycles here because we can't be sure when and where a
+  // request to synthesize will come from yet.
+  // FIXME: rdar://56844567
+  if (Bits.NominalTypeDecl.IsComputingSemanticMembers)
+    return;
+    
+  Bits.NominalTypeDecl.IsComputingSemanticMembers = true;
+  SWIFT_DEFER { Bits.NominalTypeDecl.IsComputingSemanticMembers = false; };
+
+  auto baseName = member.getBaseName();
+  auto &Context = getASTContext();
+  Optional<ImplicitMemberAction> action = None;
+  if (baseName == DeclBaseName::createConstructor())
+    action.emplace(ImplicitMemberAction::ResolveImplicitInit);
+
+  if (member.isSimpleName() && !baseName.isSpecial()) {
+    if (baseName.getIdentifier() == getASTContext().Id_CodingKeys) {
+      action.emplace(ImplicitMemberAction::ResolveCodingKeys);
+    }
+  } else {
+    auto argumentNames = member.getArgumentNames();
+    if (member.isCompoundName() && argumentNames.size() != 1)
+      return;
+
+    if (baseName == DeclBaseName::createConstructor() &&
+        (member.isSimpleName() || argumentNames.front() == Context.Id_from)) {
+      action.emplace(ImplicitMemberAction::ResolveDecodable);
+    } else if (!baseName.isSpecial() &&
+               baseName.getIdentifier() == Context.Id_encode &&
+               (member.isSimpleName() ||
+                argumentNames.front() == Context.Id_to)) {
+      action.emplace(ImplicitMemberAction::ResolveEncodable);
+    }
+  }
+
+  if (auto actionToTake = action) {
+    (void)evaluateOrDefault(Context.evaluator,
+        ResolveImplicitMemberRequest{this, actionToTake.getValue()}, false);
+  }
+}
+
 ClassDecl::ClassDecl(SourceLoc ClassLoc, Identifier Name, SourceLoc NameLoc,
                      MutableArrayRef<TypeLoc> Inherited,
                      GenericParamList *GenericParams, DeclContext *Parent)

lib/AST/NameLookup.cpp

@@ -1517,6 +1517,34 @@ bool DeclContext::lookupQualified(Type type,
   return lookupQualified(nominalTypesToLookInto, member, options, decls);
 }
 
+static void installPropertyWrapperMembersIfNeeded(NominalTypeDecl *target,
+                                                  DeclName member) {
+  auto &Context = target->getASTContext();
+  auto baseName = member.getBaseName();
+  if (!member.isSimpleName() || baseName.isSpecial())
+    return;
+
+  if ((!baseName.getIdentifier().str().startswith("$") &&
+       !baseName.getIdentifier().str().startswith("_")) ||
+      baseName.getIdentifier().str().size() <= 1) {
+    return;
+  }
+
+  // $- and _-prefixed variables can be generated by properties that have
+  // attached property wrappers.
+  auto originalPropertyName =
+      Context.getIdentifier(baseName.getIdentifier().str().substr(1));
+  for (auto member : target->lookupDirect(originalPropertyName)) {
+    if (auto var = dyn_cast<VarDecl>(member)) {
+      if (var->hasAttachedPropertyWrapper()) {
+        auto sourceFile = var->getDeclContext()->getParentSourceFile();
+        if (sourceFile && sourceFile->Kind != SourceFileKind::Interface)
+          (void)var->getPropertyWrapperBackingProperty();
+      }
+    }
+  }
+}
+
 bool DeclContext::lookupQualified(ArrayRef<NominalTypeDecl *> typeDecls,
                                   DeclName member,
                                   NLOptions options,
@@ -1572,7 +1600,8 @@ bool DeclContext::lookupQualified(ArrayRef<NominalTypeDecl *> typeDecls,
 
     // Make sure we've resolved implicit members, if we need them.
     if (typeResolver) {
-      typeResolver->resolveImplicitMember(current, member);
+      current->synthesizeSemanticMembersIfNeeded(member);
+      installPropertyWrapperMembersIfNeeded(current, member);
     }
 
     // Look for results within the current nominal type and its extensions.

lib/AST/TypeCheckRequests.cpp

@@ -1071,3 +1071,26 @@ void InheritsSuperclassInitializersRequest::cacheResult(bool value) const {
   auto *decl = std::get<0>(getStorage());
   decl->setInheritsSuperclassInitializers(value);
 }
+
+//----------------------------------------------------------------------------//
+// ResolveImplicitMemberRequest computation.
+//----------------------------------------------------------------------------//
+
+void swift::simple_display(llvm::raw_ostream &out,
+                           ImplicitMemberAction action) {
+  switch (action) {
+  case ImplicitMemberAction::ResolveImplicitInit:
+    out << "resolve implicit initializer";
+    break;
+  case ImplicitMemberAction::ResolveCodingKeys:
+    out << "resolve CodingKeys";
+    break;
+  case ImplicitMemberAction::ResolveEncodable:
+    out << "resolve Encodable.encode(to:)";
+    break;
+  case ImplicitMemberAction::ResolveDecodable:
+    out << "resolve Decodable.init(from:)";
+    break;
+  }
+}
+

lib/Sema/CodeSynthesis.cpp

@@ -1075,12 +1075,12 @@ void TypeChecker::addImplicitConstructors(NominalTypeDecl *decl) {
   (void)decl->getDefaultInitializer();
 }
 
-void TypeChecker::synthesizeMemberForLookup(NominalTypeDecl *target,
-                                            DeclName member) {
-  auto baseName = member.getBaseName();
-
-  if (baseName == DeclBaseName::createConstructor())
-    addImplicitConstructors(target);
+llvm::Expected<bool>
+ResolveImplicitMemberRequest::evaluate(Evaluator &evaluator,
+                                       NominalTypeDecl *target,
+                                       ImplicitMemberAction action) const {
+  // FIXME: This entire request is a layering violation made of smaller,
+  // finickier layering violations. See rdar://56844567
 
   // Checks whether the target conforms to the given protocol. If the
   // conformance is incomplete, force the conformance.
@@ -1091,9 +1091,9 @@ void TypeChecker::synthesizeMemberForLookup(NominalTypeDecl *target,
       return false;
 
     auto targetType = target->getDeclaredInterfaceType();
-    auto ref =
-        conformsToProtocol(targetType, protocol, target,
-                           ConformanceCheckFlags::SkipConditionalRequirements);
+    auto ref = TypeChecker::conformsToProtocol(
+        targetType, protocol, target,
+        ConformanceCheckFlags::SkipConditionalRequirements);
 
     if (ref.isInvalid()) {
       return false;
@@ -1109,67 +1109,50 @@ void TypeChecker::synthesizeMemberForLookup(NominalTypeDecl *target,
     return true;
   };
 
-  if (member.isSimpleName() && !baseName.isSpecial()) {
-    if (baseName.getIdentifier() == Context.Id_CodingKeys) {
-      // CodingKeys is a special type which may be synthesized as part of
-      // Encodable/Decodable conformance. If the target conforms to either
-      // protocol and would derive conformance to either, the type may be
-      // synthesized.
-      // If the target conforms to either and the conformance has not yet been
-      // evaluated, then we should do that here.
-      //
-      // Try to synthesize Decodable first. If that fails, try to synthesize
-      // Encodable. If either succeeds and CodingKeys should have been
-      // synthesized, it will be synthesized.
-      auto *decodableProto = Context.getProtocol(KnownProtocolKind::Decodable);
-      auto *encodableProto = Context.getProtocol(KnownProtocolKind::Encodable);
-      if (!evaluateTargetConformanceTo(decodableProto))
-        (void)evaluateTargetConformanceTo(encodableProto);
-    }
-
-    if ((baseName.getIdentifier().str().startswith("$") ||
-         baseName.getIdentifier().str().startswith("_")) &&
-        baseName.getIdentifier().str().size() > 1) {
-      // $- and _-prefixed variables can be generated by properties that have
-      // attached property wrappers.
-      auto originalPropertyName =
-        Context.getIdentifier(baseName.getIdentifier().str().substr(1));
-      for (auto member : target->lookupDirect(originalPropertyName)) {
-        if (auto var = dyn_cast<VarDecl>(member)) {
-          if (var->hasAttachedPropertyWrapper()) {
-            auto sourceFile = var->getDeclContext()->getParentSourceFile();
-            if (sourceFile && sourceFile->Kind != SourceFileKind::Interface)
-              (void)var->getPropertyWrapperBackingPropertyInfo();
-          }
-        }
-      }
-    }
-
-  } else {
-    auto argumentNames = member.getArgumentNames();
-    if (member.isCompoundName() && argumentNames.size() != 1)
-      return;
-
-    if (baseName == DeclBaseName::createConstructor() &&
-        (member.isSimpleName() || argumentNames.front() == Context.Id_from)) {
-      // init(from:) may be synthesized as part of derived conformance to the
-      // Decodable protocol.
-      // If the target should conform to the Decodable protocol, check the
-      // conformance here to attempt synthesis.
-      auto *decodableProto = Context.getProtocol(KnownProtocolKind::Decodable);
-      (void)evaluateTargetConformanceTo(decodableProto);
-    } else if (!baseName.isSpecial() &&
-               baseName.getIdentifier() == Context.Id_encode &&
-               (member.isSimpleName() ||
-                argumentNames.front() == Context.Id_to)) {
-      // encode(to:) may be synthesized as part of derived conformance to the
-      // Encodable protocol.
-      // If the target should conform to the Encodable protocol, check the
-      // conformance here to attempt synthesis.
-      auto *encodableProto = Context.getProtocol(KnownProtocolKind::Encodable);
+  auto &Context = target->getASTContext();
+  switch (action) {
+  case ImplicitMemberAction::ResolveImplicitInit:
+    TypeChecker::addImplicitConstructors(target);
+    break;
+  case ImplicitMemberAction::ResolveCodingKeys: {
+    // CodingKeys is a special type which may be synthesized as part of
+    // Encodable/Decodable conformance. If the target conforms to either
+    // protocol and would derive conformance to either, the type may be
+    // synthesized.
+    // If the target conforms to either and the conformance has not yet been
+    // evaluated, then we should do that here.
+    //
+    // Try to synthesize Decodable first. If that fails, try to synthesize
+    // Encodable. If either succeeds and CodingKeys should have been
+    // synthesized, it will be synthesized.
+    auto *decodableProto = Context.getProtocol(KnownProtocolKind::Decodable);
+    auto *encodableProto = Context.getProtocol(KnownProtocolKind::Encodable);
+    if (!evaluateTargetConformanceTo(decodableProto)) {
       (void)evaluateTargetConformanceTo(encodableProto);
     }
   }
+    break;
+  case ImplicitMemberAction::ResolveEncodable: {
+    // encode(to:) may be synthesized as part of derived conformance to the
+    // Encodable protocol.
+    // If the target should conform to the Encodable protocol, check the
+    // conformance here to attempt synthesis.
+    auto *encodableProto = Context.getProtocol(KnownProtocolKind::Encodable);
+    (void)evaluateTargetConformanceTo(encodableProto);
+  }
+    break;
+  case ImplicitMemberAction::ResolveDecodable: {
+    // init(from:) may be synthesized as part of derived conformance to the
+    // Decodable protocol.
+    // If the target should conform to the Decodable protocol, check the
+    // conformance here to attempt synthesis.
+    TypeChecker::addImplicitConstructors(target);
+    auto *decodableProto = Context.getProtocol(KnownProtocolKind::Decodable);
+    (void)evaluateTargetConformanceTo(decodableProto);
+  }
+    break;
+  }
+  return true;
 }
 
 llvm::Expected<bool>