[Sema] no Copyable conformance if marked ~Copyable

Much like when the type defines a deinit, if the nominal's declaration
is marked with ~Copyable, that prevents conformance to Copyable.

This meant that when trying to auto-derive Copyable conformance, we'll
skip types marked with ~Copyable. But now, we'll also reject
explicitly-requested conformances to Copyable because of that marking
too.

In reality right now, all marker protocols have conformances that are
always trivally checked and marked "complete", so when I refer to
rejecting a conformance here, I mean raising a diagnostic while checking
 for validity.

Author: kavon

Diff for: include/swift/AST/DiagnosticsSema.def

@@ -7539,6 +7539,9 @@ ERROR(noncopyable_but_copyable, none,
 ERROR(noncopyable_class, none,
       "classes cannot be noncopyable",
       ())
+ERROR(inverse_extension, none,
+      "cannot apply inverse %0 to extension",
+      (Type))
 ERROR(copyable_illegal_deinit, none,
       "deinitializer cannot be declared in %kind0 that conforms to 'Copyable'",
       (const ValueDecl *))

Diff for: lib/Sema/TypeCheckDecl.cpp

@@ -963,7 +963,7 @@ NoncopyableAnnotationRequest::evaluate(Evaluator &evaluator,
     return false;
   };
 
-  auto isInverseTarget = [](Type t) -> bool {
+  auto isInverseTarget = [&](Type t) -> bool {
     if (auto inverse = t->getAs<InverseType>())
       return inverse->getInverseKind() == TARGET;
     else if (auto pct = t->getAs<ProtocolCompositionType>())
@@ -1038,7 +1038,7 @@ NoncopyableAnnotationRequest::evaluate(Evaluator &evaluator,
         // Try to find a good location.
         SourceLoc loc;
         if (repr && !repr->isInvalid())
-          if (auto *constraintRepr = repr->getSecondTypeRepr())
+          if (auto *constraintRepr = repr->getConstraintRepr())
             if (!repr->isInvalid())
               loc = constraintRepr->getLoc();
 
@@ -1057,10 +1057,6 @@ NoncopyableAnnotationRequest::evaluate(Evaluator &evaluator,
   auto nominal = cast<NominalTypeDecl>(decl);
   assert(!isa<BuiltinTupleDecl>(nominal));
 
-  // Classes can't be noncopyable; it's diagnosed elsewhere.
-  if (isa<ClassDecl>(nominal))
-    return InverseMarking::forInverse(Kind::None);
-
   /// Protocols are handled specially since they do not infer an inverse based
   /// on their associated types.
   if (auto proto = dyn_cast<ProtocolDecl>(nominal)) {

Diff for: lib/Sema/TypeCheckDeclPrimary.cpp

@@ -205,6 +205,10 @@ static void checkInheritanceClause(
     if (inheritedTy->isConstraintType()) {
       auto layout = inheritedTy->getExistentialLayout();
 
+      // An inverse on an extension is an error.
+      if (isa<ExtensionDecl>(decl) && inheritedTy->is<InverseType>())
+        decl->diagnose(diag::inverse_extension, inheritedTy);
+
       // Subclass existentials are not allowed except on classes and
       // non-@objc protocols.
       if (layout.explicitSuperclass &&
@@ -227,12 +231,6 @@ static void checkInheritanceClause(
         continue;
       }
 
-      // Classes are always copyable.
-      if (layout.hasInverseCopyable && isa<ClassDecl>(decl)) {
-        decl->diagnose(diag::noncopyable_class);
-        continue;
-      }
-
       // If the existential did not have a class constraint, we're done.
       if (!layout.explicitSuperclass)
         continue;

Diff for: lib/Sema/TypeCheckInvertible.cpp

@@ -189,16 +189,36 @@ bool checkCopyableConformance(ProtocolConformance *conformance) {
   if (!nom)
     return false;
 
+  auto &ctx = nom->getASTContext();
+  bool conforms = true;
+
+  // An explicit `~Copyable` prevents conformance if any of these are true:
+  //
+  // 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.
+  auto marking = nom->getNoncopyableMarking();
+  if (marking.getInverse().getKind() == InverseMarking::Kind::Explicit) {
+    if (isa<ClassDecl>(nom)) {
+      ctx.Diags.diagnose(marking.getInverse().getLoc(),
+                         diag::noncopyable_class);
+      conforms &= false;
+    } else if (conformance->getDeclContext() == nom) {
+      ctx.Diags.diagnose(marking.getInverse().getLoc(),
+                         diag::noncopyable_but_copyable,
+                         nom);
+      conforms &= false;
+    }
+  }
+
   // All classes can store noncopyable values.
   if (isa<ClassDecl>(nom))
-    return true;
+    return conforms;
 
   // Protocols do not directly define any storage.
-  if (isa<ProtocolDecl>(nom))
-    return true;
-
-  if (isa<BuiltinTupleDecl>(nom))
-    llvm_unreachable("TODO: BuiltinTupleDecl");
+  if (isa<ProtocolDecl, BuiltinTupleDecl>(nom))
+    llvm_unreachable("unexpected nominal to check Copyable conformance");
 
   // A deinit prevents a struct or enum from conforming to Copyable.
   if (auto *deinit = nom->getValueTypeDestructor()) {
@@ -207,7 +227,7 @@ bool checkCopyableConformance(ProtocolConformance *conformance) {
                                   KnownProtocolKind::Copyable,
                                   nom->getNoncopyableMarking(),
                                   nom);
-    return false;
+    conforms &= false;
   }
 
 
@@ -256,8 +276,12 @@ bool checkCopyableConformance(ProtocolConformance *conformance) {
   };
 
   // This nominal cannot be Copyable if it contains noncopyable storage.
-  return !HasNoncopyable(nom, conformance->getDeclContext(),
+  bool haveNoncopyableStorage =
+      HasNoncopyable(nom, conformance->getDeclContext(),
                          /*diagnose=*/true).visit();
+  conforms &= !haveNoncopyableStorage;
+
+  return conforms;
 }
 
 /// Visit the instance storage of the given nominal type as seen through
@@ -372,23 +396,14 @@ ProtocolConformance *deriveConformanceForInvertible(Evaluator &evaluator,
 
   switch (*ip) {
   case InvertibleProtocolKind::Copyable: {
-    auto marking = nominal->getNoncopyableMarking();
-
-    // An explicit Copyable takes precedece over any ~Copyable marking.
-    if (marking.getPositive().getKind() == InverseMarking::Kind::Explicit) {
-      // If they also explicitly wrote ~Copyable, then diagnose that.
-      auto inverse = marking.getInverse();
-      if (inverse.getKind() == InverseMarking::Kind::Explicit) {
-        ctx.Diags.diagnose(inverse.getLoc(),
-                           diag::noncopyable_but_copyable,
-                           nominal);
-      }
-
+    // Always derive unconditional Copyable conformance for classes
+    if (isa<ClassDecl>(nominal))
       return generateConformance(nominal);
-    }
 
-    // Unexpected to have Kind::Inferred marking for Copyable; it's assumed.
-    assert(marking.getPositive().getKind() == InverseMarking::Kind::None);
+    auto marking = nominal->getNoncopyableMarking();
+
+    // Unexpected to have any marking for Copyable if we're deriving it.
+    assert(!marking.getPositive().isPresent());
 
     // Check what kind of inverse we have to determine whether to generate a
     // conformance for Copyable.

Diff for: test/Generics/inverse_copyable_generics.swift

@@ -81,6 +81,7 @@ func chk(_ T: RequireCopyable<ConditionalContainment<Int>>) {}
 /// ----------------
 
 struct AlwaysCopyableDeinit<T: ~Copyable> : Copyable {
+  let nc: NC // expected-error {{stored property 'nc' of 'Copyable'-conforming generic struct 'AlwaysCopyableDeinit' has noncopyable type 'NC'}}
   deinit {} // expected-error {{deinitializer cannot be declared in generic struct 'AlwaysCopyableDeinit' that conforms to 'Copyable'}}
 }
 
@@ -115,7 +116,7 @@ struct RequireCopyable<T> {
 }
 
 struct NC: ~Copyable {
-// expected-note@-1 2{{struct 'NC' has '~Copyable' constraint preventing 'Copyable' conformance}}
+// expected-note@-1 3{{struct 'NC' has '~Copyable' constraint preventing 'Copyable' conformance}}
   deinit {}
 }
 
@@ -150,3 +151,29 @@ struct CornerCase<T: ~Copyable> {
 func chk(_ t: CornerCase<NC>) {}
 // expected-error@-1 {{parameter of noncopyable type 'CornerCase<NC>' must specify ownership}}
 // expected-note@-2 3{{add}}
+
+
+/// MARK: tests that we diagnose ~Copyable that became invalid because it's required to be copyable
+
+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 noncopyable}}
+
+struct OopsConformance1: ~Copyable, NeedsCopyable {}
+// expected-error@-1 {{type 'OopsConformance1' does not conform to protocol 'NeedsCopyable'}}
+// expected-error@-2 {{type 'OopsConformance1' does not conform to protocol 'Copyable'}}
+
+protocol Nothing: ~Copyable, NeedsCopyable {}
+// expected-warning@-1 {{protocol 'Nothing' should be declared to refine 'Copyable' due to a same-type constraint on 'Self'}}
+
+
+struct Extendo: ~Copyable {}
+extension Extendo: Copyable, ~Copyable {} // expected-error {{cannot apply inverse '~Copyable' to extension}}
+
+enum EnumExtendo {}
+extension EnumExtendo: ~Copyable {} // expected-error {{cannot apply inverse '~Copyable' to extension}}
+
+extension NeedsCopyable where Self: ~Copyable {}
+// expected-error@-1 {{cannot add inverse constraint 'Self: ~Copyable' on generic parameter 'Self' defined in outer scope}}

Diff for: test/Parse/inverses.swift

@@ -59,7 +59,7 @@ protocol Rope<Element>: Hashable, ~ Copyable {
   associatedtype Element: ~Copyable
 }
 
-extension S: ~Copyable {}
+extension S: ~Copyable {} // expected-error {{cannot apply inverse '~Copyable' to extension}}
 
 struct S: ~U, // expected-error {{type 'U' is not invertible}}
           ~Copyable {}