[AST] Don't allow conditional conformances to imply others.

Many uses of conditional conformance to protocols with super-protocols
are for wrappers, where the conformances to those super-protocols
usually ends up using weaker bounds, such as:

  struct MyWrapper<Wrapped: Sequence> {}
  extension Wrapped: Sequence {} // Wrapped: Sequence
  extension Wrapped: Collection where Wrapped: Collection {} // *
  extension Wrapped: BidirectionalCollection where Wrapped: BidirectionalCollection {}

If this code was instead written:

  struct MyWrapper<Wrapped: Sequence> {}
  extension Wrapped: Sequence {} // Wrapped: Sequence
  extension Wrapped: BidirectionalCollection where Wrapped: BidirectionalCollection {}

Inferring a Collection conformance would have to mean

  extension Wrapped: Collection where Wrapped: BidirectionalCollection {}

which is unnecessarily strong.

It is a breaking change to change a protocol bound, and so we're
thinking we'd prefer that the compiler didn't magic up that incorrect
conformance. It also only is a small change (and the compiler even
suggests it, with a fixit) to explicitly get the implying behaviour:
declare the conformance explicitly.

  extension Wrapped: Collection, BidirectionalCollection where Wrapped: BidirectionalCollection {}

Fixes rdar://problem/36499373.

Author: huonw

include/swift/AST/DiagnosticsSema.def

@@ -1443,6 +1443,16 @@ ERROR(objc_generics_cannot_conditionally_conform,none,
       "type %0 cannot conditionally conform to protocol %1 because "
       "the type uses the Objective-C generics model",
       (Type, Type))
+ERROR(conditional_conformances_cannot_imply_conformances,none,
+      "conditional conformance of type %0 to protocol %1 does not imply conformance to "
+      "inherited protocol %2",
+      (Type, Type, Type))
+NOTE(note_explicitly_state_conditional_conformance_different,none,
+     "did you mean to explicitly state the conformance with different bounds?", ())
+NOTE(note_explicitly_state_conditional_conformance_relaxed,none,
+     "did you mean to explicitly state the conformance with relaxed bounds?", ())
+NOTE(note_explicitly_state_conditional_conformance_same,none,
+     "did you mean to explicitly state the conformance with the same bounds?", ())
 ERROR(protocol_has_missing_requirements,none,
       "type %0 cannot conform to protocol %1 because it has requirements that "
       "cannot be satisfied", (Type, Type))

lib/Sema/TypeCheckProtocol.cpp

@@ -1263,6 +1263,118 @@ void MultiConformanceChecker::checkAllConformances() {
   }
 }
 
