[Sema] require value deinit absence for Copyable

A value type like a struct or enum cannot conform to Copyable if it has
a deinit. When NoncopyableGenerics is enabled, we make that part of what
 is required to verify that such a nominal type conforms to Copyable.

This change also does some refactoring to share common code to point out
 how to make a type noncopyable.

Author: kavon

include/swift/AST/DiagnosticsParse.def

@@ -429,8 +429,6 @@ ERROR(destructor_decl_outside_class_or_noncopyable,none,
       "deinitializers may only be declared within a class, actor, or noncopyable type", ())
 ERROR(destructor_decl_on_noncopyable_enum,none,
       "deinitializers are not yet supported on noncopyable enums", ())
-ERROR(destructor_decl_on_objc_enum,none,
-      "deinitializers cannot be declared on an @objc enum type", ())
 ERROR(expected_lbrace_destructor,PointsToFirstBadToken,
       "expected '{' for deinitializer", ())
 ERROR(destructor_has_name,PointsToFirstBadToken,

include/swift/AST/DiagnosticsSema.def

@@ -7539,6 +7539,9 @@ ERROR(noncopyable_but_copyable, none,
 ERROR(noncopyable_class, none,
       "classes cannot be noncopyable",
       ())
+ERROR(copyable_illegal_deinit, none,
+      "deinitializer cannot be declared in %kind0 that conforms to 'Copyable'",
+      (const ValueDecl *))
 ERROR(noncopyable_type_member_in_copyable,none,
       "%select{stored property %2|associated value %2}1 of "
       "'Copyable'-conforming %kind3 has noncopyable type %0",

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -4071,33 +4071,33 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
   }
 
   void visitDestructorDecl(DestructorDecl *DD) {
+    auto haveFeature = [=](Feature f) -> bool {
+      return DD->getASTContext().LangOpts.hasFeature(f);
+    };
+
     // Only check again for destructor decl outside of a class if our destructor
     // is not marked as invalid.
     if (!DD->isInvalid()) {
       auto *nom = dyn_cast<NominalTypeDecl>(
                              DD->getDeclContext()->getImplementedObjCContext());
-      if (!nom || (!isa<ClassDecl>(nom) && !nom->canBeNoncopyable())) {
+      if (!nom || isa<ProtocolDecl>(nom)) {
+        DD->diagnose(diag::destructor_decl_outside_class_or_noncopyable);
+
+      } else if (!haveFeature(Feature::NoncopyableGenerics)
+                  && !isa<ClassDecl>(nom)
+                  && !nom->canBeNoncopyable()) {
+        // When we have NoncopyableGenerics, deinits get validated as part of
+        // Copyable-conformance checking.
         DD->diagnose(diag::destructor_decl_outside_class_or_noncopyable);
       }
 
       // Temporarily ban deinit on noncopyable enums, unless the experimental
       // feature flag is set.
-      if (!DD->getASTContext().LangOpts.hasFeature(
-              Feature::MoveOnlyEnumDeinits) &&
-          nom->canBeNoncopyable() && isa<EnumDecl>(nom)) {
+      if (!haveFeature(Feature::MoveOnlyEnumDeinits)
+          && isa<EnumDecl>(nom)
+          && nom->canBeNoncopyable()) {
         DD->diagnose(diag::destructor_decl_on_noncopyable_enum);
       }
-
-      // If we have a noncopyable type, check if we have an @objc enum with a
-      // deinit and emit a specialized error. We will have technically already
-      // emitted an error since @objc enum cannot be marked noncopyable, but
-      // this at least makes it a bit clearer to the user that the deinit is
-      // also incorrect.
-      if (auto *e = dyn_cast_or_null<EnumDecl>(nom)) {
-        if (e->isObjC()) {
-          DD->diagnose(diag::destructor_decl_on_objc_enum);
-        }
-      }
     }
 
     TypeChecker::checkDeclAttributes(DD);

lib/Sema/TypeCheckInvertible.cpp

@@ -47,39 +47,65 @@ static void addConformanceFixIt(const NominalTypeDecl *nominal,
   }
 }
 
-/// 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(NominalTypeDecl *enclosingNom,
-                                     Type nonConformingTy,
-                                     KnownProtocolKind kp) {
-  auto *module = enclosingNom->getParentModule();
-  auto &ctx = enclosingNom->getASTContext();
+// 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,
+                                          KnownProtocolKind kp,
+                                          InverseMarking marking,
+                                          NominalTypeDecl *nominal) {
+  // Immediately flush, then emit notes, so they're associated.
+  diag.flush();
 
-  // Check the enclosing type's markings to see what to suggest.
-  assert(kp == KnownProtocolKind::Copyable);
-  auto enclosingMarking = enclosingNom->getNoncopyableMarking();
+  auto &ctx = nominal->getASTContext();
+  // Not expecting the positive KP constraint to be classified as "Inferred".
+  assert(marking.getPositive().getKind() != InverseMarking::Kind::Inferred);
 
-  switch (enclosingMarking.getInverse().getKind()) {
+  // Have no advice for situations where the KP conformance is explicit.
+  if (marking.getPositive().isPresent())
+    return;
+
+  switch (marking.getInverse().getKind()) {
   case InverseMarking::Kind::Inferred:
     // Note that the enclosing type is conditionally conforming to KP first.
-    ctx.Diags.diagnose(enclosingMarking.getInverse().getLoc(),
+    ctx.Diags.diagnose(marking.getInverse().getLoc(),
                        diag::note_inverse_preventing_conformance_implicit,
-                       enclosingNom, getProtocolName(kp));
+                       nominal, getProtocolName(kp));
     LLVM_FALLTHROUGH;
   case InverseMarking::Kind::None: {
     // Suggest adding ~KP to make it non-KP.
-    auto diag = enclosingNom->diagnose(diag::add_inverse,
-                                       enclosingNom,
+    auto diag = nominal->diagnose(diag::add_inverse,
+                                       nominal,
                                        getProtocolName(kp));
-    addConformanceFixIt(enclosingNom, diag, kp, /*inverse=*/true);
+    addConformanceFixIt(nominal, diag, kp, /*inverse=*/true);
   }
     break;
   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,
+                                     Type nonConformingTy,
+                                     KnownProtocolKind kp) {
+  auto *module = enclosingNom->getParentModule();
+  auto &ctx = enclosingNom->getASTContext();
+
+  // Check the enclosing type's markings to see what to suggest.
+  assert(kp == KnownProtocolKind::Copyable);
+  auto enclosingMarking = enclosingNom->getNoncopyableMarking();
+
+  // First, the generic advice.
+  emitAdviceToApplyInverseAfter(std::move(diag), kp,
+                                enclosingMarking, 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.
@@ -174,8 +200,16 @@ bool checkCopyableConformance(ProtocolConformance *conformance) {
   if (isa<BuiltinTupleDecl>(nom))
     llvm_unreachable("TODO: BuiltinTupleDecl");
 
-  // NOTE: A deinit prevents a struct or enum from conforming to Copyable, but
-  // we will emit an error for that elsewhere already.
+  // A deinit prevents a struct or enum from conforming to Copyable.
+  if (auto *deinit = nom->getValueTypeDestructor()) {
+    auto diag = deinit->diagnose(diag::copyable_illegal_deinit, nom);
+    emitAdviceToApplyInverseAfter(std::move(diag),
+                                  KnownProtocolKind::Copyable,
+                                  nom->getNoncopyableMarking(),
+                                  nom);
+    return false;
+  }
+
 
   // Otherwise, we have to check its storage to ensure it is all Copyable.
 
@@ -200,10 +234,11 @@ bool checkCopyableConformance(ProtocolConformance *conformance) {
       if (!Diagnosing)
         return true; // it's noncopyable
 
-      storage->diagnose(diag::noncopyable_type_member_in_copyable, type,
-                        isEnum, storage->getName(), Nominal);
+      auto diag = storage->diagnose(diag::noncopyable_type_member_in_copyable,
+                                    type, isEnum, storage->getName(), Nominal);
 
-      tryEmitContainmentFixits(Nominal, type, KnownProtocolKind::Copyable);
+      tryEmitContainmentFixits(std::move(diag),
+                               Nominal, type, KnownProtocolKind::Copyable);
       return true;
     }
 

test/Generics/inverse_copyable_generics.swift

@@ -46,17 +46,12 @@ protocol RemovedAgain where Self: ~Copyable {
 }
 
 struct StructContainment<T: ~Copyable> : Copyable {
-    // expected-note@-1 {{consider adding '~Copyable' to generic struct 'StructContainment'}}{{50-50=, ~Copyable}}
-    // expected-note@-2 {{generic struct 'StructContainment' has '~Copyable' constraint on a generic parameter, making its 'Copyable' conformance conditional}}
-
     var storage: Maybe<T>
     // expected-error@-1 {{stored property 'storage' of 'Copyable'-conforming generic struct 'StructContainment' has noncopyable type 'Maybe<T>'}}
 }
 
 enum EnumContainment<T: ~Copyable> : Copyable {
     // expected-note@-1 {{'T' has '~Copyable' constraint preventing implicit 'Copyable' conformance}}
-    // expected-note@-2{{consider adding '~Copyable' to generic enum 'EnumContainment'}}{{46-46=, ~Copyable}}
-    // expected-note@-3{{generic enum 'EnumContainment' has '~Copyable' constraint on a generic parameter, making its 'Copyable' conformance conditional}}
 
     case some(T) // expected-error {{associated value 'some' of 'Copyable'-conforming generic enum 'EnumContainment' has noncopyable type 'T'}}
     case other(Int)
@@ -69,6 +64,8 @@ class ClassContainment<T: ~Copyable> {
         storage = t
         checkCopyable(t) // expected-error {{noncopyable type 'T' cannot be substituted for copyable generic parameter 'T' in 'checkCopyable'}}
     }
+
+    deinit {}
 }
 
 // expected-note@+2 {{generic struct 'ConditionalContainment' has '~Copyable' constraint on a generic parameter, making its 'Copyable' conformance conditional}}
@@ -83,20 +80,44 @@ func chk(_ T: RequireCopyable<ConditionalContainment<Int>>) {}
 
 /// ----------------
 
-// expected-note@+1 {{generic enum 'Maybe' has '~Copyable' constraint on a generic parameter, making its 'Copyable' conformance conditional}}
+struct AlwaysCopyableDeinit<T: ~Copyable> : Copyable {
+  deinit {} // expected-error {{deinitializer cannot be declared in generic struct 'AlwaysCopyableDeinit' that conforms to 'Copyable'}}
+}
+
+struct SometimesCopyableDeinit<T: ~Copyable> : ~Copyable {
+  deinit {} // expected-error {{deinitializer cannot be declared in generic struct 'SometimesCopyableDeinit' that conforms to 'Copyable'}}
+}
+extension SometimesCopyableDeinit: Copyable where T: Copyable {}
+
+struct NeverCopyableDeinit<T: ~Copyable>: ~Copyable {
+  deinit {}
+}
+
+/// ---------------
+
+// expected-note@+2 {{consider adding '~Copyable' to generic enum 'Maybe'}}
+// expected-note@+1 2{{generic enum 'Maybe' has '~Copyable' constraint on a generic parameter, making its 'Copyable' conformance conditional}}
 enum Maybe<Wrapped: ~Copyable> {
   case just(Wrapped)
   case none
+
+  deinit {} // expected-error {{deinitializer cannot be declared in generic enum 'Maybe' that conforms to 'Copyable'}}
+  // expected-error@-1 {{deinitializers are not yet supported on noncopyable enums}}
 }
 
-struct RequireCopyable<T> {}
-// expected-note@-1{{requirement specified as 'NC' : 'Copyable'}}
-// expected-note@-2{{requirement from conditional conformance of 'Maybe<NC>' to 'Copyable'}}
-// expected-note@-3{{requirement specified as 'Wrapped' : 'Copyable'}}
-// expected-note@-4{{requirement from conditional conformance of 'Maybe<Wrapped>' to 'Copyable'}}
+// expected-note@+4{{requirement specified as 'NC' : 'Copyable'}}
+// expected-note@+3{{requirement from conditional conformance of 'Maybe<NC>' to 'Copyable'}}
+// expected-note@+2{{requirement specified as 'Wrapped' : 'Copyable'}}
+// expected-note@+1{{requirement from conditional conformance of 'Maybe<Wrapped>' to 'Copyable'}}
+struct RequireCopyable<T> {
+  // expected-note@-1 {{consider adding '~Copyable' to generic struct 'RequireCopyable'}}{{27-27=: ~Copyable}}
+  deinit {} // expected-error {{deinitializer cannot be declared in generic struct 'RequireCopyable' that conforms to 'Copyable'}}
+}
 
-struct NC: ~Copyable {}
+struct NC: ~Copyable {
 // expected-note@-1 2{{struct 'NC' has '~Copyable' constraint preventing 'Copyable' conformance}}
+  deinit {}
+}
 
 typealias ok1 = RequireCopyable<Int>
 typealias ok2 = RequireCopyable<Maybe<Int>>

test/Sema/moveonly_objc_enum.swift

@@ -8,7 +8,7 @@
 @objc enum Foo : Int { // expected-error {{noncopyable enums cannot be marked '@objc'}}
                        // expected-error@-1 {{'Foo' declares raw type 'Int', but cannot yet conform to RawRepresentable because it is noncopyable}}
   case X, Y, Z
-  deinit {} // expected-error {{deinitializers cannot be declared on an @objc enum type}}
+  deinit {}
 }
 
 @_moveOnly