Sema: Remove a usage of hasInverseMarking()

slavapestov · slavapestov · commit 708586baea88 · 2024-03-07T15:07:48.000-05:00
This addresses some duplicate diagnostics from the previous commit.
diff --git a/lib/Sema/TypeCheckInvertible.cpp b/lib/Sema/TypeCheckInvertible.cpp
@@ -19,7 +19,6 @@
 #include "TypeCheckInvertible.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/GenericEnvironment.h"
-#include "swift/AST/InverseMarking.h"
 #include "TypeChecker.h"
 
 using namespace swift;
@@ -50,63 +49,34 @@ static void addConformanceFixIt(const NominalTypeDecl *nominal,
 // If there is not already an inverse ~KP applied to this type, suggest it.
 // The goal here is that we want to tell users how they can suppress or remove
 // a conformance to KP.
-static void emitAdviceToApplyInverseAfter(InFlightDiagnostic &&diag,
-                                          InvertibleProtocolKind ip,
-                                          InverseMarking::Mark inverseMarking,
+static void emitAdviceToApplyInverseAfter(InvertibleProtocolKind ip,
+                                          bool canAddInverse,
                                           NominalTypeDecl *nominal) {
   auto kp = getKnownProtocolKind(ip);
 
-  // Immediately flush, then emit notes, so they're associated.
-  diag.flush();
-
-  // Have no advice for situations where the KP conformance is explicit.
-  InvertibleProtocolSet inverses;
-  bool anyObject = false;
-  auto inheritedNominals = getDirectlyInheritedNominalTypeDecls(
-      nominal, inverses, anyObject);
-  for (auto entry : inheritedNominals) {
-    if (auto *otherProto = dyn_cast<ProtocolDecl>(entry.Item)) {
-      if (otherProto->isSpecificProtocol(kp))
-        return;
-    }
-  }
-
-  switch (inverseMarking.getKind()) {
-  case InverseMarking::Kind::None: {
-    // Suggest adding ~KP to make it non-KP.
+  if (canAddInverse) {
     auto diag = nominal->diagnose(diag::add_inverse,
-                                       nominal,
-                                       getProtocolName(kp));
+                                  nominal,
+                                  getProtocolName(kp));
     addConformanceFixIt(nominal, diag, kp, /*inverse=*/true);
   }
-    break;
-  case InverseMarking::Kind::LegacyExplicit:
-  case InverseMarking::Kind::Explicit:
-    // FIXME: we can probably do better here. Look for the extension where the
-    // inverse came from.
-    break;
-  }
 }
 
 /// Emit fix-it's to help the user resolve a containment issue where the
 /// \c nonConformingTy needs to be made to conform to \c kp to resolve a
 /// containment issue.
 /// \param enclosingNom is the nominal type containing a nonconforming value
 /// \param nonConformingTy is the type of the nonconforming value
-static void tryEmitContainmentFixits(InFlightDiagnostic &&diag,
-                                     NominalTypeDecl *enclosingNom,
+static void tryEmitContainmentFixits(NominalTypeDecl *enclosingNom,
+                                     bool canAddInverse,
                                      Type nonConformingTy,
                                      InvertibleProtocolKind ip) {
   auto *module = enclosingNom->getParentModule();
   auto &ctx = enclosingNom->getASTContext();
   auto kp = getKnownProtocolKind(ip);
 
-  // Check the enclosing type's markings to see what to suggest.
-  auto enclosingMarking = enclosingNom->hasInverseMarking(ip);
-
   // First, the generic advice.
-  emitAdviceToApplyInverseAfter(std::move(diag), ip,
-                                enclosingMarking, enclosingNom);
+  emitAdviceToApplyInverseAfter(ip, canAddInverse, enclosingNom);
 
   // If it's a generic parameter defined in the same module, point to the
   // parameter that must have had the inverse applied to it somewhere.
@@ -129,90 +99,84 @@ static void tryEmitContainmentFixits(InFlightDiagnostic &&diag,
   // not IP.
   if (auto nominal = nonConformingTy->getAnyNominal()) {
     if (nominal->getLoc(/*SerializedOK=*/false)) {
-      auto inverse = nominal->hasInverseMarking(ip);
-      auto loc = inverse.getLoc();
-
-      switch (inverse.getKind()) {
-      case InverseMarking::Kind::None:
-        assert(false && "how did it become noncopyable/nonescapable then?");
-        break;
-      case InverseMarking::Kind::LegacyExplicit:
-      case InverseMarking::Kind::Explicit:
-        assert(loc);
-        ctx.Diags.diagnose(loc,
-                           diag::note_inverse_preventing_conformance_explicit,
-                           nominal, getProtocolName(kp));
-        break;
-      }
+      ctx.Diags.diagnose(nominal->getLoc(),
+                         diag::note_inverse_preventing_conformance_explicit,
+                         nominal, getProtocolName(kp));
     }
   }
 }
 
 /// MARK: conformance checking
-static bool checkInvertibleConformanceCommon(DeclContext *dc,
+static void checkInvertibleConformanceCommon(DeclContext *dc,
                                              ProtocolConformance *conformance,
                                              InvertibleProtocolKind ip) {
   const auto kp = getKnownProtocolKind(ip);
   auto *proto = conformance->getProtocol();
   assert(proto->isSpecificProtocol(kp));
 
-  auto *nom = conformance->getType()->getAnyNominal();
-  assert(nom && "non-nominal with conformance?");
-  if (!nom)
-    return false;
+  auto *nominalDecl = dc->getSelfNominalTypeDecl();
+  assert(isa<StructDecl>(nominalDecl) ||
+         isa<EnumDecl>(nominalDecl) ||
+         isa<ClassDecl>(nominalDecl));
 
-  auto &ctx = nom->getASTContext();
-  bool conforms = true;
-
-  // An explicit `~IP` prevents conformance if it 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.
-  auto inverseMarking = nom->hasInverseMarking(ip);
-  if (inverseMarking.isAnyExplicit()) {
-    if (dc == nom) {
-      ctx.Diags.diagnose(inverseMarking.getLoc(),
+  auto &ctx = nominalDecl->getASTContext();
+
+  InvertibleProtocolSet inverses;
+  bool anyObject = false;
+  (void) getDirectlyInheritedNominalTypeDecls(nominalDecl, inverses, anyObject);
+
+  // Handle deprecated attributes.
+  if (nominalDecl->getAttrs().hasAttribute<MoveOnlyAttr>())
+    inverses.insert(InvertibleProtocolKind::Copyable);
+  if (nominalDecl->getAttrs().hasAttribute<NonEscapableAttr>())
+    inverses.insert(InvertibleProtocolKind::Escapable);
+
+  bool hasExplicitInverse = inverses.contains(ip);
+
+  bool hasUnconditionalConformance = false;
+  auto *normalConf = dyn_cast<NormalProtocolConformance>(conformance);
+  if (normalConf && normalConf->getConditionalRequirements().empty())
+    hasUnconditionalConformance = true;
+
+  if (!isa<ClassDecl>(nominalDecl) ||
+      ctx.LangOpts.hasFeature(Feature::MoveOnlyClasses)) {
+    // If the inheritance clause contains ~Copyable, reject an unconditional
+    // conformance to Copyable.
+    if (hasExplicitInverse && hasUnconditionalConformance) {
+      ctx.Diags.diagnose(normalConf->getLoc(),
                          diag::inverse_but_also_conforms,
-                         nom, getProtocolName(kp));
-      conforms &= false;
+                         nominalDecl, getProtocolName(kp));
     }
   }
 
   // All classes can store noncopyable/nonescaping values.
-  if (isa<ClassDecl>(nom))
-    return conforms;
+  if (isa<ClassDecl>(nominalDecl))
+    return;
 
-  // Protocols do not directly define any storage.
-  if (isa<ProtocolDecl, BuiltinTupleDecl>(nom))
-    llvm_unreachable("unexpected nominal to check invertible's conformance");
+  bool canAddInverse = !hasExplicitInverse && !hasUnconditionalConformance;
 
   // A deinit prevents a struct or enum from conforming to Copyable.
   if (ip == InvertibleProtocolKind::Copyable) {
-    if (auto *deinit = nom->getValueTypeDestructor()) {
-      auto diag = deinit->diagnose(diag::copyable_illegal_deinit, nom);
-      emitAdviceToApplyInverseAfter(std::move(diag),
-                                    ip,
-                                    inverseMarking,
-                                    nom);
-      conforms &= false;
+    if (auto *deinit = nominalDecl->getValueTypeDestructor()) {
+      deinit->diagnose(diag::copyable_illegal_deinit, nominalDecl);
+      emitAdviceToApplyInverseAfter(ip, canAddInverse, nominalDecl);
     }
   }
 
-  // Otherwise, we have to check its storage to ensure it is all
-  // Copyable/Escapable.
+  // Check storage for conformance to Copyable/Escapable.
 
   class LacksMatchingStorage: public StorageVisitor {
     NominalTypeDecl *Nominal;
     DeclContext *DC;
     InvertibleProtocolKind IP;
-    bool Diagnosing;
+    bool CanAddInverse;
   public:
     LacksMatchingStorage(NominalTypeDecl *nom,
                          DeclContext *dc,
-                         InvertibleProtocolKind ip,
-                         bool diagnose)
-        : Nominal(nom), DC(dc), IP(ip), Diagnosing(diagnose) {}
+                         bool canAddInverse,
+                         InvertibleProtocolKind ip)
+        : Nominal(nom), DC(dc), IP(ip),
+          CanAddInverse(canAddInverse) {}
 
     bool visit() { return StorageVisitor::visit(Nominal, DC); }
 
@@ -233,15 +197,11 @@ static bool checkInvertibleConformanceCommon(DeclContext *dc,
         break;
       }
 
-      if (!Diagnosing)
-        return true; // it's got storage missing conformance to IP
+      storage->diagnose(diag::inverse_type_member_in_conforming_type,
+                        type, isEnum, storage->getName(), Nominal,
+                        getProtocolName(getKnownProtocolKind(IP)));
 
-      auto diag =
-          storage->diagnose(diag::inverse_type_member_in_conforming_type,
-                            type, isEnum, storage->getName(), Nominal,
-                            getProtocolName(getKnownProtocolKind(IP)));
-
-      tryEmitContainmentFixits(std::move(diag), Nominal, type, IP);
+      tryEmitContainmentFixits(Nominal, CanAddInverse, type, IP);
       return true;
     }
 
@@ -260,23 +220,19 @@ static bool checkInvertibleConformanceCommon(DeclContext *dc,
 
   // This nominal cannot conform to IP if it contains storage that does not
   // conform to IP.
-  bool lacksMatchingStorage =
-      LacksMatchingStorage(nom, dc, ip, /*diagnose=*/true).visit();
-  conforms &= !lacksMatchingStorage;
-
-  return conforms;
+  LacksMatchingStorage(nominalDecl, dc, canAddInverse, ip).visit();
 }
 
-bool swift::checkEscapableConformance(DeclContext *dc,
+void swift::checkEscapableConformance(DeclContext *dc,
                                       ProtocolConformance *conformance) {
-  return checkInvertibleConformanceCommon(dc, conformance,
-                                          InvertibleProtocolKind::Escapable);
+  checkInvertibleConformanceCommon(dc, conformance,
+                                   InvertibleProtocolKind::Escapable);
 }
 
-bool swift::checkCopyableConformance(DeclContext *dc,
+void swift::checkCopyableConformance(DeclContext *dc,
                                      ProtocolConformance *conformance) {
-  return checkInvertibleConformanceCommon(dc, conformance,
-                                          InvertibleProtocolKind::Copyable);
+  checkInvertibleConformanceCommon(dc, conformance,
+                                   InvertibleProtocolKind::Copyable);
 }
 
 /// Visit the instance storage of the given nominal type as seen through
diff --git a/lib/Sema/TypeCheckInvertible.h b/lib/Sema/TypeCheckInvertible.h
@@ -32,11 +32,11 @@ class StorageVisitor {
 };
 
 /// Checks that all stored properties or associated values are Copyable.
-bool checkCopyableConformance(DeclContext *dc,
+void checkCopyableConformance(DeclContext *dc,
                               ProtocolConformance *conformance);
 
 /// Checks that all stored properties or associated values are Escapable.
-bool checkEscapableConformance(DeclContext *dc,
+void checkEscapableConformance(DeclContext *dc,
                                ProtocolConformance *conformance);
 }
 
diff --git a/test/Generics/inverse_generics.swift b/test/Generics/inverse_generics.swift
@@ -166,14 +166,13 @@ struct Silly: ~Copyable, Copyable {} // expected-error {{struct 'Silly' required
 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 Extendo: ~Copyable {}
 extension Extendo: Copyable, ~Copyable {} // expected-error {{cannot apply inverse '~Copyable' to extension}}
+// expected-error@-1 {{struct 'Extendo' required to be 'Copyable' but is marked with '~Copyable'}}
 
 enum EnumExtendo {}
 extension EnumExtendo: ~Copyable {} // expected-error {{cannot apply inverse '~Copyable' to extension}}