+static void diagnoseConformanceImpliedByConditionalConformance(
+    DiagnosticEngine &Diags, NormalProtocolConformance *conformance,
+    NormalProtocolConformance *implyingConf, bool issueFixit) {
+  Type T = conformance->getType();
+  auto proto = conformance->getProtocol();
+  Type protoType = proto->getDeclaredType();
+  auto implyingProto = implyingConf->getProtocol()->getDeclaredType();
+  auto loc = implyingConf->getLoc();
+  Diags.diagnose(loc, diag::conditional_conformances_cannot_imply_conformances,
+                 T, implyingProto, protoType);
+
+  if (!issueFixit)
+    return;
+
+  // Now we get down to business: constructing a few options for new
+  // extensions. They all look like:
+  //
+  // extension T: ProtoType where ... {
+  //     <# witnesses #>
+  // }
+  //
+  // The options are:
+  //
+  // - if possible, the original bounds relaxed, when the requirements match the
+  //   conforming protocol, e.g. 'X: Hashable where T: Hashable' often
+  //   corresponds to 'X: Equatable where T: Equatable'. This fixit is included
+  //   if all the requirements are conformance constraints to the protocol
+  //   that implies the conformance.
+  // - the same bounds: ... is copied from the implying extension
+  // - new bounds: ... becomes a placeholder
+  //
+  // We could also suggest adding ", ProtoType" to the existing extension,
+  // but we don't think having multiple conformances in a single extension
+  // (especially conditional ones) is good Swift style, and so we don't
+  // want to encourage it.
+
+  auto ext = cast<ExtensionDecl>(implyingConf->getDeclContext());
+
+  auto &SM = ext->getASTContext().SourceMgr;
+  StringRef extraIndent;
+  StringRef indent = Lexer::getIndentationForLine(SM, loc, &extraIndent);
+
+  // First, the bits that aren't the requirements are the same for all the
+  // types.
+  llvm::SmallString<128> prefix;
+  llvm::SmallString<128> suffix;
+  {
+    llvm::raw_svector_ostream prefixStream(prefix);
+    llvm::raw_svector_ostream suffixStream(suffix);
+
+    prefixStream << "extension " << T << ": " << protoType << " ";
+    suffixStream << " {\n"
+                 << indent << extraIndent << "<#witnesses#>\n"
+                 << indent << "}\n\n"
+                 << indent;
+  }
+
+  // First, we do the fixit for "matching" requirements (i.e. X: P where T: P).
+  bool matchingIsValid = true;
+  llvm::SmallString<128> matchingFixit = prefix;
+  {
+    llvm::raw_svector_ostream matchingStream(matchingFixit);
+    matchingStream << "where ";
+    bool first = true;
+    for (const auto &req : implyingConf->getConditionalRequirements()) {
+      auto firstType = req.getFirstType();
+      // T: ImplyingProto => T: Proto
+      if (req.getKind() == RequirementKind::Conformance &&
+          req.getSecondType()->isEqual(implyingProto)) {
+        auto comma = first ? "" : ", ";
+        matchingStream << comma << firstType << ": " << protoType;
+        first = false;
+        continue;
+      }
+      // something didn't work out, so give up on this fixit.
+      matchingIsValid = false;
+      break;
+    }
+  }
+
+  if (matchingIsValid) {
+    matchingFixit += suffix;
+    Diags
+        .diagnose(loc,
+                  diag::note_explicitly_state_conditional_conformance_relaxed)
+        .fixItInsert(loc, matchingFixit);
+  }
+
+  // Next, do the fixit for using the same requirements, but be resilient to a
+  // missing `where` clause: this is one of a few fixits that get emitted here,
+  // and so is a very low priority diagnostic, and so shouldn't crash.
+  if (auto TWC = ext->getTrailingWhereClause()) {
+    llvm::SmallString<128> sameFixit = prefix;
+    auto CSR =
+        Lexer::getCharSourceRangeFromSourceRange(SM, TWC->getSourceRange());
+    sameFixit += SM.extractText(CSR);
+    sameFixit += suffix;
+    Diags
+        .diagnose(loc, diag::note_explicitly_state_conditional_conformance_same)
+        .fixItInsert(loc, sameFixit);
+  }
+
+  // And finally, just the generic new-requirements one:
+  llvm::SmallString<128> differentFixit = prefix;
+  differentFixit += "where <#requirements#>";
+  differentFixit += suffix;
+  Diags
+      .diagnose(loc,
+                diag::note_explicitly_state_conditional_conformance_different)
+      .fixItInsert(loc, differentFixit);
+}
+
 /// \brief Determine whether the type \c T conforms to the protocol \c Proto,
 /// recording the complete witness table if it does.
 ProtocolConformance *MultiConformanceChecker::
