Sema: Fix handling of inverse requirements in checkProtocolRefinementRequirements()

Author: slavapestov

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -2047,36 +2047,41 @@ static void diagnoseWrittenPlaceholderTypes(ASTContext &Ctx,
 /// want 'Other' to appear in the inheritance clause of 'Bar', so that
 /// name lookup on Bar can find members of Other.
 static void checkProtocolRefinementRequirements(ProtocolDecl *proto) {
-  auto requiredProtos = proto->getGenericSignature()->getRequiredProtocols(
-      proto->getSelfInterfaceType());
+  auto &ctx = proto->getASTContext();
+  auto selfTy = proto->getSelfInterfaceType();
+  auto genericSig = proto->getGenericSignature();
+
   const bool EnabledNoncopyableGenerics =
-      proto->getASTContext().LangOpts.hasFeature(Feature::NoncopyableGenerics);
+      ctx.LangOpts.hasFeature(Feature::NoncopyableGenerics);
+
+  // If we make a ~P marking but our protocol Self type still conforms to P,
+  // diagnose an error.
+  //
+  // FIXME: This duplicates logic from computeRequirementDiagnostics().
+  if (EnabledNoncopyableGenerics) {
+    // Get the list of written inverses.
+    InvertibleProtocolSet inverses;
+    bool anyObject = false;
+    (void) getDirectlyInheritedNominalTypeDecls(proto, inverses, anyObject);
+
+    for (auto ip : inverses) {
+      auto kp = getKnownProtocolKind(ip);
+      auto *otherProto = ctx.getProtocol(kp);
+      if (!genericSig->requiresProtocol(selfTy, otherProto))
+        continue;
+
+      ctx.Diags.diagnose(proto,
+                         diag::inverse_generic_but_also_conforms,
+                         selfTy, getProtocolName(kp));
+    }
+  }
 
+  auto requiredProtos = genericSig->getRequiredProtocols(selfTy);
   for (auto *otherProto : requiredProtos) {
     // Every protocol 'P' has an implied requirement 'Self : P'; skip it.
     if (otherProto == proto)
       continue;
 
-    // For every invertible protocol IP and any protocol 'P', there is an
-    // implied requirement 'Self: IP', unless it was suppressed via
-    // `Self: ~IP`. So if this suppression annotation exists yet IP was
-    // implied anyway, emit a diagnostic.
-    if (EnabledNoncopyableGenerics) {
-      if (auto kp = otherProto->getKnownProtocolKind()) {
-        if (auto ip = getInvertibleProtocolKind(*kp)) {
-          auto inverse = proto->hasInverseMarking(*ip);
-          if (!inverse)
-            continue; // no ~IP annotation
-
-          auto &Diags = proto->getASTContext().Diags;
-          Diags.diagnose(inverse.getLoc(),
-                         diag::inverse_generic_but_also_conforms,
-                         proto->getSelfInterfaceType(), getProtocolName(*kp));
-          continue;
-        }
-      }
-    }
-
     // SIMDScalar in the standard library currently emits this warning for:
     // 'Hashable', 'Encodable', and 'Decodable'. This is unfortunate, but we
     // cannot fix it as it would alter the ABI of the protocol. Silence these

test/Generics/inverse_generics.swift

@@ -29,9 +29,11 @@ func checkAliases<C, NC: ~Copyable>(_ a: AlwaysCopyable<C>, _ b: AlwaysCopyable<
   checkCopyable(b)
 }
 
+protocol NeedsCopyable {}
+// expected-note@-1 {{type 'TryInferCopyable' does not conform to inherited protocol 'Copyable'}}
+
 struct TryInferCopyable: ~Copyable, NeedsCopyable {}
-// expected-error@-1 {{type 'TryInferCopyable' does not conform to protocol 'NeedsCopyable'}}
-// expected-error@-2 {{type 'TryInferCopyable' does not conform to protocol 'Copyable'}}
+// expected-error@-1 {{type 'TryInferCopyable' does not conform to protocol 'Copyable'}}
 
 protocol Removed: ~Copyable {
   func requiresCopyableSelf(_ t: AlwaysCopyable<Self>)
@@ -165,8 +167,6 @@ func chk(_ t: CornerCase<NC>) {}
 
 /// 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'}}
 
@@ -176,10 +176,6 @@ class NiceTry: ~Copyable, Copyable {} // expected-error {{classes cannot be '~Co
 @_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'}}
-// expected-error@-2 {{type 'OopsConformance1' does not conform to protocol 'Copyable'}}
-
 
 struct Extendo: ~Copyable {}
 extension Extendo: Copyable, ~Copyable {} // expected-error {{cannot apply inverse '~Copyable' to extension}}
@@ -344,8 +340,8 @@ func conflict10<T>(_ t: T, _ u: some ~Copyable & Copyable)
         T: ~Copyable {}
 // expected-error@-1 {{'T' required to be 'Copyable' but is marked with '~Copyable'}}
 
-// FIXME: this is bogus (rdar://119345796)
 protocol Conflict11: ~Copyable, Copyable {}
+// expected-error@-1 {{'Self' required to be 'Copyable' but is marked with '~Copyable'}}
 
 struct Conflict12: ~Copyable, Copyable {}
 // expected-error@-1 {{struct 'Conflict12' required to be 'Copyable' but is marked with '~Copyable'}}

test/Generics/inverse_protocols_errors.swift

@@ -3,8 +3,8 @@
 
 
 protocol RegularProto {}
-protocol NCProto: RegularProto
-  where Self: ~Copyable { // expected-error{{'Self' required to be 'Copyable' but is marked with '~Copyable'}}
+protocol NCProto: RegularProto // expected-error{{'Self' required to be 'Copyable' but is marked with '~Copyable'}}
+  where Self: ~Copyable {
   func checkIfCopyableSelf(_ s: Self)
 }
 

test/Parse/inverses.swift

@@ -1,7 +1,5 @@
 // RUN: %target-typecheck-verify-swift -enable-experimental-feature NoncopyableGenerics
 
-
-
 protocol U {}
 
 enum Maybe<Thing: ~Copyable> : ~Copyable {}
@@ -50,8 +48,7 @@ public struct MoveOnlyS1<T> : ~Copyable { /*deinit {}*/ }
 public struct MoveOnlyS2<T: Equatable> : ~Copyable { /*deinit {}*/ }
 public struct MoveOnlyS3<T: ~Copyable> : ~Copyable { /*deinit {}*/ }
 
-protocol Rope<Element>: Hashable, ~ Copyable {
-
+protocol Rope<Element>: Hashable, ~Copyable {  // expected-error {{'Self' required to be 'Copyable' but is marked with '~Copyable'}}
   associatedtype Element: ~Copyable
 }