[GenericSigBuilder] Track all same-type-to-concrete constraints.

When we see a second same-type-to-concrete constraint on a particular
potential archetype, record it. Previously, we were checking it and
then updating the requirement source eagerly. That won't work with
proper recursion detection, and meant that we missed out on some
obvious redundant-same-type-constraint diagnostics.

The scheme here is to build up the equivalence classes without losing
any information, and then determine which information is redundant at
the end.

Author: DougGregor

include/swift/AST/GenericSignatureBuilder.h

@@ -736,13 +736,15 @@ class GenericSignatureBuilder::PotentialArchetype {
   llvm::MapVector<Identifier, llvm::TinyPtrVector<PotentialArchetype *>>
     NestedTypes;
 
-  /// The concrete type to which a this potential archetype has been
+  /// The concrete types to which this potential archetype has been
   /// constrained.
-  Type ConcreteType;
+  ///
+  /// This vector runs parallel to ConcreteTypeSources.
+  llvm::TinyPtrVector<Type> concreteTypes;
 
-  /// The source of the concrete type requirement, if one was written
-  /// on this potential archetype.
-  const RequirementSource *ConcreteTypeSource = nullptr;
+  /// The source of the concrete type requirements that were written on
+  /// this potential archetype.
+  llvm::TinyPtrVector<const RequirementSource *> concreteTypeSources;
 
   /// Whether this is an unresolved nested type.
   unsigned isUnresolvedNestedType : 1;
@@ -971,15 +973,21 @@ class GenericSignatureBuilder::PotentialArchetype {
                             SameTypeConstraints.end());
   }
 
