Merge pull request #64036 from xedin/rdar-106054263

[CSSimplify] Detect and diagnose generic argument mismatches individually

Author: xedin

include/swift/Sema/CSFix.h

@@ -1103,6 +1103,10 @@ class GenericArgumentsMismatch final
     return {getTrailingObjects<unsigned>(), NumMismatches};
   }
 
+  bool coalesceAndDiagnose(const Solution &solution,
+                           ArrayRef<ConstraintFix *> secondaryFixes,
+                           bool asNote = false) const override;
+
   bool diagnose(const Solution &solution, bool asNote = false) const override;
 
   static GenericArgumentsMismatch *create(ConstraintSystem &cs, Type actual,
@@ -1115,6 +1119,9 @@ class GenericArgumentsMismatch final
   }
 
 private:
+  bool diagnose(const Solution &solution, ArrayRef<unsigned> mismatches,
+                bool asNote = false) const;
+
   MutableArrayRef<unsigned> getMismatchesBuf() {
     return {getTrailingObjects<unsigned>(), NumMismatches};
   }

include/swift/Sema/ConstraintLocator.h

@@ -643,6 +643,20 @@ class LocatorPathElt::TupleType : public StoredPointerElement<TypeBase> {
   }
 };
 
+class LocatorPathElt::GenericType : public StoredPointerElement<TypeBase> {
+public:
+  GenericType(Type type)
+      : StoredPointerElement(PathElementKind::GenericType, type.getPointer()) {
+    assert(type->getDesugaredType()->is<BoundGenericType>());
+  }
+
+  Type getType() const { return getStoredPointer(); }
+
+  static bool classof(const LocatorPathElt *elt) {
+    return elt->getKind() == PathElementKind::GenericType;
+  }
+};
+
 /// Abstract superclass for any kind of tuple element.
 class LocatorPathElt::AnyTupleElement : public StoredIntegerElement<1> {
 protected:

include/swift/Sema/ConstraintLocatorPathElts.def

@@ -181,6 +181,10 @@ CUSTOM_LOCATOR_PATH_ELT(SynthesizedArgument)
 /// path components.
 CUSTOM_LOCATOR_PATH_ELT(TupleType)
 
+/// A generic type, which provides context for subsequent generic
+/// argument path components.
+CUSTOM_LOCATOR_PATH_ELT(GenericType)
+
 /// Tuple elements.
 ABSTRACT_LOCATOR_PATH_ELT(AnyTupleElement)
   /// A tuple element referenced by position.

include/swift/Sema/ConstraintSystem.h

@@ -4655,6 +4655,10 @@ class ConstraintSystem {
     /// Indicates that we are attempting a possible type for
     /// a type variable.
     TMF_BindingTypeVariable = 0x04,
+
+    /// Indicates that the solver is matching one of the
+    /// generic argument pairs as part of matching two generic types.
+    TMF_MatchingGenericArguments = 0x08,
   };
 
   /// Options that govern how type matching should proceed.
@@ -5257,6 +5261,7 @@ class ConstraintSystem {
   /// \return true if at least some of the failures has been repaired
   /// successfully, which allows type matcher to continue.
   bool repairFailures(Type lhs, Type rhs, ConstraintKind matchKind,
+                      TypeMatchOptions flags,
                       SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
                       ConstraintLocatorBuilder locator);
 

lib/Sema/CSFix.cpp

@@ -732,10 +732,31 @@ AllowFunctionTypeMismatch::create(ConstraintSystem &cs, Type lhs, Type rhs,
       AllowFunctionTypeMismatch(cs, lhs, rhs, locator, index);
 }
 
+bool GenericArgumentsMismatch::coalesceAndDiagnose(
+    const Solution &solution, ArrayRef<ConstraintFix *> secondaryFixes,
+    bool asNote) const {
+  std::set<unsigned> scratch(getMismatches().begin(), getMismatches().end());
+
+  for (auto *fix : secondaryFixes) {
+    auto *genericArgsFix = fix->castTo<GenericArgumentsMismatch>();
+    for (auto mismatchIdx : genericArgsFix->getMismatches())
+      scratch.insert(mismatchIdx);
+  }
+
+  SmallVector<unsigned> mismatches(scratch.begin(), scratch.end());
+  return diagnose(solution, mismatches, asNote);
+}
+
+bool GenericArgumentsMismatch::diagnose(const Solution &solution,
+                                        bool asNote) const {
+  return diagnose(solution, getMismatches(), asNote);
+}
+
 bool GenericArgumentsMismatch::diagnose(const Solution &solution,
+                                        ArrayRef<unsigned> mismatches,
                                         bool asNote) const {
   GenericArgumentsMismatchFailure failure(solution, getFromType(), getToType(),
-                                          getMismatches(), getLocator());
+                                          mismatches, getLocator());
   return failure.diagnose(asNote);
 }
 

lib/Sema/CSSimplify.cpp

@@ -3748,15 +3748,23 @@ ConstraintSystem::matchDeepEqualityTypes(Type type1, Type type2,
   // Match up the generic arguments, exactly.
 
   if (shouldAttemptFixes()) {
+    auto *baseLoc =
+      getConstraintLocator(locator, {LocatorPathElt::GenericType(bound1),
+          LocatorPathElt::GenericType(bound2)});
+
+    auto argMatchingFlags =
+      subflags | TMF_ApplyingFix | TMF_MatchingGenericArguments;
+
     // Optionals have a lot of special diagnostics and only one
     // generic argument so if we' re dealing with one, don't produce generic
     // arguments mismatch fixes.
     if (bound1->getDecl()->isOptionalDecl())
-      return matchDeepTypeArguments(*this, subflags, args1, args2, locator);
+      return matchDeepTypeArguments(*this, argMatchingFlags, args1, args2,
+                                    baseLoc);
 
     SmallVector<unsigned, 4> mismatches;
     auto result = matchDeepTypeArguments(
-        *this, subflags | TMF_ApplyingFix, args1, args2, locator,
+        *this, argMatchingFlags, args1, args2, baseLoc,
         [&mismatches](unsigned position) { mismatches.push_back(position); });
 
     if (mismatches.empty())
@@ -4843,7 +4851,7 @@ static bool repairOutOfOrderArgumentsInBinaryFunction(
 /// \return true if at least some of the failures has been repaired
 /// successfully, which allows type matcher to continue.
 bool ConstraintSystem::repairFailures(
-    Type lhs, Type rhs, ConstraintKind matchKind,
+    Type lhs, Type rhs, ConstraintKind matchKind, TypeMatchOptions flags,
     SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
     ConstraintLocatorBuilder locator) {
   SmallVector<LocatorPathElt, 4> path;
@@ -5344,7 +5352,7 @@ bool ConstraintSystem::repairFailures(
       // let's re-attempt to repair without l-value conversion in the
       // locator to fix underlying type mismatch.
       if (path.back().is<LocatorPathElt::FunctionResult>()) {
-        return repairFailures(lhs, rhs, matchKind, conversionsOrFixes,
+        return repairFailures(lhs, rhs, matchKind, flags, conversionsOrFixes,
                               getConstraintLocator(anchor, path));
       }
 
@@ -6133,7 +6141,7 @@ bool ConstraintSystem::repairFailures(
     if (!path.empty() && path.back().is<LocatorPathElt::PackType>())
       path.pop_back();
 
-    return repairFailures(lhs, rhs, matchKind, conversionsOrFixes,
+    return repairFailures(lhs, rhs, matchKind, flags, conversionsOrFixes,
                           getConstraintLocator(anchor, path));
   }
 
@@ -6369,19 +6377,70 @@ bool ConstraintSystem::repairFailures(
     // failure e.g. `String bind T.Element`, so let's drop the generic argument
     // path element and recurse in repairFailures to check and potentially
     // record the requirement failure fix.
-    path.pop_back();
+    auto genericArgElt =
+        path.pop_back_val().castTo<LocatorPathElt::GenericArgument>();
 
     // If we have something like ... -> type req # -> pack element #, we're
     // solving a requirement of the form T : P where T is a type parameter pack
     if (!path.empty() && path.back().is<LocatorPathElt::PackElement>())
       path.pop_back();
 
     if (!path.empty() && path.back().is<LocatorPathElt::AnyRequirement>()) {
-      return repairFailures(lhs, rhs, matchKind, conversionsOrFixes,
+      return repairFailures(lhs, rhs, matchKind, flags, conversionsOrFixes,
                             getConstraintLocator(anchor, path));
     }
 
-    break;
+    // When the solver sets `TMF_MatchingGenericArguments` it means
+    // that it's matching generic argument pairs to identify any mismatches
+    // as part of larger matching of two generic types. Letting this
+    // fail results in a single fix that aggregates all mismatch locations.
+    //
+    // Types are not always resolved enough to enable that which means
+    // that the comparison should be delayed, which brings us here - a
+    // standalone constraint that represents such a match, in such cases
+    // we create a fix per mismatch location and coalesce them during
+    // diagnostics.
+    if (flags.contains(TMF_MatchingGenericArguments))
+      break;
+
+    Type fromType;
+    Type toType;
+
+    if (path.size() >= 2) {
+      if (path[path.size() - 2].is<LocatorPathElt::GenericType>()) {
+        fromType = path[path.size() - 2]
+                       .castTo<LocatorPathElt::GenericType>()
+                       .getType();
+      }
+
+      if (path[path.size() - 1].is<LocatorPathElt::GenericType>()) {
+        toType = path[path.size() - 1]
+                     .castTo<LocatorPathElt::GenericType>()
+                     .getType();
+      }
+    }
+
+    if (!fromType || !toType)
+      break;
+
+    // Drop both `GenericType` elements.
+    path.pop_back();
+    path.pop_back();
+
+    ConstraintFix *fix = nullptr;
+    if (!path.empty() && path.back().is<LocatorPathElt::AnyRequirement>()) {
+      fix = fixRequirementFailure(*this, fromType, toType, anchor, path);
+    } else {
+      fix = GenericArgumentsMismatch::create(
+          *this, fromType, toType, {genericArgElt.getIndex()},
+          getConstraintLocator(anchor, path));
+    }
+
+    if (!fix)
+      break;
+
+    conversionsOrFixes.push_back(fix);
+    return true;
   }
 
   case ConstraintLocator::ResultBuilderBodyResult: {
@@ -7478,7 +7537,8 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
   // Attempt fixes iff it's allowed, both types are concrete and
   // we are not in the middle of attempting one already.
   if (shouldAttemptFixes() && !flags.contains(TMF_ApplyingFix)) {
-    if (repairFailures(type1, type2, kind, conversionsOrFixes, locator)) {
+    if (repairFailures(type1, type2, kind, flags, conversionsOrFixes,
+                       locator)) {
       if (conversionsOrFixes.empty())
         return getTypeMatchSuccess();
     }
@@ -14038,7 +14098,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::MustBeCopyable:
   case FixKind::AllowInvalidPackElement:
   case FixKind::MacroMissingPound:
-  case FixKind::AllowGlobalActorMismatch: {
+  case FixKind::AllowGlobalActorMismatch:
+  case FixKind::GenericArgumentsMismatch: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
   case FixKind::IgnoreInvalidASTNode: {
@@ -14258,7 +14319,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::TreatKeyPathSubscriptIndexAsHashable:
   case FixKind::AllowInvalidRefInKeyPath:
   case FixKind::DefaultGenericArgument:
-  case FixKind::GenericArgumentsMismatch:
   case FixKind::AllowMutatingMemberOnRValueBase:
   case FixKind::AllowTupleSplatForSingleParameter:
   case FixKind::AllowNonClassTypeToConvertToAnyObject:

lib/Sema/ConstraintLocator.cpp

@@ -67,6 +67,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
   case ConstraintLocator::GenericParameter:
   case ConstraintLocator::GenericArgument:
   case ConstraintLocator::TupleType:
+  case ConstraintLocator::GenericType:
   case ConstraintLocator::NamedTupleElement:
   case ConstraintLocator::TupleElement:
   case ConstraintLocator::ProtocolRequirement:
@@ -242,6 +243,12 @@ void LocatorPathElt::dump(raw_ostream &out) const {
     break;
   }
 
+  case ConstraintLocator::GenericType: {
+    auto genericTyElt = elt.castTo<LocatorPathElt::GenericType>();
+    out << "generic type '" << genericTyElt.getType()->getString(PO) << "'";
+    break;
+  }
+
   case ConstraintLocator::NamedTupleElement: {
     auto tupleElt = elt.castTo<LocatorPathElt::NamedTupleElement>();
     out << "named tuple element #" << llvm::utostr(tupleElt.getIndex());

lib/Sema/ConstraintSystem.cpp

@@ -5393,6 +5393,7 @@ void constraints::simplifyLocator(ASTNode &anchor,
       continue;
 
    case ConstraintLocator::TupleType:
+   case ConstraintLocator::GenericType:
       path = path.slice(1);
       continue;
 

test/Constraints/generics.swift

@@ -1031,3 +1031,27 @@ func test_requirement_failures_in_ambiguous_context() {
 
   f3(A()) // expected-error {{no exact matches in call to local function 'f3'}}
 }
+
+// rdar://106054263 - failed to produce a diagnostic upon generic argument mismatch
+func test_mismatches_with_dependent_member_generic_arguments() {
+  struct Binding<T, U> {}
+  // expected-note@-1 {{arguments to generic parameter 'T' ('Double?' and 'Data.SomeAssociated') are expected to be equal}}
+  // expected-note@-2 {{arguments to generic parameter 'U' ('Int' and 'Data.SomeAssociated') are expected to be equal}}
+
+  struct Data : SomeProtocol {
+    typealias SomeAssociated = String
+  }
+
+  func test1<T: SomeProtocol>(_: Binding<T.SomeAssociated, T.SomeAssociated>, _: T) where T.SomeAssociated == String {
+  }
+
+  func test2<T: SomeProtocol>(_: Optional<T.SomeAssociated>, _: T) where T.SomeAssociated == String {
+  }
+
+  test1(Binding<Double?, Int>(), Data())
+  // expected-error@-1 {{cannot convert value of type 'Binding<Double?, Int>' to expected argument type 'Binding<Data.SomeAssociated, Data.SomeAssociated>'}}
+
+  test2(Optional<Int>(nil), Data())
+  // expected-error@-1 {{cannot convert value of type 'Optional<Int>' to expected argument type 'Optional<Data.SomeAssociated>'}}
+  // expected-note@-2 {{arguments to generic parameter 'Wrapped' ('Int' and 'Data.SomeAssociated') are expected to be equal}}
+}

test/Generics/issue-55666.swift

@@ -6,19 +6,19 @@ struct W<T> {}
 
 struct S<C1: Collection> {
   init(){}
-  // expected-note@+2 {{where 'C1.Element' = 'String', 'W<C2.Element>' = 'Int'}}
-  // expected-note@+1 {{where 'C1.Element' = 'C1', 'W<C2.Element>' = 'C2.Element'}}
+  // expected-note@+2 {{where 'C1.Element' = 'W<C1>', 'main.W<C2.Element>' = 'main.W<C2.Element>'}}
+  // expected-note@+1 {{where 'C1.Element' = 'W<String>', 'W<C2.Element>' = 'W<Array<Int>.Element>'}}
   init<C2>(_ c2: W<C2>) where C2: Collection, C1.Element == W<C2.Element> {}
-  // expected-note@+1 {{where 'C1.Element' = 'String', 'W<C2.Element>' = 'Int'}}
+  // expected-note@+1 {{where 'C1.Element' = 'W<String>', 'W<C2.Element>' = 'W<Array<Int>.Element>'}}
   static func f<C2>(_ c2: W<C2>) where C2: Collection, C1.Element == W<C2.Element> {}
-  // expected-note@+1 {{where 'C1.Element' = 'String', 'W<C2.Element>' = 'Int'}}
+  // expected-note@+1 {{where 'C1.Element' = 'W<String>', 'W<C2.Element>' = 'W<Array<Int>.Element>'}}
   func instancef<C2>(_ c2: W<C2>) where C2: Collection, C1.Element == W<C2.Element> {}
 }
-let _ = S<[W<String>]>(W<[Int]>()) // expected-error{{initializer 'init(_:)' requires the types 'String' and 'Int' be equivalent}}
-let _ = S<[W<String>]>.f(W<[Int]>()) // expected-error{{static method 'f' requires the types 'String' and 'Int' be equivalent}}
-let _ = S<[W<String>]>().instancef(W<[Int]>()) // expected-error{{instance method 'instancef' requires the types 'String' and 'Int' be equivalent}}
+let _ = S<[W<String>]>(W<[Int]>()) // expected-error{{initializer 'init(_:)' requires the types 'W<String>' and 'W<Array<Int>.Element>' be equivalent}}
+let _ = S<[W<String>]>.f(W<[Int]>()) // expected-error{{static method 'f' requires the types 'W<String>' and 'W<Array<Int>.Element>' be equivalent}}
+let _ = S<[W<String>]>().instancef(W<[Int]>()) // expected-error{{instance method 'instancef' requires the types 'W<String>' and 'W<Array<Int>.Element>' be equivalent}}
 
 // Archetypes requirement failure
 func genericFunc<C1: Collection, C2: Collection>(_ c2: W<C2>, c1: C1.Type) where C1.Element == W<C2.Element> {
-  let _ = S<[W<C1>]>(W<C2>()) // expected-error{{initializer 'init(_:)' requires the types 'C1' and 'C2.Element' be equivalent}}
+  let _ = S<[W<C1>]>(W<C2>()) // expected-error{{initializer 'init(_:)' requires the types 'W<C1>' and 'W<C2.Element>' be equivalent}}
 }