Merge pull request #26677 from xedin/dedup-generic-requirements

[Diagnostics] Correctly identify location of requirement failure

Author: xedin

lib/Sema/CSDiagnostics.cpp

@@ -373,8 +373,36 @@ ValueDecl *RequirementFailure::getDeclRef() const {
   if (isFromContextualType())
     return getAffectedDeclFromType(cs.getContextualType());
 
-  if (auto overload = getChoiceFor(getRawAnchor()))
-    return overload->choice.getDecl();
+  if (auto overload = getChoiceFor(getRawAnchor())) {
+    // If there is a declaration associated with this
+    // failure e.g. an overload choice of the call
+    // expression, let's see whether failure is
+    // associated with it directly or rather with
+    // one of its parents.
+    if (auto *decl = overload->choice.getDeclOrNull()) {
+      auto *DC = decl->getDeclContext();
+
+      do {
+        if (auto *parent = DC->getAsDecl()) {
+          if (auto *GC = parent->getAsGenericContext()) {
+            if (GC->getGenericSignature() != Signature)
+              continue;
+
+            // If this is a signature if an extension
+            // then it means that code has referenced
+            // something incorrectly and diagnostic
+            // should point to the referenced declaration.
+            if (isa<ExtensionDecl>(parent))
+              break;
+
+            return cast<ValueDecl>(parent);
+          }
+        }
+      } while ((DC = DC->getParent()));
+
+      return decl;
+    }
+  }
 
   return getAffectedDeclFromType(getOwnerType());
 }

lib/Sema/CSDiagnostics.h

