Merge pull request #1319 from gregomni/generic-func-args

[Sema] Extend callee diagnosis to multiple generics

Author: slavapestov

lib/Sema/CSDiag.cpp

@@ -987,18 +987,14 @@ static bool argumentMismatchIsNearMiss(Type argType, Type paramType) {
 /// and equal fixed type portions, and return by reference each archetype and
 /// the matching portion of the actual arg type where that archetype appears.
 static bool findGenericSubstitutions(DeclContext *dc, Type paramType, Type actualArgType,
-                                     SmallVector<ArchetypeType *, 4> &archetypes,
-                                     SmallVector<Type, 4> &substitutions) {
+                                     TypeSubstitutionMap &archetypesMap) {
   class GenericVisitor : public TypeMatcher<GenericVisitor> {
     DeclContext *dc;
-    SmallVector<ArchetypeType *, 4> &archetypes;
-    SmallVector<Type, 4> &substitutions;
+    TypeSubstitutionMap &archetypesMap;
 
   public:
-    GenericVisitor(DeclContext *dc,
-                   SmallVector<ArchetypeType *, 4> &archetypes,
-                   SmallVector<Type, 4> &substitutions)
-    : dc(dc), archetypes(archetypes), substitutions(substitutions) {}
+    GenericVisitor(DeclContext *dc, TypeSubstitutionMap &archetypesMap)
+      : dc(dc), archetypesMap(archetypesMap) {}
 
     bool mismatch(TypeBase *paramType, TypeBase *argType) {
       return paramType->isEqual(argType);
@@ -1010,12 +1006,10 @@ static bool findGenericSubstitutions(DeclContext *dc, Type paramType, Type actua
         type = ArchetypeBuilder::mapTypeIntoContext(dc, paramType);
 
       if (auto archetype = type->getAs<ArchetypeType>()) {
-        for (unsigned i = 0, c = archetypes.size(); i < c; i++) {
-          if (archetypes[i]->isEqual(archetype))
-            return substitutions[i]->isEqual(argType);
-        }
-        archetypes.push_back(archetype);
-        substitutions.push_back(argType);
+        auto existing = archetypesMap[archetype];
+        if (existing)
+          return existing->isEqual(argType);
+        archetypesMap[archetype] = argType;
         return true;
       }
       return false;
@@ -1030,15 +1024,22 @@ static bool findGenericSubstitutions(DeclContext *dc, Type paramType, Type actua
       if (dc && original->isTypeParameter())
         original = ArchetypeBuilder::mapTypeIntoContext(dc, original);
 
-      if (auto archetype = original->getAs<ArchetypeType>()) {
-        archetypes.push_back(archetype);
-        substitutions.push_back(substitution->getReplacementType());
-      }
+      Type replacement = substitution->getReplacementType();
+      // If the replacement is itself an archetype, then the constraint
+      // system was asserting equivalencies between different levels of
+      // generics, rather than binding a generic to a concrete type (and we
+      // don't/won't have a concrete type). In which case, it is the
+      // replacement we are interested in, since it is the one in our current
+      // context. That generic type should equal itself.
+      if (auto ourGeneric = replacement->getAs<ArchetypeType>())
+        archetypesMap[ourGeneric] = replacement;
+      else if (auto archetype = original->getAs<ArchetypeType>())
+        archetypesMap[archetype] = replacement;
     }
     return false;
   });
 
-  GenericVisitor visitor(dc, archetypes, substitutions);
+  GenericVisitor visitor(dc, archetypesMap);
   return visitor.match(paramType, actualArgType);
 }
 
@@ -1085,18 +1086,20 @@ CalleeCandidateInfo::evaluateCloseness(DeclContext *dc, Type candArgListType,
   // their type and count the number of mismatched arguments.
   unsigned mismatchingArgs = 0;
 
-  // Checking of archetypes.
-  // FIXME: For now just trying to verify applicability of arguments with only
-  // a single generic variable. Ideally we'd create a ConstraintSystem with
-  // type variables for all generics and solve it with the given argument types.
-  Type singleArchetype = nullptr;
-  Type matchingArgType = nullptr;
+  // Known mapping of archetypes in all arguments so far. An archetype may map
+  // to another archetype if the constraint system substituted one for another.
+  TypeSubstitutionMap allGenericSubstitutions;
 
-  // Number of args of generic archetype which are mismatched because
+  // Number of args of one generic archetype which are mismatched because
   // isSubstitutableFor() has failed. If all mismatches are of this type, we'll
   // return a different closeness for better diagnoses.
+  Type nonSubstitutableArchetype = nullptr;
   unsigned nonSubstitutableArgs = 0;
 
+  // The type of failure is that multiple occurrences of the same generic are
+  // being passed arguments with different concrete types.
+  bool genericWithDifferingConcreteTypes = false;
+  
   // We classify an argument mismatch as being a "near" miss if it is a very
   // likely match due to a common sort of problem (e.g. wrong flags on a
   // function type, optional where none was expected, etc).  This allows us to
@@ -1122,75 +1125,77 @@ CalleeCandidateInfo::evaluateCloseness(DeclContext *dc, Type candArgListType,
 
       // FIXME: Right now, a "matching" overload is one with a parameter whose
       // type is identical to the argument type, or substitutable via handling
-      // of functions with a single archetype in one or more parameters.
+      // of functions with primary archetypes in one or more parameters.
       // We can still do something more sophisticated with this.
       // FIXME: Use TC.isConvertibleTo?
 
-      SmallVector<ArchetypeType *, 4> archetypes;
-      SmallVector<Type, 4> substitutions;
+      TypeSubstitutionMap archetypesMap;
       bool matched;
-      if (paramType->is<UnresolvedType>())
+      if (paramType->is<UnresolvedType>() || rArgType->hasTypeVariable())
         matched = false;
       else
-        matched = findGenericSubstitutions(dc, paramType, rArgType,
-                                           archetypes, substitutions);
+        matched = findGenericSubstitutions(dc, paramType, rArgType, archetypesMap);
 
-      if (matched && archetypes.size() == 0)
-        continue;
-      if (matched && archetypes.size() == 1 && !rArgType->hasTypeVariable()) {
-        auto archetype = archetypes[0];
-        auto substitution = substitutions[0];
-        
-        if (singleArchetype) {
-          if (!archetype->isEqual(singleArchetype))
-            // Multiple archetypes, too complicated.
-            return { CC_ArgumentMismatch, {}};
+      if (matched) {
+        for (auto pair : archetypesMap) {
+          auto archetype = pair.first->castTo<ArchetypeType>();
+          auto substitution = pair.second;
 
-          if (substitution->isEqual(matchingArgType)) {
-            if (nonSubstitutableArgs == 0)
-              continue;
-            ++nonSubstitutableArgs;
-            mismatchesAreNearMisses = false;
+          auto existingSubstitution = allGenericSubstitutions[archetype];
+          if (!existingSubstitution) {
+            // New substitution for this callee.
+            allGenericSubstitutions[archetype] = substitution;
+            
+            // Not yet handling nested archetypes.
+            if (!archetype->isPrimary())
+              return { CC_ArgumentMismatch, {}};
+            
+            if (!CS->TC.isSubstitutableFor(substitution, archetype, CS->DC)) {
+              // If we have multiple non-substitutable types, this is just a mismatched mess.
+              if (!nonSubstitutableArchetype.isNull())
+                return { CC_ArgumentMismatch, {}};
+              
+              if (auto argOptType = argType->getOptionalObjectType())
+                mismatchesAreNearMisses &= CS->TC.isSubstitutableFor(argOptType, archetype, CS->DC);
+              else
+                mismatchesAreNearMisses = false;
+              
+              nonSubstitutableArchetype = archetype;
+              nonSubstitutableArgs = 1;
+              matched = false;
+            }
           } else {
-            if (nonSubstitutableArgs == 1) {
+            // Substitution for the same archetype as in a previous argument.
+            bool isNonSubstitutableArchetype = !nonSubstitutableArchetype.isNull() &&
+                                               nonSubstitutableArchetype->isEqual(archetype);
+            if (substitution->isEqual(existingSubstitution)) {
+              if (isNonSubstitutableArchetype) {
+                ++nonSubstitutableArgs;
+                matched = false;
+              }
+            } else {
               // If we have only one nonSubstitutableArg so far, then this different
               // type might be the one that we should be substituting for instead.
               // Note that failureInfo is already set correctly for that case.
-              if (CS->TC.isSubstitutableFor(substitution, archetype, CS->DC)) {
-                mismatchesAreNearMisses = argumentMismatchIsNearMiss(matchingArgType, substitution);
-                matchingArgType = substitution;
-                continue;
+              if (isNonSubstitutableArchetype && nonSubstitutableArgs == 1 &&
+                  CS->TC.isSubstitutableFor(substitution, archetype, CS->DC)) {
+                mismatchesAreNearMisses = argumentMismatchIsNearMiss(existingSubstitution, substitution);
+                allGenericSubstitutions[archetype] = substitution;
+              } else {
+                genericWithDifferingConcreteTypes = true;
+                matched = false;
               }
             }
-            
-            // This substitution doesn't match a previous substitution. Set up the nearMiss
-            // and failureInfo.paramType with the expected substitution inserted.
-            // (Note that this transform assumes only a single archetype.)
-            mismatchesAreNearMisses &= argumentMismatchIsNearMiss(substitution, matchingArgType);
-            paramType = paramType.transform(([&](Type type) -> Type {
-              if (type->is<SubstitutableType>())
-                return matchingArgType;
-              return type;
-            }));
           }
-        } else {
-          matchingArgType = substitution;
-          singleArchetype = archetype;
-
-          if (CS->TC.isSubstitutableFor(substitution, archetype, CS->DC))
-            continue;
-          
-          if (auto argOptType = argType->getOptionalObjectType())
-            mismatchesAreNearMisses &= CS->TC.isSubstitutableFor(argOptType, archetype, CS->DC);
-          else
-            mismatchesAreNearMisses = false;
-          ++nonSubstitutableArgs;
         }
-      } else {
-        // Keep track of whether this argument was a near miss or not.
-        mismatchesAreNearMisses &= argumentMismatchIsNearMiss(argType, paramType);
       }
 
+      if (matched)
+        continue;
+      
+      if (archetypesMap.empty())
+        mismatchesAreNearMisses &= argumentMismatchIsNearMiss(argType, paramType);
+      
       ++mismatchingArgs;
 
       failureInfo.argumentNumber = argNo;
@@ -1213,10 +1218,23 @@ CalleeCandidateInfo::evaluateCloseness(DeclContext *dc, Type candArgListType,
   // close matches are prioritized against obviously wrong ones.
   if (mismatchingArgs == 1) {
     CandidateCloseness closeness;
-    if (singleArchetype.isNull()) {
+    if (allGenericSubstitutions.empty()) {
       closeness = mismatchesAreNearMisses ? CC_OneArgumentNearMismatch
                                           : CC_OneArgumentMismatch;
     } else {
+      // If the failure is that different occurrences of the same generic have
+      // different concrete types, substitute in all the concrete types we've found
+      // into the failureInfo to improve diagnosis.
+      if (genericWithDifferingConcreteTypes) {
+        auto newType = failureInfo.parameterType.transform([&](Type type) -> Type {
+          if (auto archetype = type->getAs<ArchetypeType>())
+            if (auto replacement = allGenericSubstitutions[archetype])
+              return replacement;
+          return type;
+        });
+        failureInfo.parameterType = newType;
+      }
+      
       closeness = mismatchesAreNearMisses ? CC_OneGenericArgumentNearMismatch
                                           : CC_OneGenericArgumentMismatch;
     }
@@ -1589,16 +1607,15 @@ bool CalleeCandidateInfo::diagnoseGenericParameterErrors(Expr *badArgExpr) {
     return false;
 
   bool foundFailure = false;
-  SmallVector<ArchetypeType *, 4> archetypes;
-  SmallVector<Type, 4> substitutions;
+  TypeSubstitutionMap archetypesMap;
 
   if (!findGenericSubstitutions(failedArgument.declContext, failedArgument.parameterType,
-                                argType, archetypes, substitutions))
+                                argType, archetypesMap))
     return false;
 
-  for (unsigned i = 0, c = archetypes.size(); i < c; i++) {
-    auto archetype = archetypes[i];
-    auto substitution = substitutions[i];
+  for (auto pair : archetypesMap) {
+    auto archetype = pair.first->castTo<ArchetypeType>();
+    auto substitution = pair.second;
 
     // FIXME: Add specific error for not subclass, if the archetype has a superclass?
 

test/Constraints/generics.swift

@@ -14,8 +14,7 @@ func min<T : Comparable>(x: T, y: T) -> T {
 func weirdConcat<T : ConcatToAnything, U>(t: T, u: U) {
   t +++ u
   t +++ 1
-  u +++ t // expected-error{{binary operator '+++' cannot be applied to operands of type 'U' and 'T'}}
-  // expected-note @-1 {{expected an argument list of type '(Self, T)'}}
+  u +++ t // expected-error{{argument type 'U' does not conform to expected type 'ConcatToAnything'}}
 }
 
 // Make sure that the protocol operators don't get in the way.

test/Generics/deduction.swift

@@ -65,7 +65,7 @@ func takeTuples<T, U>(_: (T, U), _: (U, T)) {
 func useTuples(x: Int, y: Float, z: (Float, Int)) {
   takeTuples((x, y), (y, x))
 
-  takeTuples((x, y), (x, y)) // expected-error{{cannot convert value of type '(Int, Float)' to expected argument type '(_, _)'}}
+  takeTuples((x, y), (x, y)) // expected-error{{cannot convert value of type 'Int' to expected argument type 'Float'}}
 
   // FIXME: Use 'z', which requires us to fix our tuple-conversion
   // representation.
@@ -75,7 +75,7 @@ func acceptFunction<T, U>(f: (T) -> U, _ t: T, _ u: U) {}
 
 func passFunction(f: (Int) -> Float, x: Int, y: Float) {
    acceptFunction(f, x, y)
-   acceptFunction(f, y, y) // expected-error{{cannot convert value of type '(Int) -> Float' to expected argument type '(_) -> _'}}
+   acceptFunction(f, y, y) // expected-error{{cannot convert value of type 'Float' to expected argument type 'Int'}}
 }
 
 func returnTuple<T, U>(_: T) -> (T, U) { } // expected-note {{in call to function 'returnTuple'}}

test/Generics/function_defs.swift

@@ -75,8 +75,8 @@ protocol Overload {
   associatedtype B
   func getA() -> A
   func getB() -> B
-  func f1(_: A) -> A // expected-note{{found this candidate}}
-  func f1(_: B) -> B // expected-note{{found this candidate}}
+  func f1(_: A) -> A
+  func f1(_: B) -> B
   func f2(_: Int) -> A // expected-note{{found this candidate}}
   func f2(_: Int) -> B // expected-note{{found this candidate}}
   func f3(_: Int) -> Int // expected-note {{found this candidate}}
@@ -106,7 +106,8 @@ func testOverload<Ovl : Overload, OtherOvl : Overload>(ovl: Ovl, ovl2: Ovl,
   a = ovl2.f2(17)
   a = ovl2.f1(a)
 
-  other.f1(a) // expected-error{{ambiguous reference to member 'f1'}}
+  other.f1(a) // expected-error{{cannot invoke 'f1' with an argument list of type '(Ovl.A)'}}
+  // expected-note @-1 {{overloads for 'f1' exist with these partially matching parameter lists: (Self.A), (Self.B)}}
 
   // Overloading based on context
   var f3i : (Int) -> Int = ovl.f3
@@ -132,15 +133,15 @@ protocol Subscriptable {
   func getIndex() -> Index
   func getValue() -> Value
 
-  subscript (index : Index) -> Value { get set } // expected-note{{found this candidate}}
+  subscript (index : Index) -> Value { get set }
 }
 
 protocol IntSubscriptable {
   associatedtype ElementType
 
   func getElement() -> ElementType
 
-  subscript (index : Int) -> ElementType { get  } // expected-note{{found this candidate}}
+  subscript (index : Int) -> ElementType { get  }
 }
 
 func subscripting<T : protocol<Subscriptable, IntSubscriptable>>(t: T) {
@@ -153,7 +154,8 @@ func subscripting<T : protocol<Subscriptable, IntSubscriptable>>(t: T) {
   element = t[17]
   t[42] = element // expected-error{{cannot assign through subscript: subscript is get-only}}
 
-  t[value] = 17 // expected-error{{ambiguous reference to member 'subscript'}}
+  // Suggests the Int form because we prefer concrete matches to generic matches in diagnosis.
+  t[value] = 17 // expected-error{{cannot convert value of type 'T.Value' to expected argument type 'Int'}}
 }
 
 //===----------------------------------------------------------------------===//

test/Generics/generic_types.swift

@@ -200,8 +200,7 @@ func useNested(ii: Int, hni: HasNested<Int>,
 
   // Generic constructor of a generic struct
   HNI(1, 2.71828) // expected-warning{{unused}}
-  // FIXME: Should report this error: {{cannot convert the expression's type 'HNI' to type 'Int'}}
-  HNI(1.5, 2.71828) // expected-error{{cannot invoke initializer for type 'HNI' with an argument list of type '(Double, Double)'}} expected-note{{expected an argument list of type '(T, U)'}}
+  HNI(1.5, 2.71828) // expected-error{{'Double' is not convertible to 'Int'}}
 
   // Generic function in a nested generic struct
   var ids = xis.g(1, u: "Hello", v: 3.14159)