Merge pull request #41971 from hborla/conflicting-requirement-diagnostics

[RequirementMachine] Diagnose conflicting requirements.

Author: hborla

include/swift/AST/DiagnosticsSema.def

@@ -2491,6 +2491,9 @@ ERROR(recursive_superclass_constraint,none,
 ERROR(requires_generic_param_same_type_does_not_conform,none,
       "same-type constraint type %0 does not conform to required protocol %1",
       (Type, Identifier))
+ERROR(same_type_does_not_inherit,none,
+      "same-type constraint type %0 does not inherit from required superclass %1",
+      (Type, Type))
 ERROR(requires_same_concrete_type,none,
       "generic signature requires types %0 and %1 to be the same", (Type, Type))
 WARNING(redundant_conformance_constraint,none,
@@ -2506,6 +2509,9 @@ WARNING(missing_protocol_refinement, none,
 ERROR(same_type_conflict,none,
       "%select{generic parameter |protocol |}0%1 cannot be equal to both "
       "%2 and %3", (unsigned, Type, Type, Type))
+ERROR(requirement_conflict,none,
+      "no type for %0 can satisfy both %1",
+      (Type, StringRef))
 WARNING(redundant_same_type_to_concrete,none,
         "redundant same-type constraint %0 == %1", (Type, Type))
 NOTE(same_type_redundancy_here,none,

lib/AST/RequirementMachine/Diagnostics.h

@@ -30,9 +30,10 @@ struct RequirementError {
   enum class Kind {
     /// A constraint to a non-protocol, non-class type, e.g. T: Int.
     InvalidTypeRequirement,
-    /// A type mismatch, e.g. Int == String.
-    ConcreteTypeMismatch,
-    /// A requirement proven to be false, e.g. Bool: Collection
+    /// A type requirement on a trivially invalid subject type,
+    /// e.g. Bool: Collection.
+    InvalidRequirementSubject,
+    /// A pair of conflicting requirements, T == Int, T == String
     ConflictingRequirement,
     /// A redundant requirement, e.g. T == T.
     RedundantRequirement,
@@ -41,11 +42,20 @@ struct RequirementError {
   /// The invalid requirement.
   Requirement requirement;
 
+  /// A requirement that conflicts with \c requirement. Both
+  /// requirements will have the same subject type.
+  Optional<Requirement> conflictingRequirement;
+
   SourceLoc loc;
 
 private:
   RequirementError(Kind kind, Requirement requirement, SourceLoc loc)
-    : kind(kind), requirement(requirement), loc(loc) {}
+    : kind(kind), requirement(requirement), conflictingRequirement(None), loc(loc) {}
+
+  RequirementError(Kind kind, Requirement requirement,
+                   Requirement conflict,
+                   SourceLoc loc)
+    : kind(kind), requirement(requirement), conflictingRequirement(conflict), loc(loc) {}
 
 public:
   static RequirementError forInvalidTypeRequirement(Type subjectType,
@@ -55,18 +65,22 @@ struct RequirementError {
     return {Kind::InvalidTypeRequirement, requirement, loc};
   }
 
-  static RequirementError forConcreteTypeMismatch(Type type1,
-                                                  Type type2,
-                                                  SourceLoc loc) {
-    Requirement requirement(RequirementKind::SameType, type1, type2);
-    return {Kind::ConcreteTypeMismatch, requirement, loc};
+  static RequirementError forInvalidRequirementSubject(Requirement req,
+                                                       SourceLoc loc) {
+    return {Kind::InvalidRequirementSubject, req, loc};
   }
 
   static RequirementError forConflictingRequirement(Requirement req,
                                                     SourceLoc loc) {
     return {Kind::ConflictingRequirement, req, loc};
   }
 
+  static RequirementError forConflictingRequirement(Requirement first,
+                                                    Requirement second,
+                                                    SourceLoc loc) {
+    return {Kind::ConflictingRequirement, first, second, loc};
+  }
+
   static RequirementError forRedundantRequirement(Requirement req,
                                                   SourceLoc loc) {
     return {Kind::RedundantRequirement, req, loc};

lib/AST/RequirementMachine/RequirementLowering.cpp

@@ -213,10 +213,8 @@ static void desugarSameTypeRequirement(Type lhs, Type rhs, SourceLoc loc,
         return true;
       }
 
-      errors.push_back(
-          RequirementError::forConcreteTypeMismatch(sugaredFirstType,
-                                                    secondType,
-                                                    loc));
+      errors.push_back(RequirementError::forConflictingRequirement(
+          {RequirementKind::SameType, sugaredFirstType, secondType}, loc));
       recordedErrors = true;
       return true;
     }
@@ -249,7 +247,7 @@ static void desugarSuperclassRequirement(Type subjectType,
           RequirementError::forRedundantRequirement(requirement, loc));
     } else {
       errors.push_back(
-          RequirementError::forConflictingRequirement(requirement, loc));
+          RequirementError::forInvalidRequirementSubject(requirement, loc));
     }
 
     return;
@@ -271,7 +269,7 @@ static void desugarLayoutRequirement(Type subjectType,
           RequirementError::forRedundantRequirement(requirement, loc));
     } else {
       errors.push_back(
-          RequirementError::forConflictingRequirement(requirement, loc));
+          RequirementError::forInvalidRequirementSubject(requirement, loc));
     }
 
     return;
@@ -295,7 +293,7 @@ static void desugarConformanceRequirement(Type subjectType, Type constraintType,
       auto *module = protoDecl->getParentModule();
       auto conformance = module->lookupConformance(subjectType, protoDecl);
       if (conformance.isInvalid()) {
-        errors.push_back(RequirementError::forConflictingRequirement(
+        errors.push_back(RequirementError::forInvalidRequirementSubject(
             {RequirementKind::Conformance, subjectType, constraintType}, loc));
         return;
       }
@@ -712,20 +710,7 @@ bool swift::rewriting::diagnoseRequirementErrors(
       break;
     }
 
-    case RequirementError::Kind::ConcreteTypeMismatch: {
-      auto type1 = error.requirement.getFirstType();
-      auto type2 = error.requirement.getSecondType();
-
-      if (!type1->hasError() && !type2->hasError()) {
-        ctx.Diags.diagnose(loc, diag::requires_same_concrete_type,
-                           type1, type2);
-        diagnosedError = true;
-      }
-
-      break;
-    }
-
-    case RequirementError::Kind::ConflictingRequirement: {
+    case RequirementError::Kind::InvalidRequirementSubject: {
       auto subjectType = error.requirement.getFirstType();
       if (subjectType->hasError())
         break;
@@ -736,6 +721,37 @@ bool swift::rewriting::diagnoseRequirementErrors(
       break;
     }
 
+    case RequirementError::Kind::ConflictingRequirement: {
+      auto requirement = error.requirement;
+      auto conflict = error.conflictingRequirement;
+
+      if (!conflict) {
+        if (requirement.getFirstType()->hasError() ||
+            requirement.getSecondType()->hasError()) {
+          // Don't emit a cascading error.
+          break;
+        }
+        ctx.Diags.diagnose(loc, diag::requires_same_concrete_type,
+                           requirement.getFirstType(),
+                           requirement.getSecondType());
+      } else {
+        auto options = PrintOptions::forDiagnosticArguments();
+        std::string requirements;
+        llvm::raw_string_ostream OS(requirements);
+        OS << "'";
+        requirement.print(OS, options);
+        OS << "' and '";
+        conflict->print(OS, options);
+        OS << "'";
+
+        ctx.Diags.diagnose(loc, diag::requirement_conflict,
+                           requirement.getFirstType(), requirements);
+      }
+
+      diagnosedError = true;
+      break;
+    }
+
     case RequirementError::Kind::RedundantRequirement: {
       auto requirement = error.requirement;
       switch (requirement.getKind()) {

lib/AST/RequirementMachine/RequirementMachine.cpp

@@ -366,7 +366,7 @@ RequirementMachine::initWithProtocolWrittenRequirements(
 
   // For RequirementMachine::verify() when called by generic signature queries;
   // We have a single valid generic parameter at depth 0, index 0.
-  Params.push_back(component[0]->getSelfInterfaceType()->getCanonicalType());
+  Params.push_back(component[0]->getSelfInterfaceType());
 
   if (Dump) {
     llvm::dbgs() << "Adding protocols";
@@ -530,6 +530,13 @@ void RequirementMachine::freeze() {
   System.freeze();
 }
 
+void RequirementMachine::computeRequirementDiagnostics(
+    SmallVectorImpl<RequirementError> &errors, SourceLoc signatureLoc) {
+  System.computeRedundantRequirementDiagnostics(errors);
+  System.computeConflictDiagnostics(errors, signatureLoc, Map,
+                                    getGenericParams());
+}
+
 std::string RequirementMachine::getRuleAsStringForDiagnostics(
     unsigned ruleID) const {
   const auto &rule = System.getRule(ruleID);

lib/AST/RequirementMachine/RequirementMachine.h

@@ -18,6 +18,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include <vector>
 
+#include "Diagnostics.h"
 #include "PropertyMap.h"
 #include "RewriteContext.h"
 #include "RewriteSystem.h"
@@ -115,6 +116,9 @@ class RequirementMachine final {
 
   void freeze();
 
+  void computeRequirementDiagnostics(SmallVectorImpl<RequirementError> &errors,
+                                     SourceLoc signatureLoc);
+
   MutableTerm getLongestValidPrefix(const MutableTerm &term) const;
 
   void buildRequirementsFromRules(

lib/AST/RequirementMachine/RequirementMachineRequests.cpp

@@ -410,7 +410,7 @@ RequirementSignatureRequestRQM::evaluate(Evaluator &evaluator,
     if (ctx.LangOpts.RequirementMachineProtocolSignatures ==
         RequirementMachineMode::Enabled) {
       SmallVector<RequirementError, 4> errors;
-      machine->System.computeRedundantRequirementDiagnostics(errors);
+      machine->computeRequirementDiagnostics(errors, proto->getLoc());
       diagnoseRequirementErrors(ctx, errors,
                                 /*allowConcreteGenericParams=*/false);
     }
@@ -843,7 +843,7 @@ InferredGenericSignatureRequestRQM::evaluate(
     if (attempt == 0 &&
         ctx.LangOpts.RequirementMachineInferredSignatures ==
         RequirementMachineMode::Enabled) {
-      machine->System.computeRedundantRequirementDiagnostics(errors);
+      machine->computeRequirementDiagnostics(errors, loc);
       diagnoseRequirementErrors(ctx, errors, allowConcreteGenericParams);
     }
 

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"
@@ -752,6 +753,76 @@ void RewriteSystem::freeze() {
   ConflictingRules.clear();
 }
 
+static Optional<Requirement>
+getRequirementForDiagnostics(Type subject, Symbol property,
+                             const PropertyMap &map,
+                             TypeArrayView<GenericTypeParamType> genericParams,
+                             const MutableTerm &prefix) {
+  switch (property.getKind()) {
+  case Symbol::Kind::ConcreteType: {
+    auto concreteType = map.getTypeFromSubstitutionSchema(
+        property.getConcreteType(), property.getSubstitutions(),
+        genericParams, prefix);
+    return Requirement(RequirementKind::SameType, subject, concreteType);
+  }
+
+  case Symbol::Kind::Superclass: {
+    auto concreteType = map.getTypeFromSubstitutionSchema(
+        property.getConcreteType(), property.getSubstitutions(),
+        genericParams, prefix);
+    return Requirement(RequirementKind::Superclass, subject, concreteType);
+  }
+
+  case Symbol::Kind::Protocol:
+    return Requirement(RequirementKind::Conformance, subject,
+                       property.getProtocol()->getDeclaredInterfaceType());
+
+  case Symbol::Kind::Layout:
+    return Requirement(RequirementKind::Layout, subject,
+                       property.getLayoutConstraint());
+
+  default:
+    return None;
+  }
+}
+
+void RewriteSystem::computeConflictDiagnostics(
+    SmallVectorImpl<RequirementError> &errors, SourceLoc signatureLoc,
+    const PropertyMap &propertyMap,
+    TypeArrayView<GenericTypeParamType> genericParams) {
+  for (auto pair : ConflictingRules) {
+    const auto &firstRule = getRule(pair.first);
+    const auto &secondRule = getRule(pair.second);
+
+    assert(firstRule.isPropertyRule() && secondRule.isPropertyRule());
+
+    if (firstRule.isSubstitutionSimplified() ||
+        secondRule.isSubstitutionSimplified())
+      continue;
+
+    bool chooseFirstRule = firstRule.getRHS().size() > secondRule.getRHS().size();
+    auto subjectRule = chooseFirstRule ? firstRule : secondRule;
+    auto subjectTerm = subjectRule.getRHS();
+
+    auto suffixRule = chooseFirstRule ? secondRule : firstRule;
+    auto suffixTerm = suffixRule.getRHS();
+
+    // If the root protocol of the subject term isn't in this minimization
+    // domain, the conflict was already diagnosed.
+    if (!isInMinimizationDomain(subjectTerm[0].getRootProtocol()))
+      continue;
+
+    Type subject = propertyMap.getTypeForTerm(subjectTerm, genericParams);
+    MutableTerm prefix(subjectTerm.begin(), subjectTerm.end() - suffixTerm.size());
+    errors.push_back(RequirementError::forConflictingRequirement(
+        *getRequirementForDiagnostics(subject, *subjectRule.isPropertyRule(),
+                                      propertyMap, genericParams, MutableTerm()),
+        *getRequirementForDiagnostics(subject, *suffixRule.isPropertyRule(),
+                                      propertyMap, genericParams, prefix),
+        signatureLoc));
+  }
+}
+
 void RewriteSystem::dump(llvm::raw_ostream &out) const {
   out << "Rewrite system: {\n";
   for (const auto &rule : Rules) {

lib/AST/RequirementMachine/RewriteSystem.h

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

test/Generics/concrete_conflict.swift

@@ -1,5 +1,4 @@
-// RUN: %target-typecheck-verify-swift
-// RUN: %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-protocol-signatures=on -requirement-machine-inferred-signatures=on 2>&1 | %FileCheck %s
+// RUN: %target-swift-frontend -typecheck -verify %s -debug-generic-signatures -requirement-machine-protocol-signatures=on -requirement-machine-inferred-signatures=on 2>&1 | %FileCheck %s
 
 protocol P1 {
   associatedtype T where T == Int
@@ -25,11 +24,11 @@ protocol P3 {
 // CHECK-LABEL: .P4@
 // CHECK-LABEL: Requirement signature: <Self where Self.[P4]T : P1, Self.[P4]T : P2, Self.[P4]T : P3>
 
+// expected-error@+3{{no type for 'Self.T.T' can satisfy both 'Self.T.T == String' and 'Self.T.T == Int'}}
+// expected-error@+2{{no type for 'Self.T.T' can satisfy both 'Self.T.T == Float' and 'Self.T.T == Int'}}
+// expected-error@+1{{no type for 'Self.T.T' can satisfy both 'Self.T.T == Float' and 'Self.T.T == String'}}
 protocol P4 {
   associatedtype T : P1 & P2 & P3
-  // expected-note@-1 2{{same-type constraint 'Self.T.T' == 'Int' implied here}}
-  // expected-error@-2 {{'Self.T.T' cannot be equal to both 'String' and 'Int'}}
-  // expected-error@-3 {{'Self.T.T' cannot be equal to both 'Float' and 'Int'}}
 }
 
 class Class<T> {}
@@ -38,5 +37,5 @@ extension Class where T == Bool {
   // CHECK-LABEL: .badRequirement()@
   // CHECK-NEXT: <T>
   func badRequirement() where T == Int { }
-  // expected-error@-1 {{generic parameter 'T' cannot be equal to both 'Int' and 'Bool'}}
-}
+  // expected-error@-1 {{no type for 'T' can satisfy both 'T == Int' and 'T == Bool'}}
+}

test/Generics/protocol_self_concrete_error.swift

@@ -7,4 +7,5 @@
 
 struct S {}
 
+// expected-error@+1 {{no type for 'Self' can satisfy both 'Self == S' and 'Self : P'}}
 protocol P where Self == S {}