@@ -304,31 +304,12 @@ class RequirementFailure : public FailureDiagnostic {
     if (isConditional())
       return true;
 
-    auto *anchor = getAnchor();
-    // In the situations like this:
-    //
-    // ```swift
-    // enum E<T: P> { case foo(T) }
-    // let _: E = .foo(...)
-    // ```
-    //
-    // `E` is going to be opened twice. First, when
-    // it's used as a contextual type, and when `E.foo`
-    // is found and its function type is opened.
-    // We still want to record both fixes but should
-    // avoid diagnosing the same problem multiple times.
-    if (isa<UnresolvedMemberExpr>(anchor)) {
-      auto path = getLocator()->getPath();
-      if (path.front().getKind() != ConstraintLocator::UnresolvedMember)
-        return false;
-    }
-
     // For static/initializer calls there is going to be
     // a separate fix, attached to the argument, which is
     // much easier to diagnose.
     // For operator calls we can't currently produce a good
     // diagnostic, so instead let's refer to expression diagnostics.
-    return !(Apply && (isOperator(Apply) || isa<TypeExpr>(anchor)));
+    return !(Apply && isOperator(Apply));
   }
 
   static bool isOperator(const ApplyExpr *apply) {

lib/Sema/CSSimplify.cpp

@@ -2502,8 +2502,14 @@ bool ConstraintSystem::repairFailures(
     if (lhs->hasDependentMember() || rhs->hasDependentMember())
       break;
 
-    if (auto *fix = fixRequirementFailure(*this, lhs, rhs, anchor, path))
+    auto reqKind = static_cast<RequirementKind>(elt.getValue2());
+    if (hasFixedRequirement(lhs, reqKind, rhs))
+      return true;
+
+    if (auto *fix = fixRequirementFailure(*this, lhs, rhs, anchor, path)) {
+      recordFixedRequirement(lhs, reqKind, rhs);
       conversionsOrFixes.push_back(fix);
+    }
     break;
   }
 
@@ -3775,6 +3781,11 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyConformsToConstraint(
   if (type->isVoid() || type->isUninhabited())
     return SolutionKind::Error;
 
+  auto protocolTy = protocol->getDeclaredType();
+  // If this conformance has been fixed already, let's just consider this done.
+  if (hasFixedRequirement(type, RequirementKind::Conformance, protocolTy))
+    return SolutionKind::Solved;
+
   // If this is a generic requirement let's try to record that
   // conformance is missing and consider this a success, which
   // makes it much easier to diagnose problems like that.
@@ -3787,10 +3798,14 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyConformsToConstraint(
 
     if (path.back().isTypeParameterRequirement() ||
         path.back().isConditionalRequirement()) {
-      if (auto *fix = fixRequirementFailure(
-              *this, type, protocol->getDeclaredType(), anchor, path)) {
-        if (!recordFix(fix))
+      if (auto *fix =
+              fixRequirementFailure(*this, type, protocolTy, anchor, path)) {
+        if (!recordFix(fix)) {
+          // Record this conformance requirement as "fixed".
+          recordFixedRequirement(type, RequirementKind::Conformance,
+                                 protocolTy);
           return SolutionKind::Solved;
+        }
       }
     }
 

lib/Sema/CSSolver.cpp

@@ -452,6 +452,7 @@ ConstraintSystem::SolverScope::SolverScope(ConstraintSystem &cs)
   numSavedBindings = cs.solverState->savedBindings.size();
   numConstraintRestrictions = cs.ConstraintRestrictions.size();
   numFixes = cs.Fixes.size();
+  numFixedRequirements = cs.FixedRequirements.size();
   numDisjunctionChoices = cs.DisjunctionChoices.size();
   numOpenedTypes = cs.OpenedTypes.size();
   numOpenedExistentialTypes = cs.OpenedExistentialTypes.size();
@@ -505,6 +506,10 @@ ConstraintSystem::SolverScope::~SolverScope() {
   // Remove any opened types.
   truncate(cs.OpenedTypes, numOpenedTypes);
 
+  // Remove any conformances solver had to fix along
+  // the current path.
+  truncate(cs.FixedRequirements, numFixedRequirements);
+
   // Remove any opened existential types.
   truncate(cs.OpenedExistentialTypes, numOpenedExistentialTypes);
 

lib/Sema/ConstraintSystem.h

@@ -1119,6 +1119,23 @@ class ConstraintSystem {
   SmallVector<std::pair<ConstraintLocator *, ArrayRef<OpenedType>>, 4>
     OpenedTypes;
 
+  /// The list of all generic requirements fixed along the current
+  /// solver path.
+  using FixedRequirement = std::tuple<TypeBase *, RequirementKind, TypeBase *>;
+  SmallVector<FixedRequirement, 4> FixedRequirements;
+
+  bool hasFixedRequirement(Type lhs, RequirementKind kind, Type rhs) {
+    auto reqInfo = std::make_tuple(lhs.getPointer(), kind, rhs.getPointer());
+    return llvm::any_of(
+        FixedRequirements,
+        [&reqInfo](const FixedRequirement &entry) { return entry == reqInfo; });
+  }
+
+  void recordFixedRequirement(Type lhs, RequirementKind kind, Type rhs) {
+    FixedRequirements.push_back(
+        std::make_tuple(lhs.getPointer(), kind, rhs.getPointer()));
+  }
+
   /// A mapping from constraint locators to the opened existential archetype
   /// used for the 'self' of an existential type.
   SmallVector<std::pair<ConstraintLocator *, OpenedArchetypeType *>, 4>
@@ -1614,6 +1631,9 @@ class ConstraintSystem {
     /// The length of \c Fixes.
     unsigned numFixes;
 
+    /// The length of \c FixedRequirements.
+    unsigned numFixedRequirements;
+
     /// The length of \c DisjunctionChoices.
     unsigned numDisjunctionChoices;
 

test/Constraints/generics.swift

@@ -335,14 +335,14 @@ func testFixIts() {
   _ = AnyClassAndProtoBound() // expected-error {{generic parameter 'Foo' could not be inferred}} expected-note {{explicitly specify the generic arguments to fix this issue}} {{28-28=<<#Foo: SubProto & AnyObject#>>}}
   _ = AnyClassAndProtoBound2() // expected-error {{generic parameter 'Foo' could not be inferred}} expected-note {{explicitly specify the generic arguments to fix this issue}} {{29-29=<<#Foo: SubProto & AnyObject#>>}}
 
-  _ = ClassAndProtoBound() // expected-error {{referencing initializer 'init()' on 'ClassAndProtoBound' requires that 'X' conform to 'SubProto'}}
+  _ = ClassAndProtoBound() // expected-error {{generic struct 'ClassAndProtoBound' requires that 'X' conform to 'SubProto'}}
 
-  _ = ClassAndProtosBound() 
-  // expected-error@-1 {{referencing initializer 'init()' on 'ClassAndProtosBound' requires that 'X' conform to 'NSCopyish'}}
-  // expected-error@-2 {{referencing initializer 'init()' on 'ClassAndProtosBound' requires that 'X' conform to 'SubProto'}}
+  _ = ClassAndProtosBound()
+  // expected-error@-1 {{generic struct 'ClassAndProtosBound' requires that 'X' conform to 'SubProto'}}
+  // expected-error@-2 {{generic struct 'ClassAndProtosBound' requires that 'X' conform to 'NSCopyish'}}
   _ = ClassAndProtosBound2()
-  // expected-error@-1 {{referencing initializer 'init()' on 'ClassAndProtosBound2' requires that 'X' conform to 'NSCopyish'}}
-  // expected-error@-2 {{referencing initializer 'init()' on 'ClassAndProtosBound2' requires that 'X' conform to 'SubProto'}}
+  // expected-error@-1 {{generic struct 'ClassAndProtosBound2' requires that 'X' conform to 'SubProto'}}
+  // expected-error@-2 {{generic struct 'ClassAndProtosBound2' requires that 'X' conform to 'NSCopyish'}}
 
   _ = Pair()
   // expected-error@-1 {{generic parameter 'T' could not be inferred}}
@@ -631,16 +631,16 @@ func rdar40537858() {
   }
 
   var arr: [S] = []
-  _ = List(arr, id: \.id) // expected-error {{referencing initializer 'init(_:id:)' on 'List' requires that 'S.Id' conform to 'Hashable'}}
+  _ = List(arr, id: \.id) // expected-error {{generic struct 'List' requires that 'S.Id' conform to 'Hashable'}}
 
   enum E<T: P> { // expected-note 2 {{where 'T' = 'S'}}
     case foo(T)
     case bar([T])
   }
 
   var s = S(id: S.Id())
-  let _: E = .foo(s)   // expected-error {{enum case 'foo' requires that 'S' conform to 'P'}}
-  let _: E = .bar([s]) // expected-error {{enum case 'bar' requires that 'S' conform to 'P'}}
+  let _: E = .foo(s)   // expected-error {{generic enum 'E' requires that 'S' conform to 'P'}}
+  let _: E = .bar([s]) // expected-error {{generic enum 'E' requires that 'S' conform to 'P'}}
 }
 
 // https://bugs.swift.org/browse/SR-8934
@@ -798,3 +798,20 @@ struct R_51413254 {
     self.str = try anyDict ==> Key("a") // Ok
   }
 }
+
+func test_correct_identification_of_requirement_source() {
+  struct X<T: P> { // expected-note {{where 'T' = 'Int'}}
+    init<U: P>(_: T, _: U) {} // expected-note {{where 'U' = 'Int'}}
+  }
+
+  struct A : P {
+    static func foo(_ arg: A) -> A {
+      return A()
+    }
+  }
+
+  _ = X(17, A())
+  // expected-error@-1 {{generic struct 'X' requires that 'Int' conform to 'P'}}
+  _ = X(A(), 17)
+  // expected-error@-1 {{initializer 'init(_:_:)' requires that 'Int' conform to 'P'}}
+}

test/Constraints/rdar44569159.swift

@@ -6,13 +6,12 @@ struct S<V> where V: P { // expected-note {{where 'V' = 'Double'}}
 }
 
 struct A {
-  subscript<T>(_ v: S<T>) -> A { // expected-note {{where 'T' = 'Double'}}
+  subscript<T>(_ v: S<T>) -> A {
     fatalError()
   }
 }
 
 func foo(_ v: Double) {
   _ = A()[S(value: v)]
-// expected-error@-1 {{subscript 'subscript(_:)' requires that 'Double' conform to 'P'}}
-// expected-error@-2 {{referencing initializer 'init(value:)' on 'S' requires that 'Double' conform to 'P'}}
+ // expected-error@-1 {{generic struct 'S' requires that 'Double' conform to 'P'}}
 }

test/Sema/diag_ambiguous_overloads.swift

@@ -94,6 +94,8 @@ class sr7440_Genre {
     return sr7440_Genre.fetch(genreID: iTunesGenre.genreID, name: iTunesGenre.name)
 // expected-error@-1 {{value of type 'sr7440_ITunesGenre' has no member 'genreID'; did you mean 'genre'?}}
 // expected-error@-2 {{cannot convert return expression of type '()' to return type 'sr7440_Genre'}}
+// expected-error@-3 {{protocol type 'Any' cannot conform to 'BinaryInteger' because only concrete types can conform to protocols}}
+// TODO(diagnostics): Last diagnostic should not be recorded but to be able to correctly handle it we need a notion of a "hole" in constraint system.
   }
 }
 

test/decl/nested/type_in_type.swift

@@ -405,7 +405,7 @@ func test() {
   // expected-error@-1 {{generic parameter 'A' could not be inferred}}
   _ = Claws.Fangs<NotADog>()
   // expected-error@-1 {{generic parameter 'A' could not be inferred}}
-  // expected-error@-2 {{referencing initializer 'init()' on 'Claws.Fangs' requires that 'NotADog' conform to 'ExpressibleByDogLiteral'}}
+  // expected-error@-2 {{generic struct 'Fangs' requires that 'NotADog' conform to 'ExpressibleByDogLiteral'}}
   // expected-note@-3 {{explicitly specify the generic arguments to fix this issue}} {{12-12=<<#A: ExpressibleByCatLiteral#>>}}
 }
 

test/expr/unary/keypath/salvage-with-other-type-errors.swift

@@ -4,9 +4,10 @@
 // to diagnose other errors in adjacent exprs.
 
 struct P<T: K> { }
+// expected-note@-1 {{arguments to generic parameter 'T' ('String' and '_') are expected to be equal}}
 
 struct S {
-    init<B>(_ a: P<B>) { // expected-note {{where 'B' = 'String'}}
+    init<B>(_ a: P<B>) {
         fatalError()
     }
 }
@@ -17,7 +18,7 @@ func + <Object>(lhs: KeyPath<A, Object>, rhs: String) -> P<Object> {
     fatalError()
 }
 
-// expected-error@+1{{}}
+// expected-error@+1{{type 'String' does not conform to protocol 'K'}}
 func + (lhs: KeyPath<A, String>, rhs: String) -> P<String> {
     fatalError()
 }
@@ -27,7 +28,7 @@ struct A {
 }
 
 extension A: K {
-    static let j = S(\A.id + "id") // expected-error {{initializer 'init(_:)' requires that 'String' conform to 'K'}}
+    static let j = S(\A.id + "id") // expected-error {{cannot convert value of type 'P<String>' to expected argument type 'P<_>'}}
 }
 
 // SR-5034