NCGenerics: find conflicts in PCT's

We weren't diagnosing conflicts in PCT's like `Copyable & ~Copyable`,
instead deferring until that PCT was constrained to something like the
existential Self or a generic parameter, which then we'd diagnose.

But we should canonicalize PCT's such as `Copyable & Copyable` into
`Any`, which represents the empty composition. That's what the assert in
 PCT::build is about.

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -7567,6 +7567,9 @@ ERROR(inverse_with_class_constraint, none,
       "composition involving %select{class requirement %2|'AnyObject'}0 "
       "cannot contain '~%1'",
       (bool, StringRef, Type))
+ERROR(inverse_conflicts_explicit_composition, none,
+      "composition cannot contain '~%0' when another member requires '%0'",
+      (StringRef))
 ERROR(inverse_extension, none,
       "cannot apply inverse %0 to extension",
       (Type))

include/swift/AST/Requirement.h

@@ -255,15 +255,26 @@ struct InverseRequirement {
   static void enumerateDefaultedParams(GenericContext *decl,
                                        SmallVectorImpl<Type> &result);
 
-  /// \returns the protocols that are required by default for the given type
-  /// parameter. These are not inverses themselves.
-  static InvertibleProtocolSet expandDefault(Type gp);
-
   /// Appends additional requirements corresponding to defaults for the given
   /// generic parameters.
   static void expandDefaults(ASTContext &ctx,
                              ArrayRef<Type> gps,
                              SmallVectorImpl<StructuralRequirement> &result);
+
+  /// Adds the inferred default protocols for an assumed generic parameter with
+  /// respect to that parameter's inverses and existing required protocols.
+  /// For example, if an inverse ~P exists, then P will not be added to the
+  /// protocols list.
+  ///
+  /// Similarly, if the protocols list has a protocol Q that already implies
+  /// Copyable, then we will not add `Copyable` to the protocols list.
+  ///
+  /// \param inverses the inverses '& ~P' that are applied to the generic param.
+  /// \param protocols the existing required protocols, to which defaults will
+  ///                  be appended.
+  static void expandDefaults(ASTContext &ctx,
+                             InvertibleProtocolSet inverses,
+                             SmallVectorImpl<ProtocolDecl*> &protocols);
 };
 
 } // end namespace swift

lib/AST/Requirement.cpp

@@ -342,27 +342,64 @@ void InverseRequirement::enumerateDefaultedParams(
     add(gtpd->getDeclaredInterfaceType());
 }
 
