Merge pull request #27764 from CodaFi/interface-type-request

Define InterfaceTypeRequest

Author: CodaFi

docs/proposals/DeclarationTypeChecker.rst

@@ -13,13 +13,13 @@ This document describes some of the problems with our current "declaration" type
 Problems with the Current Approach
 ----------------------------------
 
-The current declaration type checker---in particular, ``validateDecl``, which assigns a type to a given declaration---is the source of a large number of Swift bugs, including crashes on both well-formed and ill-formed code, different behavior depending on the order of declarations within a file or across multiple files, infinite recursion, and broken ASTs. The main issues are:
+The current declaration type checker is the source of a large number of Swift bugs, including crashes on both well-formed and ill-formed code, different behavior depending on the order of declarations within a file or across multiple files, infinite recursion, and broken ASTs. The main issues are:
 
 **Conceptual phases are tangled together**: We have a vague notion that there are phases within the compiler, e.g., extension binding occurs before name binding, which occurs before type checking. However, the implementations in the compiler don't respect phases: extension binding goes through type validation, which does both name binding and type checking. Name lookup attempts to do type checking so that it can establish whether one declaration shadows another.
 
 **Unprincipled recursion**: Whenever type checking some particular declaration requires information about another declaration, it recurses to type-check that declaration. There are dozens of these recursion points scattered throughout the compiler, which makes it impossible to reason about the recursion or deal with, e.g., recursion that is too deep for the program stack.
 
-**Ad hoc recursion breaking**: When we do encounter circular dependencies, we have scattered checks for recursion based on a number of separate bits stashed in the AST: ``BeingTypeChecked``, ``EarlyAttrValidation``,  ``ValidatingGenericSignature``, etc. Adding these checks is unprincipled: adding a new check in the wrong place tends to break working code (because the dependency is something the compiler should be able to handle), while missing a check permits infinite recursion to continue.
+**Ad hoc recursion breaking**: When we do encounter circular dependencies, we have scattered checks for recursion based on a number of separate bits stashed in the AST: ``isComputingRequirementSignature()``, ``isComputingPatternBindingEntry()``,  ``isComputingGenericSignature()/hasComputedGenericSignature()``, etc. Adding these checks is unprincipled: adding a new check in the wrong place tends to break working code (because the dependency is something the compiler should be able to handle), while missing a check permits infinite recursion to continue.
 
 **Type checker does too much work**: validating a declaration is all-or-nothing. It includes computing its type, but also checking redeclarations and overrides, as well as numerous other aspects that a user of that declaration might not care about. Aside from the performance impact of checking too much, this can introduce false circularities in type-checking, because the user might only need some very basic information to continue.
 
@@ -156,7 +156,7 @@ The proposed architecture is significantly different from the current type check
 
 **Dependency graph and priority queue**: Extend the current-phase trait with an operation that enumerates the dependencies that need to be satisfied to bring a given AST node up to a particular phase. Start with ``TypeRepr`` nodes, and use the dependency graph and priority queue to satisfy all dependencies ahead of time, eliminating direct recursion from the type-resolution code path. Build circular-dependency detection within this test-bed.
 
-**Incremental adoption of dependency graph**: Make other AST nodes (``Pattern``, ``VarDecl``, etc.) implement the phase-awareness trait, enumerating dependencies and updating their logic to perform minimal updates. Certain entry points that are used for ad hoc recursion (such as ``validateDecl``) can push/pop dependency graph and priority-queue instances, which leaves the existing ad hoc recursion checking in place but allows isolated subproblems to use the newer mechanisms.
+**Incremental adoption of dependency graph**: Make other AST nodes (``Pattern``, ``VarDecl``, etc.) implement the phase-awareness trait, enumerating dependencies and updating their logic to perform minimal updates. Certain entry points that are used for ad hoc recursion can push/pop dependency graph and priority-queue instances, which leaves the existing ad hoc recursion checking in place but allows isolated subproblems to use the newer mechanisms.
 
 **Strengthen accessor assertions**: As ad hoc recursion gets eliminated from the type checker, strengthen assertions on the various AST nodes to make sure the AST node has been brought to the appropriate phase.
 

include/swift/AST/Decl.h