-  /// Retrieve the concrete type source as written on this potential archetype.
-  const RequirementSource *getConcreteTypeSourceAsWritten() const {
-    return ConcreteTypeSource;
+  /// Retrieve the concrete types as written on this potential archetype.
+  const llvm::TinyPtrVector<Type>& getConcreteTypesAsWritten() const {
+    return concreteTypes;
+  }
+
+  /// Retrieve the concrete type sources as written on this potential archetype.
+  ArrayRef<const RequirementSource *> getConcreteTypeSourcesAsWritten() const {
+    return concreteTypeSources;
   }
 
   /// Find a source of the same-type constraint that maps this potential
   /// archetype to a concrete type somewhere in the equivalence class of this
-  /// type.
-  const RequirementSource *findAnyConcreteTypeSourceAsWritten() const;
+  /// type along with the concrete type that was written there.
+  Optional<std::pair<Type, const RequirementSource *>>
+  findAnyConcreteTypeSourceAsWritten() const;
 
   /// \brief Retrieve (or create) a nested type with the given name.
   PotentialArchetype *getNestedType(Identifier Name,

lib/AST/GenericSignatureBuilder.cpp

@@ -501,24 +501,38 @@ void GenericSignatureBuilder::PotentialArchetype::resolveAssociatedType(
   --builder.Impl->NumUnresolvedNestedTypes;
 }
 
-const RequirementSource *
+Optional<std::pair<Type, const RequirementSource *>>
 PotentialArchetype::findAnyConcreteTypeSourceAsWritten() const {
-  // If we have a concrete type source, use that.
-  if (ConcreteTypeSource && ConcreteTypeSource->getLoc().isValid())
-    return ConcreteTypeSource;
+  using Result = std::pair<Type, const RequirementSource *>;
+
+  // Local function to look for a source in the given potential archetype.
+  auto lookInPA = [](const PotentialArchetype *pa) -> Optional<Result> {
+    for (unsigned i : indices(pa->concreteTypeSources)) {
+      auto source = pa->concreteTypeSources[i];
+      if (source->getLoc().isValid())
+        return Result(pa->concreteTypes[i], source);
+    }
+
+    return None;
+  };
+
+  // If we have a concrete type source with location information, use that.
+  if (auto result = lookInPA(this))
+    return result;
 
   // If we don't have a concrete type, there's no source.
   auto rep = getRepresentative();
-  if (!rep->isConcreteType()) return nullptr;
+  if (!rep->isConcreteType()) return None;
 
   // Otherwise, go look for the source.
   for (auto pa : rep->getEquivalenceClassMembers()) {
-    if (pa->ConcreteTypeSource &&
-        pa->ConcreteTypeSource->getLoc().isValid())
-      return pa->ConcreteTypeSource;
+    if (pa != this) {
+      if (auto result = lookInPA(pa))
+        return result;
+    }
   }
 
-  return nullptr;
+  return None;
 }
 
 bool GenericSignatureBuilder::updateRequirementSource(
@@ -589,7 +603,7 @@ struct GenericSignatureBuilder::ResolvedType {
 
   static ResolvedType forNewTypeAlias(PotentialArchetype *pa) {
     assert(pa->getParent() && pa->getTypeAliasDecl() &&
-           pa->ConcreteType.isNull() &&
+           pa->concreteTypes.empty() &&
            pa->getEquivalenceClassMembers().size() == 1 &&
            "not a new typealias");
     return ResolvedType(pa);
@@ -1340,17 +1354,18 @@ void GenericSignatureBuilder::PotentialArchetype::dump(llvm::raw_ostream &Out,
   }
 
   // Print concrete type.
-  if (ConcreteType) {
+  for (unsigned i : indices(concreteTypes)) {
+    auto concreteType = concreteTypes[i];
     Out << " == ";
-    ConcreteType.print(Out);
-    if (ConcreteTypeSource) {
-      Out << " ";
-      if (!ConcreteTypeSource->isDerivedRequirement())
-        Out << "*";
-      Out << "[";
-      ConcreteTypeSource->print(Out, SrcMgr);
-      Out << "]";
-    }
+    concreteType.print(Out);
+
+    auto concreteTypeSource = concreteTypeSources[i];
+    Out << " ";
+    if (!concreteTypeSource->isDerivedRequirement())
+      Out << "*";
+    Out << "[";
+    concreteTypeSource->print(Out, SrcMgr);
+    Out << "]";
   }
 
   // Print requirements.
@@ -1673,10 +1688,10 @@ bool GenericSignatureBuilder::addSuperclassRequirement(PotentialArchetype *T,
     Type concrete = T->getConcreteType();
     if (!Superclass->isExactSuperclassOf(concrete, getLazyResolver())) {
       if (auto source = T->findAnyConcreteTypeSourceAsWritten()) {
-        Diags.diagnose(source->getLoc(), diag::type_does_not_inherit,
+        Diags.diagnose(source->second->getLoc(), diag::type_does_not_inherit,
                        T->getDependentType(/*FIXME:*/{ },
                                            /*allowUnresolved=*/true),
-                       concrete, Superclass)
+                       source->first, Superclass)
           .highlight(Source->getLoc());
       }
       return true;
@@ -1912,11 +1927,8 @@ bool GenericSignatureBuilder::addSameTypeRequirementToConcrete(
        Type Concrete,
        const RequirementSource *Source) {
   // Record the concrete type and its source.
-  if (!T->ConcreteType) {
-    // FIXME: Always record this.
-    T->ConcreteType = Concrete;
-    T->ConcreteTypeSource = Source;
-  }
+  T->concreteTypes.push_back(Concrete);
+  T->concreteTypeSources.push_back(Source);
 
   // If we've already been bound to a type, match that type.
   if (auto oldConcrete = T->getConcreteType()) {
@@ -2540,11 +2552,11 @@ GenericSignatureBuilder::finalize(SourceLoc loc,
       // Check for recursive same-type bindings.
       if (isRecursiveConcreteType(archetype, /*isSuperclass=*/false)) {
         if (auto source = archetype->findAnyConcreteTypeSourceAsWritten()) {
-          Diags.diagnose(source->getLoc(),
+          Diags.diagnose(source->second->getLoc(),
                          diag::recursive_same_type_constraint,
                          archetype->getDependentType(genericParams,
                                                      /*allowUnresolved=*/true),
-                         archetype->getConcreteType());
+                         source->first);
         }
 
         archetype->RecursiveConcreteType = true;
@@ -2590,7 +2602,7 @@ GenericSignatureBuilder::finalize(SourceLoc loc,
       // because then we don't actually have a parameter.
       if (rep->getConcreteType()) {
         if (auto source = rep->findAnyConcreteTypeSourceAsWritten())
-          Diags.diagnose(source->getLoc(),
+          Diags.diagnose(source->second->getLoc(),
                          diag::requires_generic_param_made_equal_to_concrete,
                          rep->getDependentType(genericParams,
                                                /*allowUnresolved=*/true));
@@ -2718,46 +2730,49 @@ void GenericSignatureBuilder::checkRedundantConcreteTypeConstraints(
   // Gather the concrete constraints within this equivalence class.
   SmallVector<ConcreteConstraint, 4> concreteConstraints;
   for (auto pa : representative->getEquivalenceClassMembers()) {
-    auto source = pa->getConcreteTypeSourceAsWritten();
-    if (!source) continue;
-
-    // Save this constraint.
-    auto constraint = ConcreteConstraint{pa, pa->ConcreteType, source};
-    concreteConstraints.push_back(constraint);
-
-    // Check whether this constraint is better than the best we've seen so far
-    // at being the representative constraint against which others will be
-    // compared.
-    if (!representativeConstraint) {
-      representativeConstraint = constraint;
-      continue;
-    }
-
-    // We prefer derived constraints to non-derived constraints.
-    bool thisIsDerived = source->isDerivedRequirement();
-    bool representativeIsDerived =
-      representativeConstraint->source->isDerivedRequirement();
-    if (thisIsDerived != representativeIsDerived) {
-      if (thisIsDerived)
+    auto types = pa->getConcreteTypesAsWritten();
+    auto sources = pa->getConcreteTypeSourcesAsWritten();
+    for (unsigned i : indices(types)) {
+      auto source = sources[i];
+
+      // Save this constraint.
+      auto constraint = ConcreteConstraint{pa, types[i], source};
+      concreteConstraints.push_back(constraint);
+
+      // Check whether this constraint is better than the best we've seen so far
+      // at being the representative constraint against which others will be
+      // compared.
+      if (!representativeConstraint) {
         representativeConstraint = constraint;
+        continue;
+      }
 
-      continue;
-    }
+      // We prefer derived constraints to non-derived constraints.
+      bool thisIsDerived = source->isDerivedRequirement();
+      bool representativeIsDerived =
+        representativeConstraint->source->isDerivedRequirement();
+      if (thisIsDerived != representativeIsDerived) {
+        if (thisIsDerived)
+          representativeConstraint = constraint;
 
-    // We prefer constraints with locations to constraints without locations.
-    bool thisHasValidSourceLoc = source->getLoc().isValid();
-    bool representativeHasValidSourceLoc =
-      representativeConstraint->source->getLoc().isValid();
-    if (thisHasValidSourceLoc != representativeHasValidSourceLoc) {
-      if (thisHasValidSourceLoc)
-        representativeConstraint = constraint;
+        continue;
+      }
 
-      continue;
-    }
+      // We prefer constraints with locations to constraints without locations.
+      bool thisHasValidSourceLoc = source->getLoc().isValid();
+      bool representativeHasValidSourceLoc =
+        representativeConstraint->source->getLoc().isValid();
+      if (thisHasValidSourceLoc != representativeHasValidSourceLoc) {
+        if (thisHasValidSourceLoc)
+          representativeConstraint = constraint;
 
-    // Otherwise, order via the constraint itself.
-    if (constraint < *representativeConstraint)
-      representativeConstraint = constraint;
+        continue;
+      }
+
+      // Otherwise, order via the constraint itself.
+      if (constraint < *representativeConstraint)
+        representativeConstraint = constraint;
+    }
   }
 
   // Sort the concrete constraints, so we get deterministic ordering of
@@ -2935,20 +2950,25 @@ static SmallVector<SameTypeComponent, 2> getSameTypeComponents(
 
     // Find the best anchor and concrete type source for this component.
     PotentialArchetype *anchor = component[0];
-    auto bestConcreteTypeSource = anchor->getConcreteTypeSourceAsWritten();
+    const RequirementSource *bestConcreteTypeSource = nullptr;
+    auto considerNewSource = [&](const RequirementSource *source) {
+      if (!bestConcreteTypeSource ||
+          source->compare(bestConcreteTypeSource) < 0)
+        bestConcreteTypeSource = source;
+    };
 
-    for (auto componentPA : ArrayRef<PotentialArchetype *>(component).slice(1)){
+    if (!anchor->getConcreteTypeSourcesAsWritten().empty())
+      bestConcreteTypeSource = anchor->getConcreteTypeSourcesAsWritten()[0];
+
+    for (auto componentPA : ArrayRef<PotentialArchetype *>(component)) {
       // Update the anchor.
       if (compareDependentTypes(&componentPA, &anchor) < 0)
         anchor = componentPA;
 
       // If this potential archetype has a better concrete type source than
       // the best we've seen, take it.
-      if (auto concreteSource = componentPA->getConcreteTypeSourceAsWritten()) {
-        if (!bestConcreteTypeSource ||
-            concreteSource->compare(bestConcreteTypeSource) < 0)
-          bestConcreteTypeSource = concreteSource;
-      }
+      for (auto source: componentPA->getConcreteTypeSourcesAsWritten())
+        considerNewSource(source);
     }
 
     // Record the anchor.

test/Constraints/same_types.swift

@@ -87,6 +87,7 @@ func test6<T: Barrable>(_ t: T) -> (Y, X) where T.Bar == Y {
 }
 
 func test7<T: Barrable>(_ t: T) -> (Y, X) where T.Bar == Y, T.Bar.Foo == X {
+	// expected-warning@-1{{redundant same-type constraint 'T.Bar.Foo' == 'X'}}
   return (t.bar, t.bar.foo)
 }
 
@@ -99,8 +100,9 @@ func fail4<T: Barrable>(_ t: T) -> (Y, Z)
 
 func fail5<T: Barrable>(_ t: T) -> (Y, Z)
   where
-  T.Bar.Foo == Z,
+  T.Bar.Foo == Z, // expected-warning{{redundant same-type constraint 'T.Bar.Foo' == 'Z'}}
   T.Bar == Y { // expected-error{{associated type 'T.Bar.Foo' cannot be equal to both 'Z' and 'X'}}
+	// expected-note@-1{{same-type constraint 'T.Bar.Foo' == 'Y.Foo' (aka 'X') implied here}}
   return (t.bar, t.bar.foo) // expected-error{{cannot convert return expression of type 'X' to return type 'Z'}}
 }