@@ -1300,6 +1412,7 @@ checkIndividualConformance(NormalProtocolConformance *conformance,
   auto canT = T->getCanonicalType();
   DeclContext *DC = conformance->getDeclContext();
   auto Proto = conformance->getProtocol();
+  auto ProtoType = Proto->getDeclaredType();
   SourceLoc ComplainLoc = conformance->getLoc();
 
   // Note that we are checking this conformance now.
@@ -1317,7 +1430,7 @@ checkIndividualConformance(NormalProtocolConformance *conformance,
   // If the protocol requires a class, non-classes are a non-starter.
   if (Proto->requiresClass() && !canT->getClassOrBoundGenericClass()) {
     TC.diagnose(ComplainLoc, diag::non_class_cannot_conform_to_class_protocol,
-                T, Proto->getDeclaredType());
+                T, ProtoType);
     conformance->setInvalid();
     return conformance;
   }
@@ -1338,8 +1451,7 @@ checkIndividualConformance(NormalProtocolConformance *conformance,
           break;
       }
       if (diagKind) {
-        TC.diagnose(ComplainLoc, diagKind.getValue(),
-                    T, Proto->getDeclaredType());
+        TC.diagnose(ComplainLoc, diagKind.getValue(), T, ProtoType);
         conformance->setInvalid();
         return conformance;
       }
@@ -1357,7 +1469,7 @@ checkIndividualConformance(NormalProtocolConformance *conformance,
         if (clas->usesObjCGenericsModel()) {
           TC.diagnose(ComplainLoc,
                       diag::objc_generics_cannot_conditionally_conform, T,
-                      Proto->getDeclaredType());
+                      ProtoType);
           conformance->setInvalid();
           return conformance;
         }
@@ -1376,21 +1488,49 @@ checkIndividualConformance(NormalProtocolConformance *conformance,
       if (serialized->getLanguageVersionBuiltWith() !=
           TC.getLangOpts().EffectiveLanguageVersion) {
         TC.diagnose(ComplainLoc,
-                    diag::protocol_has_missing_requirements_versioned,
-                    T, Proto->getDeclaredType(),
-                    serialized->getLanguageVersionBuiltWith(),
+                    diag::protocol_has_missing_requirements_versioned, T,
+                    ProtoType, serialized->getLanguageVersionBuiltWith(),
                     TC.getLangOpts().EffectiveLanguageVersion);
         hasDiagnosed = true;
       }
     }
     if (!hasDiagnosed) {
-      TC.diagnose(ComplainLoc, diag::protocol_has_missing_requirements,
-                  T, Proto->getDeclaredType());
+      TC.diagnose(ComplainLoc, diag::protocol_has_missing_requirements, T,
+                  ProtoType);
     }
     conformance->setInvalid();
     return conformance;
   }
 