-InvertibleProtocolSet InverseRequirement::expandDefault(Type gp) {
-  assert(gp->isTypeParameter());
-  return InvertibleProtocolSet::full();
-}
-
 void InverseRequirement::expandDefaults(
     ASTContext &ctx,
     ArrayRef<Type> gps,
     SmallVectorImpl<StructuralRequirement> &result) {
-  if (!ctx.LangOpts.hasFeature(Feature::NoncopyableGenerics))
+
+  SmallVector<ProtocolDecl*, NumInvertibleProtocols> defaults;
+  expandDefaults(ctx, /*inverses=*/{}, defaults);
+
+  // Fast-path.
+  if (defaults.empty())
     return;
 
   for (auto gp : gps) {
-    auto protos = InverseRequirement::expandDefault(gp);
-    for (auto ip : protos) {
-      auto proto = ctx.getProtocol(getKnownProtocolKind(ip));
-      assert(proto && "missing Copyable/Escapable from stdlib!");
-      
+    for (auto *proto : defaults) {
       auto protoTy = proto->getDeclaredInterfaceType();
       result.push_back({{RequirementKind::Conformance, gp, protoTy},
                         SourceLoc()});
     }
   }
 }
+
+void InverseRequirement::expandDefaults(
+    ASTContext &ctx,
+    InvertibleProtocolSet inverses,
+    SmallVectorImpl<ProtocolDecl*> &protocols) {
+
+  // Skip unless noncopyable generics is enabled
+  if (!ctx.LangOpts.hasFeature(swift::Feature::NoncopyableGenerics))
+    return;
+
+  // Try to add all invertible protocols, unless:
+  //  - an inverse was provided
+  //  - an existing protocol already requires it
+  for (auto ip : InvertibleProtocolSet::full()) {
+    // This matches with `lookupExistentialConformance`'s use of 'inheritsFrom'.
+    bool alreadyRequired = false;
+    for (auto proto : protocols) {
+      if (proto->isSpecificProtocol(getKnownProtocolKind(ip))
+          || proto->requiresInvertible(ip)) {
+        alreadyRequired = true;
+        break;
+      }
+    }
+
+    // If some protocol member already implies P, then we don't need to
+    // add a requirement for P.
+    if (alreadyRequired) {
+      assert(!inverses.contains(ip) && "cannot require P and ~P");
+      continue;
+    }
+
+    // Nothing implies P, so unless there's an inverse ~P, add the requirement.
+    if (inverses.contains(ip))
+      continue;
+
+    auto proto = ctx.getProtocol(getKnownProtocolKind(ip));
+    assert(proto && "missing Copyable/Escapable from stdlib!");
+
+    protocols.push_back(proto);
+  }
+}

lib/AST/Type.cpp

@@ -278,50 +278,7 @@ bool TypeBase::allowsOwnership(const GenericSignatureImpl *sig) {
   return getCanonicalType().allowsOwnership(sig);
 }
 
-/// Adds the inferred default protocols for an ExistentialLayout with respect
-/// to that existential's inverses and existing protocols. For example, if an
-/// inverse ~P exists, then P will not be added to the protocols list.
-///
-/// Similarly, if the protocols list has a protocol Q that already implies
-/// Copyable, then we will not add `Copyable` to the protocols list.
-///
-/// \param inverses the inverses '& ~P' that are in the existential's type.
-/// \param protocols the output vector of protocols for an ExistentialLayout
-///                  to be modified.
-static void expandDefaultProtocols(
-    ASTContext &ctx,
-    InvertibleProtocolSet inverses,
-    SmallVectorImpl<ProtocolDecl*> &protocols) {
-
-  // Skip unless noncopyable generics is enabled
-  if (!ctx.LangOpts.hasFeature(swift::Feature::NoncopyableGenerics))
-    return;
 
-  // Try to add all invertible protocols, unless:
-  //  - an inverse was provided
-  //  - an existing protocol already requires it
-  for (auto ip : InvertibleProtocolSet::full()) {
-    if (inverses.contains(ip))
-      continue;
-
-    // This matches with `lookupExistentialConformance`'s use of 'inheritsFrom'.
-    bool alreadyRequired = false;
-    for (auto proto : protocols) {
-      if (proto->requiresInvertible(ip)) {
-        alreadyRequired = true;
-        break;
-      }
-    }
-
-    if (alreadyRequired)
-      continue;
-
-    auto proto = ctx.getProtocol(getKnownProtocolKind(ip));
-    assert(proto && "missing Copyable/Escapable from stdlib!");
-
-    protocols.push_back(proto);
-  }
-}
 
 ExistentialLayout::ExistentialLayout(CanProtocolType type) {
   auto *protoDecl = type->getDecl();
@@ -336,7 +293,7 @@ ExistentialLayout::ExistentialLayout(CanProtocolType type) {
   protocols.push_back(protoDecl);
 
   // NOTE: all the invertible protocols are usable from ObjC.
-  expandDefaultProtocols(type->getASTContext(), {}, protocols);
+  InverseRequirement::expandDefaults(type->getASTContext(), {}, protocols);
 }
 
 ExistentialLayout::ExistentialLayout(CanProtocolCompositionType type) {
@@ -373,7 +330,9 @@ ExistentialLayout::ExistentialLayout(CanProtocolCompositionType type) {
       hasExplicitAnyObject && !explicitSuperclass && getProtocols().empty();
 
   // NOTE: all the invertible protocols are usable from ObjC.
-  expandDefaultProtocols(type->getASTContext(), type->getInverses(), protocols);
+  InverseRequirement::expandDefaults(type->getASTContext(),
+                                     type->getInverses(),
+                                     protocols);
 }
 
 ExistentialLayout::ExistentialLayout(CanParameterizedProtocolType type)
@@ -3939,7 +3898,7 @@ Type ProtocolCompositionType::get(const ASTContext &C,
                                   ArrayRef<Type> Members,
                                   InvertibleProtocolSet Inverses,
                                   bool HasExplicitAnyObject) {
-  // Fast path for 'AnyObject' and 'Any'.
+  // Fast path for 'AnyObject', 'Any', and '~Copyable'.
   if (Members.empty()) {
     return build(C, Members, Inverses, HasExplicitAnyObject);
   }
@@ -3994,6 +3953,13 @@ Type ProtocolCompositionType::get(const ASTContext &C,
       }
     }
 
+    // Drop any explicitly provided invertible protocols.
+    if (auto ip = proto->getInvertibleProtocolKind()) {
+      // We diagnose '~Copyable & Copyable' before forming the PCT.
+      assert(!Inverses.contains(*ip) && "opposing invertible constraints!");
+      continue;
+    }
+
     CanTypes.push_back(proto->getDeclaredInterfaceType());
   }
 

lib/Sema/TypeCheckType.cpp

@@ -5367,6 +5367,46 @@ NeverNullType TypeResolver::resolveTupleType(TupleTypeRepr *repr,
   return TupleType::get(elements, ctx);
 }
 
+/// \returns the inverse ~P that is conflicted if a protocol in \c tys
+/// requires P. For example, `Q & ~Copyable` is a conflict, if `Q` requires
+/// or is equal to `Copyable`
+static std::optional<InvertibleProtocolKind>
+hasConflictedInverse(ArrayRef<Type> tys, InvertibleProtocolSet inverses) {
+  // Fast-path: no inverses that could be conflicted!
+  if (inverses.empty())
+    return std::nullopt;
+
+  for (auto ty : tys) {
+    // Handle nested PCT's recursively since we haven't flattened them away yet.
+    if (auto pct = dyn_cast<ProtocolCompositionType>(ty)) {
+      if (auto conflict = hasConflictedInverse(pct->getMembers(), inverses))
+        return conflict;
+      continue;
+    }
+
+    // Dig out a protocol.
+    ProtocolDecl *decl = nullptr;
+    if (auto protoTy = dyn_cast<ProtocolType>(ty))
+      decl = protoTy->getDecl();
+    else if (auto paramProtoTy = dyn_cast<ParameterizedProtocolType>(ty))
+      decl = paramProtoTy->getProtocol();
+
+    if (!decl)
+      continue;
+
+    // If an inverse ~I exists for this protocol member of the PCT that
+    // requires I, then it's a conflict.
+    for (auto inverse : inverses) {
+      if (decl->isSpecificProtocol(getKnownProtocolKind(inverse))
+          || decl->requiresInvertible(inverse)) {
+        return inverse;
+      }
+    }
+  }
+
+  return std::nullopt;
+}
+
 NeverNullType
 TypeResolver::resolveCompositionType(CompositionTypeRepr *repr,
                                      TypeResolutionOptions options) {
@@ -5462,6 +5502,13 @@ TypeResolver::resolveCompositionType(CompositionTypeRepr *repr,
     IsInvalid = true;
   }
 
+  // Cannot provide an inverse in the same composition requiring the protocol.
+  if (auto conflict = hasConflictedInverse(Members, Inverses)) {
+    diagnose(repr->getLoc(),
+       diag::inverse_conflicts_explicit_composition,
+       getProtocolName(getKnownProtocolKind(*conflict)));
+    IsInvalid = true;
+  }
 
   if (IsInvalid) {
     repr->setInvalid();

test/Generics/inverse_generics.swift

@@ -80,6 +80,9 @@ struct ConditionalContainment<T: ~Copyable> {
 
 func chk(_ T: RequireCopyable<ConditionalContainment<Int>>) {}
 
+// expected-note@+2 3{{add}}
+// expected-error@+1 {{parameter of noncopyable type 'some Escapable & ~Copyable' must specify ownership}}
+func dogDays(_ t: some Escapable & ~Copyable) {}
 
 /// ----------------
 
@@ -289,11 +292,14 @@ func conflict1<T>(_ t: T) where T: NeedsCopyable, T: ~Copyable {}
 func conflict2<T: ~Copyable>(_ t: AlwaysCopyable<T>) {}
 // expected-error@-1 {{'T' required to be 'Copyable' but is marked with '~Copyable'}}
 
-func conflict3<T: NeedsCopyable & ~Copyable>(_ t: T) {}
+func conflict3a<T: NeedsCopyable & ~Copyable>(_ t: T) {}
+// expected-error@-1 {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
+
+func conflict3b<T>(_ t: T) where T: NeedsCopyable, T: ~Copyable {}
 // expected-error@-1 {{'T' required to be 'Copyable' but is marked with '~Copyable'}}
 
-func conflict4(_ t: some NeedsCopyable & ~Copyable) {}
-// expected-error@-1 {{'some NeedsCopyable & ~Copyable' required to be 'Copyable' but is marked with '~Copyable'}}
+func conflict4a(_ t: some NeedsCopyable & ~Copyable) {}
+// expected-error@-1 {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
 
 protocol Conflict5: ~Copyable {
   borrowing func whatever() -> AlwaysCopyable<Self> // expected-error {{type 'Self' does not conform to protocol 'Copyable'}}
@@ -322,7 +328,7 @@ func conflict9<U: ~Copyable>(_ u: Conflict9<U>) {}
 // expected-error@-1 {{'U' required to be 'Copyable' but is marked with '~Copyable'}}
 
 func conflict10<T>(_ t: T, _ u: some ~Copyable & Copyable)
-// expected-error@-1 {{'some ~Copyable & Copyable' required to be 'Copyable' but is marked with '~Copyable'}}
+// expected-error@-1 {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
   where T: Copyable,
         T: ~Copyable {}
 // expected-error@-1 {{'T' required to be 'Copyable' but is marked with '~Copyable'}}
@@ -378,6 +384,10 @@ func checkClassBound2<T>(_ t: T) where T: ~Escapable, T: AnyObject, T: ~Copyable
 func checkClassBound3<T>(_ t: T) where T: Soup & ~Copyable & ~Escapable {}
 // expected-error@-1 {{composition involving class requirement 'Soup' cannot contain '~Copyable'}}
 
+func checkClassBound4<T>(_ t: T) where T: Soup, T: ~Copyable & ~Escapable {}
+// expected-error@-1 {{'T' required to be 'Copyable' but is marked with '~Copyable'}}
+// expected-error@-2 {{'T' required to be 'Escapable' but is marked with '~Escapable'}}
+
 public func checkAnyObjInv1<Result: AnyObject>(_ t: borrowing Result) where Result: ~Copyable {}
 // expected-error@-1 {{'Result' required to be 'Copyable' but is marked with '~Copyable'}}
 
@@ -390,8 +400,10 @@ public func checkAnyObject<Result>(_ t: Result) where Result: AnyObject {
 
 func checkExistentialAndClasses(
     _ a: any AnyObject & ~Copyable, // expected-error {{composition involving 'AnyObject' cannot contain '~Copyable'}}
-    _ b: any Soup & Copyable & ~Escapable & ~Copyable, // expected-error {{composition involving class requirement 'Soup' cannot contain '~Copyable'}}
-    _ c: some (~Escapable & Removed) & Soup // expected-error {{composition involving class requirement 'Soup' cannot contain '~Escapable'}}
+    _ b: any Soup & Copyable & ~Escapable & ~Copyable,
+    // expected-error@-1 {{composition involving class requirement 'Soup' cannot contain '~Copyable'}}
+    // expected-error@-2 {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
+    _ c: some (~Escapable & Removed) & Soup // expected-error {{composition cannot contain '~Escapable' when another member requires 'Escapable'}}
     ) {}
 
 protocol HasNCBuddy: ~Copyable {
@@ -435,3 +447,13 @@ protocol Q: Copyable {}
 // expected-error@+1 {{parameter of noncopyable type 'Blahaj<T>' must specify ownership}}
 func testBlahaj<T, U: Q>(_ x: Blahaj<T>,
                          _ y: Blahaj<U>) {}
+
+extension Int: NeedsCopyable {}
+
+func checkExistentials() {
+    let _: any ~Escapable & (NeedsCopyable & ~Copyable) // expected-error {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
+    let _: any NeedsCopyable & ~Copyable = 1 // expected-error {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
+    let _: any NeedsCopyable & ~Escapable = 1 // expected-error {{composition cannot contain '~Escapable' when another member requires 'Escapable'}}
+    let _: any Copyable & ~Copyable = 1 // expected-error {{composition cannot contain '~Copyable' when another member requires 'Copyable'}}
+    let _: any Escapable & ~Escapable = 1 // expected-error {{composition cannot contain '~Escapable' when another member requires 'Escapable'}}
+}