Merge pull request swiftlang#34123 from DougGregor/circular-inheritance-decl

DougGregor · web-flow · commit 44c8052bb531 · 2020-09-30T00:02:00.000-07:00
Teach SuperclassDeclRequest to always diagnose circularity.
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -3920,9 +3920,6 @@ class ClassDecl final : public NominalTypeDecl {
   /// Set the superclass of this class.
   void setSuperclass(Type superclass);
 
-  /// Whether this class has a circular reference in its inheritance hierarchy.
-  bool hasCircularInheritance() const;
-
   /// Walk this class and all of the superclasses of this class, transitively,
   /// invoking the callback function for each class.
   ///
diff --git a/include/swift/AST/NameLookupRequests.h b/include/swift/AST/NameLookupRequests.h
@@ -160,6 +160,10 @@ class SuperclassDeclRequest :
   evaluate(Evaluator &evaluator, NominalTypeDecl *subject) const;
 
 public:
+  // Cycle handling
+  void diagnoseCycle(DiagnosticEngine &diags) const;
+  void noteCycleStep(DiagnosticEngine &diags) const;
+
   // Caching
   bool isCached() const { return true; }
   Optional<ClassDecl *> getCachedResult() const;
diff --git a/include/swift/AST/TypeCheckRequests.h b/include/swift/AST/TypeCheckRequests.h
@@ -1986,29 +1986,6 @@ class PreCheckFunctionBuilderRequest
   bool isCached() const { return true; }
 };
 
-/// Computes whether a class has a circular reference in its inheritance
-/// hierarchy.
-class HasCircularInheritanceRequest
-    : public SimpleRequest<HasCircularInheritanceRequest, bool(ClassDecl *),
-                           RequestFlags::Cached> {
-public:
-  using SimpleRequest::SimpleRequest;
-
-private:
-  friend SimpleRequest;
-
-  // Evaluation.
-  bool evaluate(Evaluator &evaluator, ClassDecl *decl) const;
-
-public:
-  // Cycle handling.
-  void diagnoseCycle(DiagnosticEngine &diags) const;
-  void noteCycleStep(DiagnosticEngine &diags) const;
-
-  // Cached.
-  bool isCached() const { return true; }
-};
-
 /// Computes whether a protocol has a circular reference in its list of
 /// inherited protocols.
 class HasCircularInheritedProtocolsRequest
