[RequirementMachine] Mention the type parameter that is the subject of

two conflicting requirements in diagnostics where possible.

Author: hborla

lib/AST/RequirementMachine/Diagnostics.h

@@ -40,14 +40,26 @@ struct RequirementError {
     RedundantRequirement,
   } kind;
 
+  /// The type parameter on which there is an invalid or conflicting
+  /// requirement.
+  ///
+  /// FIXME: We probably want to just store two separate requirements
+  /// in the case of a confict. Right now, the conflicting constraint
+  /// types are both stored in the requirement below, and this serves
+  /// as the subject type.
+  Type typeParameter;
+
   /// The invalid requirement.
   Requirement requirement;
 
   SourceLoc loc;
 
 private:
   RequirementError(Kind kind, Requirement requirement, SourceLoc loc)
-    : kind(kind), requirement(requirement), loc(loc) {}
+    : kind(kind), typeParameter(Type()), requirement(requirement), loc(loc) {}
+
+  RequirementError(Kind kind, Type subject, Requirement requirement, SourceLoc loc)
+    : kind(kind), typeParameter(subject), requirement(requirement), loc(loc) {}
 
 public:
   static RequirementError forInvalidTypeRequirement(Type subjectType,
@@ -69,6 +81,12 @@ struct RequirementError {
     return {Kind::ConflictingRequirement, req, loc};
   }
 
+  static RequirementError forConflictingRequirement(Type typeParameter,
+                                                    Requirement req,
+                                                    SourceLoc loc) {
+    return {Kind::ConflictingRequirement, typeParameter, req, loc};
+  }
+
   static RequirementError forSameTypeMissingRequirement(Requirement req,
                                                         SourceLoc loc) {
     return {Kind::SameTypeMissingRequirement, req, loc};

lib/AST/RequirementMachine/RequirementLowering.cpp

@@ -726,6 +726,24 @@ bool swift::rewriting::diagnoseRequirementErrors(
     }
 
     case RequirementError::Kind::ConflictingRequirement: {
+      // FIXME: Unify this case with SameTypeMissingRequirement.
+      if (auto subjectType = error.typeParameter) {
+        auto firstType = error.requirement.getFirstType();
+        auto secondType = error.requirement.getSecondType();
+
+        if (error.requirement.getKind() == RequirementKind::SameType) {
+          ctx.Diags.diagnose(loc, diag::same_type_conflict,
+                             false, subjectType, firstType, secondType);
+        } else {
+          assert(error.requirement.getKind() == RequirementKind::Superclass);
+          ctx.Diags.diagnose(loc, diag::conflicting_superclass_constraints,
+                             subjectType, firstType, secondType);
+
+        }
+        diagnosedError = true;
+        break;
+      }
+
       auto subjectType = error.requirement.getFirstType();
       if (subjectType->hasError())
         break;

lib/AST/RequirementMachine/RequirementMachine.cpp

@@ -533,7 +533,8 @@ void RequirementMachine::freeze() {
 void RequirementMachine::computeRequirementDiagnostics(
     SmallVectorImpl<RequirementError> &errors, SourceLoc signatureLoc) {
   System.computeRedundantRequirementDiagnostics(errors);
-  System.computeConflictDiagnostics(errors, signatureLoc);
+  System.computeConflictDiagnostics(errors, signatureLoc, Map,
+                                    getGenericParams());
 }
 
 std::string RequirementMachine::getRuleAsStringForDiagnostics(

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -15,6 +15,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <vector>
+#include "PropertyMap.h"
 #include "RewriteContext.h"
 #include "RewriteLoop.h"
 #include "RewriteSystem.h"
@@ -753,7 +754,9 @@ void RewriteSystem::freeze() {
 }
 
 void RewriteSystem::computeConflictDiagnostics(
-    SmallVectorImpl<RequirementError> &errors, SourceLoc signatureLoc) {
+    SmallVectorImpl<RequirementError> &errors, SourceLoc signatureLoc,
+    const PropertyMap &propertyMap,
+    TypeArrayView<GenericTypeParamType> genericParams) {
   for (auto pair : ConflictingRules) {
     auto *firstRule = &getRule(pair.first);
     auto *secondRule = &getRule(pair.second);
@@ -763,6 +766,48 @@ void RewriteSystem::computeConflictDiagnostics(
     if (!firstProperty || !secondProperty)
       continue;
 
+    MutableTerm firstTerm(
+        firstRule->getLHS().begin(), firstRule->getLHS().end() - 1);
+    MutableTerm secondTerm(
+        secondRule->getLHS().begin(), secondRule->getLHS().end() - 1);
+
+    auto firstSubject = propertyMap.getTypeForTerm(firstTerm, genericParams);
+    auto secondSubject = propertyMap.getTypeForTerm(secondTerm, genericParams);
+    assert(firstSubject && secondSubject);
+
+    // Record conflicting requirements on a type parameter, e.g.
+    // conflicting superclass requirements:
+    //
+    //   class C1 {}
+    //   class C2 {}
+    //   protocol P { associatedtype A: C1 }
+    //   func conflict<T: P>(_: T) where T.A: C2 {}
+    if (firstProperty->getKind() == secondProperty->getKind() &&
+        firstTerm.back().getKind() != Symbol::Kind::Name &&
+        firstSubject->isEqual(secondSubject)) {
+      switch (firstProperty->getKind()) {
+      case Symbol::Kind::ConcreteType:
+        errors.push_back(RequirementError::forConflictingRequirement(firstSubject,
+            {RequirementKind::SameType, firstProperty->getConcreteType(),
+             secondProperty->getConcreteType()},
+            signatureLoc));
+        continue;
+
+      case Symbol::Kind::Superclass:
+        // FIXME: shoving the conflicting superclass types into a superclass
+        // requiement is a little gross.
+        errors.push_back(RequirementError::forConflictingRequirement(firstSubject,
+            {RequirementKind::Superclass, firstProperty->getConcreteType(),
+             secondProperty->getConcreteType()},
+            signatureLoc));
+        continue;
+
+      // FIXME: Conflicting layout requirements?
+      default:
+        continue;
+      }
+    }
+
     auto recordError = [&](Symbol subject, Symbol constraint) {
       auto subjectType = subject.getConcreteType();
       switch (constraint.getKind()) {
@@ -802,19 +847,6 @@ void RewriteSystem::computeConflictDiagnostics(
       recordError(*firstProperty, *secondProperty);
     } else if (secondProperty->getKind() == Symbol::Kind::ConcreteType) {
       recordError(*secondProperty, *firstProperty);
-    } else {
-      // FIXME: This can happen when there are conflicting requirements
-      // on a type parameter, e.g. conflicting superclass requirements:
-      //
-      //   class C1 {}
-      //   class C2 {}
-      //   protocol P { associatedtype A: C1 }
-      //   func conflict<T: P>(_: T) where T.A: C2 {}
-      //
-      // In this case, we want to compute the type `T.A` from
-      // its corresponding term. For this, we need the property map
-      // from the RequirementMachine.
-      continue;
     }
   }
 }

lib/AST/RequirementMachine/RewriteSystem.h

@@ -230,7 +230,9 @@ class RewriteSystem final {
   void computeRedundantRequirementDiagnostics(SmallVectorImpl<RequirementError> &errors);
 
   void computeConflictDiagnostics(SmallVectorImpl<RequirementError> &errors,
-                                  SourceLoc signatureLoc);
+                                  SourceLoc signatureLoc,
+                                  const PropertyMap &map,
+                                  TypeArrayView<GenericTypeParamType> genericParams);
 
 private:
   struct CriticalPair {

test/Generics/requirement_machine_diagnostics.swift

@@ -233,10 +233,12 @@ func testMissingRequirements() {
   func conflict2<T: RequiresConformance>(_: T) where T.A == C {}
   // expected-error@-1{{same-type constraint type 'C' does not conform to required protocol 'P'}}
 
-  // FIXME: Diagnose conflicting superclass requirements on T.A
   class C {}
   func conflict3<T: RequiresSuperclass>(_: T) where T.A == C {}
+  // expected-error@-1{{type 'T.A' cannot be a subclass of both 'Super' and 'C'}}
+
   func conflict4<T: RequiresSuperclass>(_: T) where T.A: C {}
+  // expected-error@-1{{type 'T.A' cannot be a subclass of both 'Super' and 'C'}}
 }
 
 protocol Fooable {
@@ -262,15 +264,15 @@ func sameTypeConflicts() {
     var bar: Y { return Y() }
   }
 
-  // expected-error@+1{{generic signature requires types 'Y' and 'X' to be the same}}
+  // expected-error@+1{{generic parameter 'T.Foo' cannot be equal to both 'Y' and 'X'}}
   func fail1<
     T: Fooable, U: Fooable
   >(_ t: T, u: U) -> (X, Y)
     where T.Foo == X, U.Foo == Y, T.Foo == U.Foo {
     fatalError()
   }
 
-  // expected-error@+1{{generic signature requires types 'X' and 'Y' to be the same}}
+  // expected-error@+1{{generic parameter 'T.Foo' cannot be equal to both 'X' and 'Y'}}
   func fail2<
     T: Fooable, U: Fooable
   >(_ t: T, u: U) -> (X, Y)
@@ -284,15 +286,15 @@ func sameTypeConflicts() {
     fatalError()
   }
 
-  // expected-error@+1{{generic signature requires types 'Z' and 'X' to be the same}}
+  // expected-error@+1{{generic parameter 'T.Bar.Foo' cannot be equal to both 'Z' and 'X'}}
   func fail4<T: Barrable>(_ t: T) -> (Y, Z)
     where
     T.Bar == Y,
     T.Bar.Foo == Z {
     fatalError()
   }
 
-  // expected-error@+1{{generic signature requires types 'Z' and 'X' to be the same}}
+  // expected-error@+1{{generic parameter 'T.Bar.Foo' cannot be equal to both 'Z' and 'X'}}
   func fail5<T: Barrable>(_ t: T) -> (Y, Z)
     where
     T.Bar.Foo == Z,