@@ -2467,6 +2467,7 @@ class ValueDecl : public Decl {
   friend class IsFinalRequest;
   friend class IsDynamicRequest;
   friend class IsImplicitlyUnwrappedOptionalRequest;
+  friend class InterfaceTypeRequest;
   friend class Decl;
   SourceLoc getLocFromSource() const { return NameLoc; }
 protected:

include/swift/AST/LazyResolver.h

@@ -53,13 +53,6 @@ class LazyResolver {
   virtual void resolveWitness(const NormalProtocolConformance *conformance,
                               ValueDecl *requirement) = 0;
 
-  /// Resolve the type and declaration attributes of a value.
-  ///
-  /// This can be called when the type or signature of a value is needed.
-  /// It does not perform full type-checking, only checks for basic
-  /// consistency and provides the value a type.
-  virtual void resolveDeclSignature(ValueDecl *VD) = 0;
-
   /// Resolve any implicitly-declared constructors within the given nominal.
   virtual void resolveImplicitConstructors(NominalTypeDecl *nominal) = 0;
 

include/swift/AST/TypeCheckRequests.h

@@ -1427,6 +1427,27 @@ class NamingPatternRequest
   void cacheResult(NamedPattern *P) const;
 };
 
+class InterfaceTypeRequest :
+    public SimpleRequest<InterfaceTypeRequest,
+                         Type (ValueDecl *),
+                         CacheKind::SeparatelyCached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  // Evaluation.
+  llvm::Expected<Type>
+  evaluate(Evaluator &evaluator, ValueDecl *decl) const;
+
+public:
+  // Separate caching.
+  bool isCached() const { return true; }
+  Optional<Type> getCachedResult() const;
+  void cacheResult(Type value) const;
+};
+
 // Allow AnyValue to compare two Type values, even though Type doesn't
 // support ==.
 template<>

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -67,6 +67,8 @@ SWIFT_REQUEST(TypeChecker, InheritedTypeRequest,
               SeparatelyCached, HasNearestLocation)
 SWIFT_REQUEST(TypeChecker, InitKindRequest,
               CtorInitializerKind(ConstructorDecl *), Cached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, InterfaceTypeRequest,
+              Type(ValueDecl *), SeparatelyCached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, IsAccessorTransparentRequest, bool(AccessorDecl *),
               SeparatelyCached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, IsDynamicRequest, bool(ValueDecl *),

lib/AST/Decl.cpp

@@ -2805,30 +2805,23 @@ bool ValueDecl::isRecursiveValidation() const {
 }
 
 Type ValueDecl::getInterfaceType() const {
-  if (!hasInterfaceType()) {
-    // Our clients that don't register the lazy resolver are relying on the
-    // fact that they can't pull an interface type out to avoid doing work.
-    // This is a necessary evil until we can wean them off.
-    if (auto resolver = getASTContext().getLazyResolver()) {
-      resolver->resolveDeclSignature(const_cast<ValueDecl *>(this));
-      if (!hasInterfaceType())
-        return ErrorType::get(getASTContext());
-    }
+  // Our clients that don't register the lazy resolver are relying on the
+  // fact that they can't pull an interface type out to avoid doing work.
+  // This is a necessary evil until we can wean them off.
+  if (!getASTContext().getLazyResolver()) {
+    return TypeAndAccess.getPointer();
   }
-  return TypeAndAccess.getPointer();
+
+  auto ty =
+      evaluateOrDefault(getASTContext().evaluator,
+                        InterfaceTypeRequest{const_cast<ValueDecl *>(this)},
+                        ErrorType::get(getASTContext()));
+  return ty ?: ErrorType::get(getASTContext());
 }
 
 void ValueDecl::setInterfaceType(Type type) {
-  if (type) {
-    assert(!type->hasTypeVariable() && "Type variable in interface type");
-    assert(!type->is<InOutType>() && "Interface type must be materializable");
-    assert(!type->hasArchetype() && "Archetype in interface type");
-
-    if (type->hasError())
-      setInvalid();
-  }
-
-  TypeAndAccess.setPointer(type);
+  getASTContext().evaluator.cacheOutput(InterfaceTypeRequest{this},
+                                        std::move(type));
 }
 
 Optional<ObjCSelector> ValueDecl::getObjCRuntimeName(
@@ -4001,7 +3994,7 @@ ClassAncestryFlagsRequest::evaluate(Evaluator &evaluator, ClassDecl *value) cons
   do {
     // If we hit circularity, we will diagnose at some point in typeCheckDecl().
     // However we have to explicitly guard against that here because we get
-    // called as part of validateDecl().
+    // called as part of the interface type request.
     if (!visited.insert(CD).second)
       break;
 

lib/AST/TypeCheckRequests.cpp

@@ -1001,3 +1001,28 @@ void NamingPatternRequest::cacheResult(NamedPattern *value) const {
   auto *VD = std::get<0>(getStorage());
   VD->NamingPattern = value;
 }
+
+//----------------------------------------------------------------------------//
+// InterfaceTypeRequest computation.
+//----------------------------------------------------------------------------//
+
+Optional<Type> InterfaceTypeRequest::getCachedResult() const {
+  auto *decl = std::get<0>(getStorage());
+  if (auto Ty = decl->TypeAndAccess.getPointer()) {
+    return Ty;
+  }
+  return None;
+}
+
+void InterfaceTypeRequest::cacheResult(Type type) const {
+  auto *decl = std::get<0>(getStorage());
+  if (type) {
+    assert(!type->hasTypeVariable() && "Type variable in interface type");
+    assert(!type->is<InOutType>() && "Interface type must be materializable");
+    assert(!type->hasArchetype() && "Archetype in interface type");
+
+    if (type->hasError())
+      decl->setInvalid();
+  }
+  decl->TypeAndAccess.setPointer(type);
+}

lib/Sema/TypeCheckDecl.cpp

@@ -2287,9 +2287,9 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     // WARNING: Anything you put in this function will only be run when the
     // VarDecl is fully type-checked within its own file. It will NOT be run
     // when the VarDecl is merely used from another file.
-    TC.validateDecl(VD);
 
     // Compute these requests in case they emit diagnostics.
+    (void) VD->getInterfaceType();
     (void) VD->isGetterMutating();
     (void) VD->isSetterMutating();
     (void) VD->getPropertyWrapperBackingProperty();
@@ -4091,15 +4091,9 @@ static Type validateParameterType(ParamDecl *decl) {
   return TL.getType();
 }
 
-void TypeChecker::validateDecl(ValueDecl *D) {
-  // Handling validation failure due to re-entrancy is left
-  // up to the caller, who must call hasInterfaceType() to
-  // check that validateDecl() returned a fully-formed decl.
-  if (D->isBeingValidated() || D->hasInterfaceType())
-    return;
-
-  PrettyStackTraceDecl StackTrace("validating", D);
-  FrontendStatsTracer StatsTracer(Context.Stats, "validate-decl", D);
+llvm::Expected<Type>
+InterfaceTypeRequest::evaluate(Evaluator &eval, ValueDecl *D) const {
+  auto &Context = D->getASTContext();
 
   TypeChecker::checkForForbiddenPrefix(Context, D->getBaseName());
 
@@ -4123,13 +4117,12 @@ void TypeChecker::validateDecl(ValueDecl *D) {
   case DeclKind::OpaqueType:
   case DeclKind::GenericTypeParam:
     llvm_unreachable("should not get here");
-    return;
+    return Type();
 
   case DeclKind::AssociatedType: {
     auto assocType = cast<AssociatedTypeDecl>(D);
     auto interfaceTy = assocType->getDeclaredInterfaceType();
-    assocType->setInterfaceType(MetatypeType::get(interfaceTy, Context));
-    break;
+    return MetatypeType::get(interfaceTy, Context);
   }
 
   case DeclKind::TypeAlias: {
@@ -4146,8 +4139,7 @@ void TypeChecker::validateDecl(ValueDecl *D) {
       parent = parentDC->getSelfInterfaceType();
     auto sugaredType = TypeAliasType::get(typeAlias, parent, subs,
                                           typeAlias->getUnderlyingType());
-    typeAlias->setInterfaceType(MetatypeType::get(sugaredType, Context));
-    break;
+    return MetatypeType::get(sugaredType, Context);
   }
 
   case DeclKind::Enum:
@@ -4156,8 +4148,7 @@ void TypeChecker::validateDecl(ValueDecl *D) {
   case DeclKind::Protocol: {
     auto nominal = cast<NominalTypeDecl>(D);
     Type declaredInterfaceTy = nominal->getDeclaredInterfaceType();
-    nominal->setInterfaceType(MetatypeType::get(declaredInterfaceTy, Context));
-    break;
+    return MetatypeType::get(declaredInterfaceTy, Context);
   }
 
   case DeclKind::Param: {
@@ -4167,40 +4158,33 @@ void TypeChecker::validateDecl(ValueDecl *D) {
       auto selfParam = computeSelfParam(AFD,
                                         /*isInitializingCtor*/true,
                                         /*wantDynamicSelf*/true);
-      PD->setInterfaceType(selfParam.getPlainType());
-      break;
+      return selfParam.getPlainType();
     }
 
     if (auto *accessor = dyn_cast<AccessorDecl>(PD->getDeclContext())) {
       auto *storage = accessor->getStorage();
       auto *originalParam = getOriginalParamFromAccessor(
         storage, accessor, PD);
       if (originalParam == nullptr) {
-        auto type = storage->getValueInterfaceType();
-        PD->setInterfaceType(type);
-        break;
+        return storage->getValueInterfaceType();
       }
 
       if (originalParam != PD) {
-        PD->setInterfaceType(originalParam->getInterfaceType());
-        break;
+        return originalParam->getInterfaceType();
       }
     }
 
     if (!PD->getTypeRepr())
-      return;
+      return Type();
 
-    auto ty = validateParameterType(PD);
-    PD->setInterfaceType(ty);
-    break;
+    return validateParameterType(PD);
   }
 
   case DeclKind::Var: {
     auto *VD = cast<VarDecl>(D);
     auto *namingPattern = VD->getNamingPattern();
     if (!namingPattern) {
-      VD->setInterfaceType(ErrorType::get(Context));
-      break;
+      return ErrorType::get(Context);
     }
 
     Type interfaceType = namingPattern->getType();
@@ -4213,9 +4197,8 @@ void TypeChecker::validateDecl(ValueDecl *D) {
         interfaceType =
             TypeChecker::checkReferenceOwnershipAttr(VD, interfaceType, attr);
     }
-    VD->setInterfaceType(interfaceType);
 
-    break;
+    return interfaceType;
   }
 
   case DeclKind::Func:
@@ -4224,8 +4207,54 @@ void TypeChecker::validateDecl(ValueDecl *D) {
   case DeclKind::Destructor: {
     auto *AFD = cast<AbstractFunctionDecl>(D);
     DeclValidationRAII IBV(AFD);
-    AFD->computeType();
-    break;
+
+    auto sig = AFD->getGenericSignature();
+    bool hasSelf = AFD->hasImplicitSelfDecl();
+
+    AnyFunctionType::ExtInfo info;
+
+    // Result
+    Type resultTy;
+    if (auto fn = dyn_cast<FuncDecl>(D)) {
+      resultTy = fn->getResultInterfaceType();
+    } else if (auto ctor = dyn_cast<ConstructorDecl>(D)) {
+      resultTy = ctor->getResultInterfaceType();
+    } else {
+      assert(isa<DestructorDecl>(D));
+      resultTy = TupleType::getEmpty(AFD->getASTContext());
+    }
+
+    // (Args...) -> Result
+    Type funcTy;
+
+    {
+      SmallVector<AnyFunctionType::Param, 4> argTy;
+      AFD->getParameters()->getParams(argTy);
+
+      // 'throws' only applies to the innermost function.
+      info = info.withThrows(AFD->hasThrows());
+      // Defer bodies must not escape.
+      if (auto fd = dyn_cast<FuncDecl>(D))
+        info = info.withNoEscape(fd->isDeferBody());
+
+      if (sig && !hasSelf) {
+        funcTy = GenericFunctionType::get(sig, argTy, resultTy, info);
+      } else {
+        funcTy = FunctionType::get(argTy, resultTy, info);
+      }
+    }
+
+    // (Self) -> (Args...) -> Result
+    if (hasSelf) {
+      // Substitute in our own 'self' parameter.
+      auto selfParam = computeSelfParam(AFD);
+      if (sig)
+        funcTy = GenericFunctionType::get(sig, {selfParam}, funcTy);
+      else
+        funcTy = FunctionType::get({selfParam}, funcTy);
+    }
+
+    return funcTy;
   }
 
   case DeclKind::Subscript: {
@@ -4243,9 +4272,7 @@ void TypeChecker::validateDecl(ValueDecl *D) {
     else
       funcTy = FunctionType::get(argTy, elementTy);
 
-    // Record the interface type.
-    SD->setInterfaceType(funcTy);
-    break;
+    return funcTy;
   }
 
   case DeclKind::EnumElement: {
@@ -4272,13 +4299,9 @@ void TypeChecker::validateDecl(ValueDecl *D) {
     else
       resultTy = FunctionType::get({selfTy}, resultTy);
 
-    // Record the interface type.
-    EED->setInterfaceType(resultTy);
-    break;
+    return resultTy;
   }
   }
-
-  assert(D->hasInterfaceType());
 }
 
 llvm::Expected<NamedPattern *>

lib/Sema/TypeCheckStorage.cpp

@@ -39,7 +39,7 @@ void swift::setBoundVarsTypeError(Pattern *pattern, ASTContext &ctx) {
   pattern->forEachVariable([&](VarDecl *var) {
     // Don't change the type of a variable that we've been able to
     // compute a type for.
-    if (var->hasInterfaceType() && !var->getType()->hasError())
+    if (var->hasInterfaceType())
       return;
 
     var->setInterfaceType(ErrorType::get(var->getASTContext()));