diff --git a/include/swift/AST/TypeCheckerTypeIDZone.def b/include/swift/AST/TypeCheckerTypeIDZone.def
@@ -95,8 +95,6 @@ SWIFT_REQUEST(TypeChecker, FunctionOperatorRequest, OperatorDecl *(FuncDecl *),
 SWIFT_REQUEST(NameLookup, GenericSignatureRequest,
               GenericSignature (GenericContext *),
               SeparatelyCached, NoLocationInfo)
-SWIFT_REQUEST(TypeChecker, HasCircularInheritanceRequest,
-              bool(ClassDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, HasCircularInheritedProtocolsRequest,
               bool(ProtocolDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, HasCircularRawValueRequest,
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -4148,13 +4148,6 @@ void NominalTypeDecl::synthesizeSemanticMembersIfNeeded(DeclName member) {
   }
 }
 
-bool ClassDecl::hasCircularInheritance() const {
-  auto &ctx = getASTContext();
-  auto *mutableThis = const_cast<ClassDecl *>(this);
-  return evaluateOrDefault(ctx.evaluator,
-                           HasCircularInheritanceRequest{mutableThis}, true);
-}
-
 ClassDecl::ClassDecl(SourceLoc ClassLoc, Identifier Name, SourceLoc NameLoc,
                      MutableArrayRef<TypeLoc> Inherited,
                      GenericParamList *GenericParams, DeclContext *Parent)
@@ -4276,7 +4269,7 @@ bool ClassDecl::isIncompatibleWithWeakReferences() const {
 
 bool ClassDecl::inheritsSuperclassInitializers() const {
   // If there's no superclass, there's nothing to inherit.
-  if (!getSuperclass())
+  if (!getSuperclassDecl())
     return false;
 
   auto &ctx = getASTContext();
@@ -4294,19 +4287,11 @@ AncestryOptions ClassDecl::checkAncestry() const {
 AncestryFlags
 ClassAncestryFlagsRequest::evaluate(Evaluator &evaluator,
                                     ClassDecl *value) const {
-  llvm::SmallPtrSet<const ClassDecl *, 8> visited;
-
   AncestryOptions result;
   const ClassDecl *CD = value;
   auto *M = value->getParentModule();
 
   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 the interface type request.
-    if (!visited.insert(CD).second)
-      break;
-
     if (CD->isGenericContext())
       result |= AncestryFlags::Generic;
 
@@ -4496,10 +4481,9 @@ ClassDecl::findImplementingMethod(const AbstractFunctionDecl *Method) const {
 bool ClassDecl::walkSuperclasses(
     llvm::function_ref<TypeWalker::Action(ClassDecl *)> fn) const {
 
-  SmallPtrSet<ClassDecl *, 8> seen;
   auto *cls = const_cast<ClassDecl *>(this);
 
-  while (cls && seen.insert(cls).second) {
+  while (cls) {
     switch (fn(cls)) {
     case TypeWalker::Action::Stop:
       return true;
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -2290,12 +2290,33 @@ SuperclassDeclRequest::evaluate(Evaluator &evaluator,
                                   inheritedTypes, modulesFound, anyObject);
 
     // Look for a class declaration.
+    ClassDecl *superclass = nullptr;
     for (auto inheritedNominal : inheritedNominalTypes) {
-      if (auto classDecl = dyn_cast<ClassDecl>(inheritedNominal))
-        return classDecl;
+      if (auto classDecl = dyn_cast<ClassDecl>(inheritedNominal)) {
+        superclass = classDecl;
+        break;
+      }
     }
-  }
 
+    // If we found a superclass, ensure that we don't have a circular
+    // inheritance hierarchy by evaluating its superclass. This forces the
+    // diagnostic at this point and then suppresses the superclass failure.
+    if (superclass) {
+      auto result = Ctx.evaluator(SuperclassDeclRequest{superclass});
+      bool hadCycle = false;
+      if (auto err = result.takeError()) {
+        llvm::handleAllErrors(std::move(err),
+          [&hadCycle](const CyclicalRequestError<SuperclassDeclRequest> &E) {
+          hadCycle = true;
+          });
+
+        if (hadCycle)
+          return nullptr;
+      }
+
+      return superclass;
+    }
+  }
 
   return nullptr;
 }
diff --git a/lib/AST/NameLookupRequests.cpp b/lib/AST/NameLookupRequests.cpp
@@ -45,6 +45,19 @@ SourceLoc InheritedDeclsReferencedRequest::getNearestLoc() const {
 //----------------------------------------------------------------------------//
 // Superclass declaration computation.
 //----------------------------------------------------------------------------//
+void SuperclassDeclRequest::diagnoseCycle(DiagnosticEngine &diags) const {
+  // FIXME: Improve this diagnostic.
+  auto nominalDecl = std::get<0>(getStorage());
+  diags.diagnose(nominalDecl, diag::circular_class_inheritance,
+                 nominalDecl->getName());
+}
+
+void SuperclassDeclRequest::noteCycleStep(DiagnosticEngine &diags) const {
+  auto decl = std::get<0>(getStorage());
+  diags.diagnose(decl, diag::kind_declname_declared_here,
+                 decl->getDescriptiveKind(), decl->getName());
+}
+
 Optional<ClassDecl *> SuperclassDeclRequest::getCachedResult() const {
   auto nominalDecl = std::get<0>(getStorage());
 
diff --git a/lib/AST/TypeCheckRequests.cpp b/lib/AST/TypeCheckRequests.cpp
@@ -1136,23 +1136,6 @@ void swift::simple_display(llvm::raw_ostream &out,
   }
 }
 
-//----------------------------------------------------------------------------//
-// HasCircularInheritanceRequest computation.
-//----------------------------------------------------------------------------//
-
-void HasCircularInheritanceRequest::diagnoseCycle(
-    DiagnosticEngine &diags) const {
-  auto *decl = std::get<0>(getStorage());
-  diags.diagnose(decl, diag::circular_class_inheritance, decl->getName());
-}
-
-void HasCircularInheritanceRequest::noteCycleStep(
-    DiagnosticEngine &diags) const {
-  auto *decl = std::get<0>(getStorage());
-  diags.diagnose(decl, diag::kind_declname_declared_here,
-                 decl->getDescriptiveKind(), decl->getName());
-}
-
 //----------------------------------------------------------------------------//
 // HasCircularInheritedProtocolsRequest computation.
 //----------------------------------------------------------------------------//
diff --git a/lib/SILGen/SILGen.cpp b/lib/SILGen/SILGen.cpp
@@ -1288,8 +1288,8 @@ bool SILGenModule::requiresIVarDestroyer(ClassDecl *cd) {
   // Only needed if we have non-trivial ivars, we're not a root class, and
   // the superclass is not @objc.
   return (hasNonTrivialIVars(cd) &&
-          cd->getSuperclass() &&
-          !cd->getSuperclass()->getClassOrBoundGenericClass()->hasClangNode());
+          cd->getSuperclassDecl() &&
+          !cd->getSuperclassDecl()->hasClangNode());
 }
 
 /// TODO: This needs a better name.
diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp
@@ -1076,12 +1076,11 @@ InheritsSuperclassInitializersRequest::evaluate(Evaluator &eval,
   if (decl->getAttrs().hasAttribute<InheritsConvenienceInitializersAttr>())
     return true;
 
-  auto superclass = decl->getSuperclass();
-  assert(superclass);
+  auto superclassDecl = decl->getSuperclassDecl();
+  assert(superclassDecl);
 
   // If the superclass has known-missing designated initializers, inheriting
   // is unsafe.
-  auto *superclassDecl = superclass->getClassOrBoundGenericClass();
   if (superclassDecl->getModuleContext() != decl->getParentModule() &&
       superclassDecl->hasMissingDesignatedInitializers())
     return false;
@@ -1357,7 +1356,7 @@ HasDefaultInitRequest::evaluate(Evaluator &evaluator,
   // Don't synthesize a default for a subclass, it will attempt to inherit its
   // initializers from its superclass.
   if (auto *cd = dyn_cast<ClassDecl>(decl))
-    if (cd->getSuperclass())
+    if (cd->getSuperclassDecl())
       return false;
 
   // If the user has already defined a designated initializer, then don't
diff --git a/lib/Sema/DerivedConformanceCodable.cpp b/lib/Sema/DerivedConformanceCodable.cpp
@@ -30,10 +30,15 @@ using namespace swift;
 /// Returns whether the type represented by the given ClassDecl inherits from a
 /// type which conforms to the given protocol.
 static bool superclassConformsTo(ClassDecl *target, KnownProtocolKind kpk) {
-  if (!target || !target->getSuperclass() || target->hasCircularInheritance()) {
+  if (!target) {
     return false;
   }
-  return !target->getSuperclassDecl()
+
+  auto superclass = target->getSuperclassDecl();
+  if (!superclass)
+    return false;
+
+  return !superclass
               ->getModuleContext()
               ->lookupConformance(target->getSuperclass(),
                                   target->getASTContext().getProtocol(kpk))
diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp
@@ -233,24 +233,6 @@ static bool canSkipCircularityCheck(NominalTypeDecl *decl) {
   return decl->hasClangNode() || decl->wasDeserialized();
 }
 
-bool
-HasCircularInheritanceRequest::evaluate(Evaluator &evaluator,
-                                        ClassDecl *decl) const {
-  if (canSkipCircularityCheck(decl) || !decl->hasSuperclass())
-    return false;
-
-  auto *superclass = decl->getSuperclassDecl();
-  auto result = evaluator(HasCircularInheritanceRequest{superclass});
-
-  // If we have a cycle, handle it and return true.
-  if (!result) {
-    using Error = CyclicalRequestError<HasCircularInheritanceRequest>;
-    llvm::handleAllErrors(result.takeError(), [](const Error &E) {});
-    return true;
-  }
-  return result.get();
-}
-
 bool
 HasCircularInheritedProtocolsRequest::evaluate(Evaluator &evaluator,
                                                ProtocolDecl *decl) const {
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -2011,7 +2011,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     checkGenericParams(CD);
 
     // Check for circular inheritance.
-    (void)CD->hasCircularInheritance();
+    (void)CD->getSuperclassDecl();
 
     // Force lowering of stored properties.
     (void) CD->getStoredProperties();
diff --git a/lib/Sema/TypeCheckRequestFunctions.cpp b/lib/Sema/TypeCheckRequestFunctions.cpp
@@ -93,6 +93,12 @@ SuperclassTypeRequest::evaluate(Evaluator &evaluator,
       return proto->getGenericSignature()
           ->getSuperclassBound(proto->getSelfInterfaceType());
     }
+
+    if (!proto->getSuperclassDecl())
+      return Type();
+  } else if (auto classDecl = dyn_cast<ClassDecl>(nominalDecl)) {
+    if (!classDecl->getSuperclassDecl())
+      return Type();
   }
 
   for (unsigned int idx : indices(nominalDecl->getInherited())) {
diff --git a/test/decl/class/circular_inheritance.swift b/test/decl/class/circular_inheritance.swift
@@ -2,13 +2,14 @@
 // RUN: mkdir -p %t/stats-dir
 // RUN: %target-typecheck-verify-swift
 // RUN: not %target-swift-frontend -typecheck -debug-cycles %s -build-request-dependency-graph -output-request-graphviz %t.dot -stats-output-dir %t/stats-dir 2> %t.cycles
+// RUN: %FileCheck -check-prefix CHECK-DOT %s < %t.dot
 
-class Left // expected-error {{circular reference}} expected-note {{through reference here}}
+class Left // expected-error {{'Left' inherits from itself}} expected-note {{through reference here}}
     : Right.Hand { // expected-note {{through reference here}}
   class Hand {}
 }
 
-class Right // expected-note 2 {{through reference here}}
+class Right // expected-note {{through reference here}} expected-note{{class 'Right' declared here}}
   : Left.Hand { // expected-note {{through reference here}}
   class Hand {}
 }
@@ -23,58 +24,42 @@ protocol P : P {} // expected-error {{protocol 'P' refines itself}}
 class Isomorphism : Automorphism { }
 class Automorphism : Automorphism { } // expected-error{{'Automorphism' inherits from itself}}
 
-// FIXME: Useless error
-let _ = A() // expected-error{{'A' cannot be constructed because it has no accessible initializers}}
+let _ = A()
 
 class Outer {
   class Inner : Outer {}
 }
 
-class Outer2 // expected-error {{circular reference}} expected-note {{through reference here}}
+class Outer2 // expected-error {{'Outer2' inherits from itself}} expected-note {{through reference here}}
     : Outer2.Inner { // expected-note {{through reference here}}
 
   class Inner {}
 }
 
-class Outer3 // expected-error {{circular reference}} expected-note {{through reference here}}
+class Outer3 // expected-error {{'Outer3' inherits from itself}} expected-note {{through reference here}}
     : Outer3.Inner<Int> { // expected-note {{through reference here}}
   class Inner<T> {}
 }
 
-// CHECK: ===CYCLE DETECTED===
-// CHECK-LABEL: `--{{.*}}HasCircularInheritanceRequest(circular_inheritance.(file).Left@
-// CHECK-NEXT:     `--{{.*}}SuperclassDeclRequest({{.*Left}}
-// CHECK:          `--{{.*}}InheritedDeclsReferencedRequest(circular_inheritance.(file).Left@
-// CHECK:              `--{{.*}}SuperclassDeclRequest
-// CHECK:                  `--{{.*}}InheritedDeclsReferencedRequest(circular_inheritance.(file).Right@
-// CHECK:                      `--{{.*}}SuperclassDeclRequest{{.*(cyclic dependency)}}
-
 // CHECK-DOT: digraph Dependencies
 // CHECK-DOT: label="InheritedTypeRequest
 
 protocol Initable {
   init()
-  // expected-note@-1 {{protocol requires initializer 'init()' with type '()'; do you want to add a stub?}}
-  // expected-note@-2 {{did you mean 'init'?}}
 }
 
 protocol Shape : Circle {}
 
 class Circle : Initable & Circle {}
 // expected-error@-1 {{'Circle' inherits from itself}}
-// expected-error@-2 {{type 'Circle' does not conform to protocol 'Initable'}}
+// expected-error@-2 {{initializer requirement 'init()' can only be satisfied by a 'required' initializer in non-final class 'Circle'}}
 
 func crash() {
-  Circle()
-  // expected-error@-1 {{'Circle' cannot be constructed because it has no accessible initializers}}
+  _ = Circle()
 }
 
-// FIXME: We shouldn't emit the redundant "circular reference" diagnostics here.
 class WithDesignatedInit : WithDesignatedInit {
   // expected-error@-1 {{'WithDesignatedInit' inherits from itself}}
-  // expected-error@-2 {{circular reference}}
-  // expected-note@-3 {{through reference here}}
-  // expected-note@-4 2 {{through reference here}}
 
-  init(x: Int) {} // expected-error {{circular reference}}
+  init(x: Int) {}
 }
