Merge pull request swiftlang#6043 from slavapestov/circularity

Consolidate circularity checks for recursive validateDecl() calls

Author: slavapestov

include/swift/AST/Decl.h

@@ -215,7 +215,7 @@ bool conflicting(const OverloadSignature& sig1, const OverloadSignature& sig2);
 class alignas(1 << DeclAlignInBits) Decl {
   class DeclBitfields {
     friend class Decl;
-    unsigned Kind : 8;
+    unsigned Kind : 6;
 
     /// \brief Whether this declaration is invalid.
     unsigned Invalid : 1;
@@ -233,10 +233,10 @@ class alignas(1 << DeclAlignInBits) Decl {
     /// FIXME: This is ugly.
     unsigned EarlyAttrValidation : 1;
 
-    /// \brief Whether or not this declaration is currently being type-checked.
-    unsigned BeingTypeChecked : 1;
+    /// \brief Whether this declaration is currently being validated.
+    unsigned BeingValidated : 1;
   };
-  enum { NumDeclBits = 13 };
+  enum { NumDeclBits = 11 };
   static_assert(NumDeclBits <= 32, "fits in an unsigned");
 
   class PatternBindingDeclBitfields {
@@ -632,7 +632,7 @@ class alignas(1 << DeclAlignInBits) Decl {
     DeclBits.Implicit = false;
     DeclBits.FromClang = false;
     DeclBits.EarlyAttrValidation = false;
-    DeclBits.BeingTypeChecked = false;
+    DeclBits.BeingValidated = false;
   }
 
   ClangNode getClangNodeImpl() const {
@@ -778,12 +778,16 @@ class alignas(1 << DeclAlignInBits) Decl {
     DeclBits.EarlyAttrValidation = validated;
   }
 
-  /// Whether the declaration is currently being validated.
-  bool isBeingTypeChecked();
-  
+  /// Whether the declaration has a valid interface type and
+  /// generic signature.
+  bool isBeingValidated() const {
+    return DeclBits.BeingValidated;
+  }
+
   /// Toggle whether or not the declaration is being validated.
-  void setIsBeingTypeChecked(bool ibt = true) {
-    DeclBits.BeingTypeChecked = ibt;
+  void setIsBeingValidated(bool ibv = true) {
+    assert(DeclBits.BeingValidated != ibv);
+    DeclBits.BeingValidated = ibv;
   }
 
   /// \returns the unparsed comment attached to this declaration.
@@ -1522,7 +1526,7 @@ class ExtensionDecl final : public Decl, public DeclContext,
 
   void setInherited(MutableArrayRef<TypeLoc> i) { Inherited = i; }
 
-  /// Whether we already validated this extension.
+  /// Whether we started validating this extension.
   bool validated() const {
     return ExtensionDeclBits.Validated;
   }
@@ -1532,6 +1536,11 @@ class ExtensionDecl final : public Decl, public DeclContext,
     ExtensionDeclBits.Validated = validated;
   }
 
+  /// Whether we have fully checked the extension.
+  bool hasValidSignature() const {
+    return validated() && !isBeingValidated();
+  }
+
   /// Whether we already type-checked the inheritance clause.
   bool checkedInheritanceClause() const {
     return ExtensionDeclBits.CheckedInheritanceClause;
@@ -2111,7 +2120,11 @@ class ValueDecl : public Decl {
 
   /// Set the interface type for the given value.
   void setInterfaceType(Type type);
-  
+
+  bool hasValidSignature() const {
+    return hasInterfaceType() && !isBeingValidated();
+  }
+
   /// isSettable - Determine whether references to this decl may appear
   /// on the left-hand side of an assignment or as the operand of a
   /// `&` or 'inout' operator.
@@ -2257,11 +2270,6 @@ class GenericTypeDecl : public TypeDecl, public DeclContext {
   mutable llvm::PointerUnion<GenericSignature *, GenericEnvironment *>
     GenericSigOrEnv;
 
-  /// \brief Whether or not the generic signature of the type declaration is
-  /// currently being validated.
-  // TODO: Merge into GenericSig bits.
-  unsigned ValidatingGenericSignature = false;
-
   /// Lazily populate the generic environment.
   GenericEnvironment *getLazyGenericEnvironmentSlow() const;
 
@@ -2299,14 +2307,6 @@ class GenericTypeDecl : public TypeDecl, public DeclContext {
   /// Retrieve the generic context for this type.
   GenericEnvironment *getGenericEnvironment() const;
 
-  void setIsValidatingGenericSignature(bool validating=true) {
-    ValidatingGenericSignature = validating;
-  }
-  
-  bool isValidatingGenericSignature() const {
-    return ValidatingGenericSignature;
-  }
-
   /// Set a lazy generic environment.
   void setLazyGenericEnvironment(LazyMemberLoader *lazyLoader,
                                  GenericSignature *genericSig,

include/swift/AST/DiagnosticsSema.def

@@ -1173,8 +1173,6 @@ ERROR(unsupported_nested_protocol,none,
       (Identifier))
 
 // Type aliases
-ERROR(circular_type_alias,none,
-      "type alias %0 circularly references itself", (Identifier))
 ERROR(type_alias_underlying_type_access,none,
       "type alias %select{must be declared %select{"
       "%select{private|fileprivate|internal|PUBLIC}2|private or fileprivate}3"
@@ -1548,7 +1546,7 @@ ERROR(requires_superclass_conflict,none,
       "generic parameter %0 cannot be a subclass of both %1 and %2",
       (Identifier, Type, Type))
 ERROR(recursive_type_reference,none,
-      "type %0 references itself", (Identifier))
+      "%0 %1 references itself", (DescriptiveDeclKind, Identifier))
 ERROR(recursive_requirement_reference,none,
       "type may not reference itself as a requirement",())
 ERROR(recursive_same_type_constraint,none,

lib/AST/Decl.cpp

@@ -375,24 +375,6 @@ bool AbstractFunctionDecl::isTransparent() const {
   return false;
 }
 
-bool Decl::isBeingTypeChecked() {
-  auto decl = this;
-  while (true) {
-    if (decl->DeclBits.BeingTypeChecked)
-      return true;
-
-    auto dc = decl->getDeclContext();
-    if (auto nominal = dyn_cast<NominalTypeDecl>(dc))
-      decl = nominal;
-    else if (auto ext = dyn_cast<ExtensionDecl>(dc))
-      decl = ext;
-    else
-      break;
-  }
-
-  return false;
-}
-
 bool Decl::isPrivateStdlibDecl(bool whitelistProtocols) const {
   const Decl *D = this;
   if (auto ExtD = dyn_cast<ExtensionDecl>(D))
@@ -1937,6 +1919,15 @@ Type TypeDecl::getDeclaredInterfaceType() const {
   if (auto *NTD = dyn_cast<NominalTypeDecl>(this))
     return NTD->getDeclaredInterfaceType();
 
+  if (auto *ATD = dyn_cast<AssociatedTypeDecl>(this)) {
+    auto &ctx = getASTContext();
+    auto selfTy = getDeclContext()->getSelfInterfaceType();
+    if (!selfTy)
+      return ErrorType::get(ctx);
+    return DependentMemberType::get(
+        selfTy, const_cast<AssociatedTypeDecl *>(ATD));
+  }
+
   Type interfaceType = getInterfaceType();
   if (interfaceType.isNull() || interfaceType->is<ErrorType>())
     return interfaceType;
@@ -2363,11 +2354,7 @@ AssociatedTypeDecl::AssociatedTypeDecl(DeclContext *dc, SourceLoc keywordLoc,
 
 void AssociatedTypeDecl::computeType() {
   auto &ctx = getASTContext();
-  auto proto = cast<ProtocolDecl>(getDeclContext());
-  auto selfTy = proto->getSelfInterfaceType();
-  if (!selfTy)
-    selfTy = ErrorType::get(ctx);
-  auto interfaceTy = DependentMemberType::get(selfTy, this);
+  auto interfaceTy = getDeclaredInterfaceType();
   setInterfaceType(MetatypeType::get(interfaceTy, ctx));
 }
 
@@ -2719,7 +2706,7 @@ bool ProtocolDecl::requiresClassSlow() {
   ProtocolDeclBits.RequiresClass = false;
 
   // Ensure that the result cannot change in future.
-  assert(isInheritedProtocolsValid() || isBeingTypeChecked());
+  assert(isInheritedProtocolsValid());
 
   if (getAttrs().hasAttribute<ObjCAttr>() || isObjC()) {
     ProtocolDeclBits.RequiresClass = true;

lib/AST/GenericEnvironment.cpp

@@ -305,7 +305,8 @@ Type GenericEnvironment::mapTypeIntoContext(ModuleDecl *M, Type type) const {
 Type GenericEnvironment::mapTypeIntoContext(GenericTypeParamType *type) const {
   auto self = const_cast<GenericEnvironment *>(this);
   Type result = QueryInterfaceTypeSubstitutions(self)(type);
-  assert(result && "Missing context type for generic parameter");
+  if (!result)
+    return ErrorType::get(type);
   return result;
 }
 

lib/AST/NameLookup.cpp

@@ -149,33 +149,20 @@ bool swift::removeShadowedDecls(SmallVectorImpl<ValueDecl*> &decls,
     ObjCCollidingConstructors;
   bool anyCollisions = false;
   for (auto decl : decls) {
-    // Determine the signature of this declaration.
-    // FIXME: the canonical type makes a poor signature, because we don't
-    // canonicalize away default arguments and don't canonicalize polymorphic
-    // types well.
-    CanType signature;
-
     // FIXME: Egregious hack to avoid failing when there are no declared types.
-    if (!decl->hasInterfaceType() ||
-        isa<TypeAliasDecl>(decl) ||
-        isa<AbstractTypeParamDecl>(decl)) {
-      // FIXME: Pass this down instead of getting it from the ASTContext.
-      if (typeResolver && !decl->isBeingTypeChecked())
-        typeResolver->resolveDeclSignature(decl);
-      if (!decl->hasInterfaceType())
-        continue;
-      if (auto assocType = dyn_cast<AssociatedTypeDecl>(decl))
-        if (!assocType->getProtocol()->isValidGenericContext())
-          continue;
-    }
+    // FIXME: Pass this down instead of getting it from the ASTContext.
+    if (typeResolver)
+      typeResolver->resolveDeclSignature(decl);
 
     // If the decl is currently being validated, this is likely a recursive
     // reference and we'll want to skip ahead so as to avoid having its type
     // attempt to desugar itself.
-    if (decl->isBeingTypeChecked())
+    if (!decl->hasValidSignature())
       continue;
 
-    signature = decl->getInterfaceType()->getCanonicalType();
+    // FIXME: the canonical type makes a poor signature, because we don't
+    // canonicalize away default arguments.
+    auto signature = decl->getInterfaceType()->getCanonicalType();
 
     // FIXME: The type of a variable or subscript doesn't include
     // enough context to distinguish entities from different
@@ -1497,12 +1484,12 @@ bool DeclContext::lookupQualified(Type type,
   // Allow filtering of the visible declarations based on various
   // criteria.
   bool onlyCompleteObjectInits = false;
-  auto isAcceptableDecl = [&](NominalTypeDecl *current, Decl *decl) -> bool {
+  auto isAcceptableDecl = [&](NominalTypeDecl *current, ValueDecl *decl) -> bool {
     // If the decl is currently being type checked, then we have something
     // cyclic going on.  Instead of poking at parts that are potentially not
     // set up, just assume it is acceptable.  This will make sure we produce an
     // error later.
-    if (decl->isBeingTypeChecked())
+    if (!decl->hasValidSignature())
       return true;
 
     // Filter out designated initializers, if requested.
@@ -1523,8 +1510,7 @@ bool DeclContext::lookupQualified(Type type,
 
     // Check access.
     if (!(options & NL_IgnoreAccessibility))
-      if (auto VD = dyn_cast<ValueDecl>(decl))
-        return VD->isAccessibleFrom(this);
+      return decl->isAccessibleFrom(this);
 
     return true;
   };
@@ -1564,13 +1550,9 @@ bool DeclContext::lookupQualified(Type type,
 
       // Resolve the declaration signature when we find the
       // declaration.
-      if (typeResolver && !decl->isBeingTypeChecked()) {
+      if (typeResolver)
         typeResolver->resolveDeclSignature(decl);
 
-        if (!decl->hasInterfaceType())
-          continue;
-      }
-
       if (isAcceptableDecl(current, decl))
         decls.push_back(decl);
     }
@@ -1635,11 +1617,8 @@ bool DeclContext::lookupQualified(Type type,
       if ((options & NL_OnlyTypes) && !isa<TypeDecl>(decl))
         continue;
 
-      if (typeResolver && !decl->isBeingTypeChecked()) {
+      if (typeResolver)
         typeResolver->resolveDeclSignature(decl);
-        if (!decl->hasInterfaceType())
-          continue;
-      }
 
       // If the declaration has an override, name lookup will also have
       // found the overridden method. Skip this declaration, because we

lib/Sema/CSGen.cpp

@@ -1021,7 +1021,7 @@ namespace {
       if (!decl)
         return nullptr;
 
-      CS.getTypeChecker().validateDecl(decl, true);
+      CS.getTypeChecker().validateDecl(decl);
       if (decl->isInvalid())
         return nullptr;
 
@@ -1312,7 +1312,7 @@ namespace {
       // FIXME: If the decl is in error, we get no information from this.
       // We may, alternatively, want to use a type variable in that case,
       // and possibly infer the type of the variable that way.
-      CS.getTypeChecker().validateDecl(E->getDecl(), true);
+      CS.getTypeChecker().validateDecl(E->getDecl());
       if (E->getDecl()->isInvalid())
         return nullptr;
 
@@ -1390,7 +1390,7 @@ namespace {
         // If the result is invalid, skip it.
         // FIXME: Note this as invalid, in case we don't find a solution,
         // so we don't let errors cascade further.
-        CS.getTypeChecker().validateDecl(decls[i], true);
+        CS.getTypeChecker().validateDecl(decls[i]);
         if (decls[i]->isInvalid())
           continue;
 

lib/Sema/CSSimplify.cpp

@@ -2942,7 +2942,7 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
     bool labelMismatch = false;
     for (auto ctor : ctors) {
       // If the constructor is invalid, we fail entirely to avoid error cascade.
-      TC.validateDecl(ctor, true);
+      TC.validateDecl(ctor);
       if (ctor->isInvalid())
         return result.markErrorAlreadyDiagnosed();
 
@@ -3038,7 +3038,7 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
       return;
     }
     // If the result is invalid, skip it.
-    TC.validateDecl(cand, true);
+    TC.validateDecl(cand);
     if (cand->isInvalid()) {
       result.markErrorAlreadyDiagnosed();
       return;
@@ -3228,7 +3228,7 @@ performMemberLookup(ConstraintKind constraintKind, DeclName memberName,
                                   memberName, lookupOptions);
     for (auto cand : lookup) {
       // If the result is invalid, skip it.
-      TC.validateDecl(cand, true);
+      TC.validateDecl(cand);
       if (cand->isInvalid()) {
         result.markErrorAlreadyDiagnosed();
         return result;