NCGenerics: break cycle with SuperclassTypeRequest

kavon · kavon · commit 6dc0c7c8f208 · 2024-02-13T09:16:58.000-08:00
With NoncopyableGenerics, we get a cycle involving
`SuperclassTypeRequest` with this program:

  public struct RawMarkupHeader {}
  final class RawMarkup: ManagedBuffer&lt;RawMarkupHeader, RawMarkup&gt; { }

Because we generally don't support the following kind of relationship:

  class Base&lt;T: P&gt;: P {}
  class Derived: Base&lt;Derived&gt; {}

This commit works around the root-cause, which is that Derived's
synthesized conformance to Copyable gets superceded by the inherited one
from Base. Instead of recording conformances in the ConformanceLookup
table at all, create builtin conformances on the fly, since classes
cannot be conditionally Copyable or Escapable.
diff --git a/include/swift/AST/InverseMarking.h b/include/swift/AST/InverseMarking.h
@@ -57,6 +57,11 @@ struct InverseMarking {
     }
     operator bool() const { return isPresent(); }
 
+    // Is there any kind of explicit marking?
+    bool isAnyExplicit() const {
+      return is(Kind::Explicit) || is(Kind::LegacyExplicit);
+    }
+
     SourceLoc getLoc() const { return loc; }
 
     void set(Kind k, SourceLoc l = SourceLoc()) {
diff --git a/include/swift/AST/ProtocolConformance.h b/include/swift/AST/ProtocolConformance.h
@@ -1157,6 +1157,19 @@ class BuiltinProtocolConformance final : public RootProtocolConformance {
     return getBuiltinConformanceKind() == BuiltinConformanceKind::Missing;
   }
 
+  bool isInvalid() const {
+    switch (getBuiltinConformanceKind()) {
+    case BuiltinConformanceKind::Synthesized:
+      return false;
+    case BuiltinConformanceKind::Missing:
+      return true;
+    }
+  }
+
+  SourceLoc getLoc() const {
+    return SourceLoc();
+  }
+
   /// Get any requirements that must be satisfied for this conformance to apply.
   llvm::Optional<ArrayRef<Requirement>>
   getConditionalRequirementsIfAvailable() const {
@@ -1191,6 +1204,10 @@ class BuiltinProtocolConformance final : public RootProtocolConformance {
     llvm_unreachable("builtin-conformances never have associated types");
   }
 
+  bool hasWitness(ValueDecl *requirement) const {
+    llvm_unreachable("builtin-conformances never have requirement witnesses");
+  }
+
   /// Retrieve the type witness and type decl (if one exists)
   /// for the given associated type.
   TypeWitnessAndDecl
@@ -1199,6 +1216,10 @@ class BuiltinProtocolConformance final : public RootProtocolConformance {
     llvm_unreachable("builtin-conformances never have associated types");
   }
 
+  Witness getWitness(ValueDecl *requirement) const {
+    llvm_unreachable("builtin-conformances never have requirement witnesses");
+  }
+
   /// Given that the requirement signature of the protocol directly states
   /// that the given dependent type must conform to the given protocol,
   /// return its associated conformance.
diff --git a/lib/AST/ConformanceLookup.cpp b/lib/AST/ConformanceLookup.cpp
@@ -29,6 +29,7 @@
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/GenericEnvironment.h"
+#include "swift/AST/InverseMarking.h"
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/NameLookupRequests.h"
 #include "swift/AST/PackConformance.h"
@@ -402,6 +403,41 @@ static ProtocolConformanceRef getBuiltinMetaTypeTypeConformance(
   return ProtocolConformanceRef::forMissingOrInvalid(type, protocol);
 }
 
+static ProtocolConformanceRef
+getBuiltinInvertibleProtocolConformance(NominalTypeDecl *nominal,
+                                        Type type,
+                                        ProtocolDecl *protocol) {
+  assert(isa<ClassDecl>(nominal));
+  ASTContext &ctx = protocol->getASTContext();
+
+  auto ip = protocol->getInvertibleProtocolKind();
+  switch (*ip) {
+  case InvertibleProtocolKind::Copyable:
+    // If move-only classes is enabled, we'll check the markings.
+    if (ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses)) {
+      auto marking = nominal->getMarking(*ip);
+      switch (marking.getInverse().getKind()) {
+      case InverseMarking::Kind::LegacyExplicit:
+      case InverseMarking::Kind::Explicit:
+        // An inverse ~Copyable prevents conformance.
+        return ProtocolConformanceRef::forInvalid();
+
+      case InverseMarking::Kind::Inferred: // ignore "inferred" inverse marking
+      case InverseMarking::Kind::None:
+        break;
+      }
+    }
+    break;
+  case InvertibleProtocolKind::Escapable:
+    // Always conforms.
+    break;
+  }
+
+  return ProtocolConformanceRef(
+      ctx.getBuiltinConformance(type, protocol,
+                                BuiltinConformanceKind::Synthesized));
+}
+
 /// Synthesize a builtin type conformance to the given protocol, if
 /// appropriate.
 static ProtocolConformanceRef
@@ -586,6 +622,13 @@ LookupConformanceInModuleRequest::evaluate(
   if (!nominal || isa<ProtocolDecl>(nominal))
     return ProtocolConformanceRef::forMissingOrInvalid(type, protocol);
 
+  // We specially avoid recording conformances to invertible protocols in a
+  // class's conformance table. This prevents an evaluator cycle.
+  if (ctx.LangOpts.hasFeature(Feature::NoncopyableGenerics)
+      && isa<ClassDecl>(nominal)
+      && protocol->getInvertibleProtocolKind())
+        return getBuiltinInvertibleProtocolConformance(nominal, type, protocol);
+
   // Expand conformances added by extension macros.
   //
   // FIXME: This expansion should only be done if the
diff --git a/lib/AST/ProtocolConformance.cpp b/lib/AST/ProtocolConformance.cpp
@@ -102,9 +102,10 @@ switch (getKind()) {                                                         \
     return cast<NormalProtocolConformance>(this)->Method Args;               \
   case ProtocolConformanceKind::Self:                                        \
     return cast<SelfProtocolConformance>(this)->Method Args;                 \
+  case ProtocolConformanceKind::Builtin:                                     \
+    return cast<BuiltinProtocolConformance>(this)->Method Args;              \
   case ProtocolConformanceKind::Specialized:                                 \
   case ProtocolConformanceKind::Inherited:                                   \
-  case ProtocolConformanceKind::Builtin:                                     \
     llvm_unreachable("not a root conformance");                              \
 }                                                                            \
 llvm_unreachable("bad ProtocolConformanceKind");
@@ -1090,19 +1091,21 @@ void NominalTypeDecl::prepareConformanceTable() const {
   // Synthesize the unconditional conformances to invertible protocols.
   // For conditional ones, see findSynthesizedConformances .
   if (ctx.LangOpts.hasFeature(Feature::NoncopyableGenerics)) {
-    bool missingOne = false;
-    for (auto ip : InvertibleProtocolSet::full()) {
-      auto invertible = getMarking(ip);
-      if (!invertible.getInverse() || bool(invertible.getPositive()))
-        addSynthesized(ctx.getProtocol(getKnownProtocolKind(ip)));
-      else
-        missingOne = true;
-    }
-
-    // FIXME: rdar://122289155 (NCGenerics: convert Equatable, Hashable, and RawRepresentable to ~Copyable.)
-    if (missingOne)
-      return;
+    // Classes get their conformances during ModuleDecl::lookupConformance.
+    if (!isa<ClassDecl>(this)) {
+      bool missingOne = false;
+      for (auto ip : InvertibleProtocolSet::full()) {
+        auto invertible = getMarking(ip);
+        if (!invertible.getInverse() || bool(invertible.getPositive()))
+          addSynthesized(ctx.getProtocol(getKnownProtocolKind(ip)));
+        else
+          missingOne = true;
+      }
 
+      // FIXME: rdar://122289155 (NCGenerics: convert Equatable, Hashable, and RawRepresentable to ~Copyable.)
+      if (missingOne)
+        return;
+    }
   } else if (!canBeCopyable()) {
     return; // No synthesized conformances for move-only nominals.
   }
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -3183,6 +3183,29 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     }
   }
 
+  static void diagnoseInverseOnClass(ClassDecl *decl) {
+    auto &ctx = decl->getASTContext();
+
+    for (auto ip : InvertibleProtocolSet::full()) {
+      auto marking = decl->getMarking(ip);
+
+      // Inferred inverses are already ignored for classes.
+      // FIXME: we can also diagnose @_moveOnly here if we use `isAnyExplicit`
+      if (!marking.getInverse().is(InverseMarking::Kind::Explicit))
+        continue;
+
+      // Allow ~Copyable when MoveOnlyClasses is enabled
+      if (ip == InvertibleProtocolKind::Copyable
+          && ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses))
+        continue;
+
+
+      ctx.Diags.diagnose(marking.getInverse().getLoc(),
+                         diag::inverse_on_class,
+                         getProtocolName(getKnownProtocolKind(ip)));
+    }
+  }
+
   /// check to see if a move-only type can ever conform to the given type.
   /// \returns true iff a diagnostic was emitted because it was not compatible
   static bool diagnoseIncompatibleWithMoveOnlyType(SourceLoc loc,
@@ -3415,6 +3438,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
 
     diagnoseIncompatibleProtocolsForMoveOnlyType(CD);
 
+    diagnoseInverseOnClass(CD);
   }
 
   void visitProtocolDecl(ProtocolDecl *PD) {
diff --git a/lib/Sema/TypeCheckInvertible.cpp b/lib/Sema/TypeCheckInvertible.cpp
@@ -171,20 +171,14 @@ static bool checkInvertibleConformanceCommon(ProtocolConformance *conformance,
   auto &ctx = nom->getASTContext();
   bool conforms = true;
 
-  // An explicit `~IP` prevents conformance if any of these are true:
+  // An explicit `~IP` prevents conformance if it appears on the same
+  // declaration that also declares the conformance.
   //
-  // 1. It appears on a class.
-  // 2. Appears on the same declaration that also declares the conformance.
-  //    So, if the nominal has `~Copyable` but this conformance is
-  //    written in an extension, then we do not raise an error.
+  // So, if the nominal has `~Copyable` but this conformance is
+  // written in an extension, then we do not raise an error.
   auto marking = nom->getMarking(ip);
-  if (marking.getInverse().getKind() == InverseMarking::Kind::Explicit) {
-    if (isa<ClassDecl>(nom)) {
-      ctx.Diags.diagnose(marking.getInverse().getLoc(),
-                         diag::inverse_on_class,
-                         getProtocolName(kp));
-      conforms &= false;
-    } else if (conformance->getDeclContext() == nom) {
+  if (marking.getInverse().isAnyExplicit()) {
+    if (conformance->getDeclContext() == nom) {
       ctx.Diags.diagnose(marking.getInverse().getLoc(),
                          diag::inverse_but_also_conforms,
                          nom, getProtocolName(kp));
@@ -198,7 +192,7 @@ static bool checkInvertibleConformanceCommon(ProtocolConformance *conformance,
 
   // Protocols do not directly define any storage.
   if (isa<ProtocolDecl, BuiltinTupleDecl>(nom))
-    llvm_unreachable("unexpected nominal to check Copyable conformance");
+    llvm_unreachable("unexpected nominal to check invertible's conformance");
 
   // A deinit prevents a struct or enum from conforming to Copyable.
   if (ip == InvertibleProtocolKind::Copyable) {
@@ -343,6 +337,7 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator,
   if (!ip)
     llvm_unreachable("not an invertible protocol");
 
+  assert(!isa<ClassDecl>(nominal) && "classes aren't handled here");
   auto file = cast<FileUnit>(nominal->getModuleScopeContext());
 
   // Generates a conformance for the nominal to the protocol.
@@ -403,20 +398,6 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator,
     return generateConformance(ext);
   };
 
-  switch (*ip) {
-  case InvertibleProtocolKind::Copyable:
-    // If move-only classes is enabled, we'll check the markings.
-    if (ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses))
-      break;
-
-  LLVM_FALLTHROUGH;
-  case InvertibleProtocolKind::Escapable:
-    // Always derive unconditional IP conformance for classes
-    if (isa<ClassDecl>(nominal))
-      return generateConformance(nominal);
-    break;
-  }
-
   auto marking = nominal->getMarking(*ip);
 
   // Unexpected to have any positive marking for IP if we're deriving it.
@@ -430,10 +411,8 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator,
     return nullptr; // No positive IP conformance will be inferred.
 
   case InverseMarking::Kind::Inferred:
-    if (!isa<ClassDecl>(nominal))
-      return generateConditionalConformance();
+    return generateConditionalConformance();
 
-  LLVM_FALLTHROUGH;
   case InverseMarking::Kind::None:
     // All types already start with conformances to the invertible protocols in
     // this case, within `NominalTypeDecl::prepareConformanceTable`.
diff --git a/test/Generics/inverse_generics.swift b/test/Generics/inverse_generics.swift
@@ -169,7 +169,12 @@ protocol NeedsCopyable {}
 
 struct Silly: ~Copyable, Copyable {} // expected-error {{struct 'Silly' required to be 'Copyable' but is marked with '~Copyable'}}
 enum Sally: Copyable, ~Copyable, NeedsCopyable {} // expected-error {{enum 'Sally' required to be 'Copyable' but is marked with '~Copyable'}}
+
 class NiceTry: ~Copyable, Copyable {} // expected-error {{classes cannot be '~Copyable'}}
+                                      // expected-error@-1 {{class 'NiceTry' required to be 'Copyable' but is marked with '~Copyable}}
+
+@_moveOnly class NiceTry2: Copyable {} // expected-error {{'@_moveOnly' attribute is only valid on structs or enums}}
+                                       // expected-error@-1 {{class 'NiceTry2' required to be 'Copyable' but is marked with '~Copyable'}}
 
 struct OopsConformance1: ~Copyable, NeedsCopyable {}
 // expected-error@-1 {{type 'OopsConformance1' does not conform to protocol 'NeedsCopyable'}}