+  bool impliedDisablesMissingWitnessFixits = false;
+  if (conformance->getSourceKind() == ConformanceEntryKind::Implied) {
+    // We've got something like:
+    //
+    //   protocol Foo : Proto {}
+    //   extension SomeType : Foo {}
+    //
+    // We don't want to allow this when the SomeType : Foo conformance is
+    // conditional
+    auto implyingConf = conformance->getImplyingConformance();
+    // There might be a long chain of implications, e.g. protocol Foo: Proto {}
+    // protocol Bar: Foo {} extension SomeType: Bar {}, so keep looking all the
+    // way up.
+    while (implyingConf->getSourceKind() == ConformanceEntryKind::Implied) {
+      implyingConf = implyingConf->getImplyingConformance();
+    }
+    if (!implyingConf->getConditionalRequirements().empty()) {
+      // We shouldn't suggest including witnesses for the conformance, because
+      // those suggestions will go in the current DeclContext, but really they
+      // should go into the new extension we (might) suggest here.
+      impliedDisablesMissingWitnessFixits = true;
+
+      diagnoseConformanceImpliedByConditionalConformance(
+          TC.Diags, conformance, implyingConf, issueFixit);
+
+      conformance->setInvalid();
+    }
+  }
+
   // Check that T conforms to all inherited protocols.
   for (auto InheritedProto : Proto->getInheritedProtocols()) {
     auto InheritedConformance =
@@ -1420,9 +1560,11 @@ checkIndividualConformance(NormalProtocolConformance *conformance,
   AllUsedCheckers.emplace_back(TC, conformance, MissingWitnesses);
   MissingWitnesses.insert(revivedMissingWitnesses.begin(),
                           revivedMissingWitnesses.end());
-  AllUsedCheckers.back().checkConformance(issueFixit ?
-                                    MissingWitnessDiagnosisKind::ErrorFixIt :
-                                    MissingWitnessDiagnosisKind::ErrorOnly);
+
+  auto missingWitnessFixits = issueFixit && !impliedDisablesMissingWitnessFixits;
+  AllUsedCheckers.back().checkConformance(
+      missingWitnessFixits ? MissingWitnessDiagnosisKind::ErrorFixIt
+                           : MissingWitnessDiagnosisKind::ErrorOnly);
   return conformance;
 }
 

test/Generics/conditional_conformances.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -typecheck %s -verify
+// RUN: %target-typecheck-verify-swift -typecheck -verify
 // RUN: %target-typecheck-verify-swift -typecheck -debug-generic-signatures %s > %t.dump 2>&1
 // RUN: %FileCheck %s < %t.dump
 
@@ -7,10 +7,7 @@ protocol P2 {}
 protocol P3 {}
 protocol P4: P1 {}
 protocol P5: P2 {}
-// expected-note@-1{{type 'InheritImplicitGood<T>' does not conform to inherited protocol 'P2'}}
-// expected-note@-2{{type 'InheritImplicitBad<T>' does not conform to inherited protocol 'P2'}}
 protocol P6: P2 {}
-// expected-note@-1{{type 'InheritImplicitBad<T>' does not conform to inherited protocol 'P2'}}
 
 protocol Assoc { associatedtype AT }
 
@@ -276,35 +273,64 @@ func inheritequal_bad_bad<U>(_: U) {
   // expected-note@-3{{requirement from conditional conformance of 'InheritMore<U>' to 'P5'}}
 }
 
-struct InheritImplicitGood<T> {}
-// FIXME: per SE-0143, this should result in an implicit conformance
-// InheritImplicitGood: P2.
-extension InheritImplicitGood: P5 where T: P1 {}
-// expected-error@-1{{type 'InheritImplicitGood<T>' does not conform to protocol 'P2'}}
+struct InheritImplicitOne<T> {}
+// This shouldn't give anything implicit since we disallow implication for
+// conditional conformances (in many cases, the implied bounds are
+// incorrect/insufficiently general).
+extension InheritImplicitOne: P5 where T: P1 {}
+// expected-error@-1{{conditional conformance of type 'InheritImplicitOne<T>' to protocol 'P5' does not imply conformance to inherited protocol 'P2'}}
+// expected-note@-2{{did you mean to explicitly state the conformance with the same bounds?}}
+// expected-note@-3{{did you mean to explicitly state the conformance with different bounds?}}
+
+struct InheritImplicitTwo<T> {}
+// Even if we relax the rule about implication, this double-up should still be
+// an error, because either conformance could imply InheritImplicitTwo: P2.
+extension InheritImplicitTwo: P5 where T: P1 {}
+// expected-error@-1{{conditional conformance of type 'InheritImplicitTwo<T>' to protocol 'P5' does not imply conformance to inherited protocol 'P2'}}
+// expected-note@-2{{did you mean to explicitly state the conformance with the same bounds?}}
+// expected-note@-3{{did you mean to explicitly state the conformance with different bounds?}}
+extension InheritImplicitTwo: P6 where T: P1 {}
+
+// However, if there's a non-conditional conformance that implies something, we
+// can imply from that one.
+struct InheritImplicitGood1<T> {}
+extension InheritImplicitGood1: P5 {}
+extension InheritImplicitGood1: P6 where T: P1 {}
+
+func inheritimplicitgood1<T>(_ : T) {
+    takes_P2(InheritImplicitGood1<T>()) // unconstrained!
+    takes_P2(InheritImplicitGood1<Int>())
+}
+struct InheritImplicitGood2<T> {}
+extension InheritImplicitGood2: P6 where T: P1 {}
+extension InheritImplicitGood2: P5 {}
 
-struct InheritImplicitBad<T> {}
-// This shouldn't give anything implicit since either conformance could imply
-// InheritImplicitBad: P2.
-extension InheritImplicitBad: P5 where T: P1 {}
-// expected-error@-1{{type 'InheritImplicitBad<T>' does not conform to protocol 'P2'}}
-extension InheritImplicitBad: P6 where T: P1 {}
-// expected-error@-1{{type 'InheritImplicitBad<T>' does not conform to protocol 'P2'}}
+func inheritimplicitgood2<T>(_: T) {
+    takes_P2(InheritImplicitGood2<T>()) // unconstrained!
+    takes_P2(InheritImplicitGood2<Int>())
 
+}
+struct InheritImplicitGood3<T>: P5 {}
+extension InheritImplicitGood3: P6 where T: P1 {}
 
+func inheritimplicitgood3<T>(_: T) {
+    takes_P2(InheritImplicitGood3<T>()) // unconstrained!
+    takes_P2(InheritImplicitGood3<Int>())
+}
 
 // "Multiple conformances" from SE0143
 
 struct TwoConformances<T> {}
 extension TwoConformances: P2 where T: P1 {}
-// expected-error@-1{{redundant conformance of 'TwoConformances<T>' to protocol 'P2'}}
-extension TwoConformances: P2 where T: P3 {}
 // expected-note@-1{{'TwoConformances<T>' declares conformance to protocol 'P2' here}}
+extension TwoConformances: P2 where T: P3 {}
+// expected-error@-1{{redundant conformance of 'TwoConformances<T>' to protocol 'P2'}}
 
 struct TwoDisjointConformances<T> {}
 extension TwoDisjointConformances: P2 where T == Int {}
-// expected-error@-1{{redundant conformance of 'TwoDisjointConformances<T>' to protocol 'P2'}}
-extension TwoDisjointConformances: P2 where T == String {}
 // expected-note@-1{{'TwoDisjointConformances<T>' declares conformance to protocol 'P2' here}}
+extension TwoDisjointConformances: P2 where T == String {}
+// expected-error@-1{{redundant conformance of 'TwoDisjointConformances<T>' to protocol 'P2'}}
 
 
 // FIXME: these cases should be equivalent (and both with the same output as the

test/Generics/conditional_conformances_fixit.swift

@@ -0,0 +1,41 @@
+// RUN: %target-typecheck-verify-swift -emit-fixits-path %t.remap -fixit-all
+// RUN: c-arcmt-test %t.remap | arcmt-test -verify-transformed-files %s.result
+
+protocol P1 {}
+protocol P2: P1 {}
+
+
+struct S1<T> {}
+
+extension S1: P2 where T: P1 {}
+// expected-error@-1 {{conditional conformance of type 'S1<T>' to protocol 'P2' does not imply conformance to inherited protocol 'P1'}}
+// expected-note@-2 {{did you mean to explicitly state the conformance with the same bounds?}}
+// expected-note@-3 {{did you mean to explicitly state the conformance with different bounds?}}
+
+protocol P3 {
+    associatedtype X
+}
+struct S2<T, U, V: P3> {}
+
+extension S2: P2 where T: P2, U: P2, V.X: P2 {}
+// expected-error@-1 {{conditional conformance of type 'S2<T, U, V>' to protocol 'P2' does not imply conformance to inherited protocol 'P1'}}
+// expected-note@-2 {{did you mean to explicitly state the conformance with relaxed bounds?}}
+// expected-note@-3 {{did you mean to explicitly state the conformance with the same bounds?}}
+// expected-note@-4 {{did you mean to explicitly state the conformance with different bounds?}}
+
+
+struct S3<T, U, V: P3> {}
+
+extension S3: P2 where T: P2, U: P2, V.X == Int {}
+// expected-error@-1 {{conditional conformance of type 'S3<T, U, V>' to protocol 'P2' does not imply conformance to inherited protocol 'P1'}}
+// expected-note@-2 {{did you mean to explicitly state the conformance with the same bounds?}}
+// expected-note@-3 {{did you mean to explicitly state the conformance with different bounds?}}
+
+
+struct S4<T, U, V: P3> {}
+
+extension S4: P2 where T: P2, U: P3, V.X: P2 {}
+// expected-error@-1 {{conditional conformance of type 'S4<T, U, V>' to protocol 'P2' does not imply conformance to inherited protocol 'P1'}}
+// expected-note@-2 {{did you mean to explicitly state the conformance with the same bounds?}}
+// expected-note@-3 {{did you mean to explicitly state the conformance with different bounds?}}
+