Merge pull request #19920 from gregomni/8757

[Sema][QoI] Call out missing conformances in protocol witness candidates.

Author: gregomni

include/swift/AST/DiagnosticsSema.def

@@ -1770,9 +1770,18 @@ NOTE(bad_associated_type_deduction,none,
      "unable to infer associated type %0 for protocol %1",
      (DeclName, DeclName))
 NOTE(associated_type_deduction_witness_failed,none,
-     "inferred type %1 (by matching requirement %0) is invalid: "
-     "does not %select{inherit from|conform to}3 %2",
+     "candidate would match and infer %0 = %1 if %1 "
+     "%select{inherited from|conformed to}3 %2",
      (DeclName, Type, Type, bool))
+NOTE(associated_type_witness_conform_impossible,none,
+     "candidate can not infer %0 = %1 because %1 "
+     "is not a nominal type and so can't conform to %2",
+     (DeclName, Type, Type))
+NOTE(associated_type_witness_inherit_impossible,none,
+     "candidate can not infer %0 = %1 because %1 "
+     "is not a class type and so can't inherit from %2",
+     (DeclName, Type, Type))
+
 NOTE(ambiguous_associated_type_deduction,none,
      "ambiguous inference of associated type %0: %1 vs. %2",
      (DeclName, Type, Type))
@@ -1793,6 +1802,9 @@ NOTE(protocol_witness_kind_conflict,none,
      "a subscript}0", (RequirementKind))
 NOTE(protocol_witness_type_conflict,none,
      "candidate has non-matching type %0%1", (Type, StringRef))
+NOTE(protocol_witness_missing_requirement,none,
+     "candidate would match if %0 %select{conformed to|subclassed|"
+     "was the same type as}2 %1", (Type, Type, unsigned))
 
 NOTE(protocol_witness_optionality_conflict,none,
      "candidate %select{type has|result type has|parameter type has|"

lib/Sema/CSFix.h

@@ -75,6 +75,11 @@ enum class FixKind : uint8_t {
   /// Skip same-type generic requirement constraint,
   /// and assume that types are equal.
   SkipSameTypeRequirement,
+
+  /// Skip superclass generic requirement constraint,
+  /// and assume that types are related.
+  SkipSuperclassRequirement,
+
 };
 
 class ConstraintFix {
@@ -288,6 +293,10 @@ class MissingConformance final : public ConstraintFix {
   static MissingConformance *create(ConstraintSystem &cs, Type type,
                                     ProtocolDecl *protocol,
                                     ConstraintLocator *locator);
+
+  Type getNonConformingType() { return NonConformingType; }
+
+  ProtocolDecl *getProtocol() { return Protocol; }
 };
 
 /// Skip same-type generic requirement constraint,
@@ -307,6 +316,9 @@ class SkipSameTypeRequirement final : public ConstraintFix {
 
   bool diagnose(Expr *root, bool asNote = false) const override;
 
+  Type lhsType() { return LHS; }
+  Type rhsType() { return RHS; }
+
   static SkipSameTypeRequirement *create(ConstraintSystem &cs, Type lhs,
                                          Type rhs, ConstraintLocator *locator);
 };
@@ -318,8 +330,8 @@ class SkipSuperclassRequirement final : public ConstraintFix {
 
   SkipSuperclassRequirement(ConstraintSystem &cs, Type lhs, Type rhs,
                             ConstraintLocator *locator)
-      : ConstraintFix(cs, FixKind::SkipSameTypeRequirement, locator), LHS(lhs),
-        RHS(rhs) {}
+      : ConstraintFix(cs, FixKind::SkipSuperclassRequirement, locator),
+        LHS(lhs), RHS(rhs) {}
 
 public:
   std::string getName() const override {
@@ -328,6 +340,9 @@ class SkipSuperclassRequirement final : public ConstraintFix {
 
   bool diagnose(Expr *root, bool asNote = false) const override;
 
+  Type subclassType() { return LHS; }
+  Type superclassType() { return RHS; }
+
   static SkipSuperclassRequirement *
   create(ConstraintSystem &cs, Type lhs, Type rhs, ConstraintLocator *locator);
 };

lib/Sema/CSSimplify.cpp

@@ -5046,7 +5046,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
     return result;
   }
 
-  case FixKind::SkipSameTypeRequirement: {
+  case FixKind::SkipSameTypeRequirement:
+  case FixKind::SkipSuperclassRequirement: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
 

lib/Sema/CSSolver.cpp

@@ -560,10 +560,17 @@ ConstraintSystem::SolverScope::~SolverScope() {
 /// \returns a solution if a single unambiguous one could be found, or None if
 /// ambiguous or unsolvable.
 Optional<Solution>
-ConstraintSystem::solveSingle(FreeTypeVariableBinding allowFreeTypeVariables) {
+ConstraintSystem::solveSingle(FreeTypeVariableBinding allowFreeTypeVariables,
+                              bool allowFixes) {
+
+  SolverState state(nullptr, *this, allowFreeTypeVariables);
+  state.recordFixes = allowFixes;
+
   SmallVector<Solution, 4> solutions;
-  if (solve(nullptr, solutions, allowFreeTypeVariables) ||
-      solutions.size() != 1)
+  solve(solutions);
+  filterSolutions(solutions, state.ExprWeights);
+
+  if (solutions.size() != 1)
     return Optional<Solution>();
 
   return std::move(solutions[0]);

lib/Sema/ConstraintSystem.h

@@ -3053,10 +3053,13 @@ class ConstraintSystem {
   /// \param allowFreeTypeVariables How to bind free type variables in
   /// the solution.
   ///
+  /// \param allowFixes Whether to allow fixes in the solution.
+  ///
   /// \returns a solution if a single unambiguous one could be found, or None if
   /// ambiguous or unsolvable.
   Optional<Solution> solveSingle(FreeTypeVariableBinding allowFreeTypeVariables
-                                    = FreeTypeVariableBinding::Disallow);
+                                 = FreeTypeVariableBinding::Disallow,
+                                 bool allowFixes = false);
 
 private:
   /// \brief Solve the system of constraints.

lib/Sema/TypeCheckProtocol.cpp

@@ -686,6 +686,69 @@ static const RequirementEnvironment &getOrCreateRequirementEnvironment(
   return cacheIter->getSecond();
 }
 
+static Optional<RequirementMatch> findMissingGenericRequirementForSolutionFix(
+    constraints::ConstraintFix *fix, ValueDecl *witness,
+    ProtocolConformance *conformance,
+    const RequirementEnvironment &reqEnvironment) {
+  Type type, missingType;
+  RequirementKind requirementKind;
+
+  using namespace constraints;
+
+  switch (fix->getKind()) {
+  case FixKind::AddConformance: {
+    auto missingConform = (MissingConformance *)fix;
+    requirementKind = RequirementKind::Conformance;
+    type = missingConform->getNonConformingType();
+    missingType = missingConform->getProtocol()->getDeclaredType();
+    break;
+  }
+  case FixKind::SkipSameTypeRequirement: {
+    requirementKind = RequirementKind::SameType;
+    auto requirementFix = (SkipSameTypeRequirement *)fix;
+    type = requirementFix->lhsType();
+    missingType = requirementFix->rhsType();
+    break;
+  }
+  case FixKind::SkipSuperclassRequirement: {
+    requirementKind = RequirementKind::Superclass;
+    auto requirementFix = (SkipSuperclassRequirement *)fix;
+    type = requirementFix->subclassType();
+    missingType = requirementFix->superclassType();
+    break;
+  }
+  default:
+    return Optional<RequirementMatch>();
+  }
+
+  auto missingRequirementMatch = [&](Type type) -> RequirementMatch {
+    Requirement requirement(requirementKind, type, missingType);
+    return RequirementMatch(witness, MatchKind::MissingRequirement,
+                            requirement);
+  };
+
+  if (auto memberTy = type->getAs<DependentMemberType>())
+    return missingRequirementMatch(type);
+
+  type = type->mapTypeOutOfContext();
+  if (type->hasTypeParameter())
+    if (auto env = conformance->getGenericEnvironment())
+      if (auto assocType = env->mapTypeIntoContext(type))
+        return missingRequirementMatch(assocType);
+
+  auto reqSubMap = reqEnvironment.getRequirementToSyntheticMap();
+  auto proto = conformance->getProtocol();
+  Type selfTy = proto->getSelfInterfaceType().subst(reqSubMap);
+  if (type->isEqual(selfTy)) {
+    type = conformance->getType();
+    if (auto agt = type->getAs<AnyGenericType>())
+      type = agt->getDecl()->getDeclaredInterfaceType();
+    return missingRequirementMatch(type);
+  }
+
+  return Optional<RequirementMatch>();
+}
+
 RequirementMatch
 swift::matchWitness(TypeChecker &tc,
                     WitnessChecker::RequirementEnvironmentCache &reqEnvCache,
@@ -766,7 +829,7 @@ swift::matchWitness(TypeChecker &tc,
   auto setup = [&]() -> std::tuple<Optional<RequirementMatch>, Type, Type> {
     // Construct a constraint system to use to solve the equality between
     // the required type and the witness type.
-    cs.emplace(tc, dc, ConstraintSystemOptions());
+    cs.emplace(tc, dc, ConstraintSystemFlags::AllowFixes);
 
     auto reqGenericEnv = reqEnvironment.getSyntheticEnvironment();
     auto reqSubMap = reqEnvironment.getRequirementToSyntheticMap();
@@ -848,8 +911,19 @@ swift::matchWitness(TypeChecker &tc,
     // Try to solve the system disallowing free type variables, because
     // that would resolve in incorrect substitution matching when witness
     // type has free type variables present as well.
-    auto solution = cs->solveSingle(FreeTypeVariableBinding::Disallow);
-    if (!solution)
+    auto solution = cs->solveSingle(FreeTypeVariableBinding::Disallow,
+                                    /* allowFixes */ true);
+
+    // If the types would match but for some other missing conformance, find and
+    // call that out.
+    if (solution && conformance && solution->Fixes.size()) {
+      for (auto fix : solution->Fixes) {
+        if (auto result = findMissingGenericRequirementForSolutionFix(
+                fix, witness, conformance, reqEnvironment))
+          return *result;
+      }
+    }
+    if (!solution || solution->Fixes.size())
       return RequirementMatch(witness, MatchKind::TypeConflict,
                               witnessType);
 
@@ -2032,6 +2106,12 @@ diagnoseMatch(ModuleDecl *module, NormalProtocolConformance *conformance,
     break;
   }
 
+  case MatchKind::MissingRequirement:
+    diags.diagnose(match.Witness, diag::protocol_witness_missing_requirement,
+                   match.WitnessType, match.MissingRequirement->getSecondType(),
+                   (unsigned)match.MissingRequirement->getKind());
+    break;
+
   case MatchKind::ThrowsConflict:
     diags.diagnose(match.Witness, diag::protocol_witness_throws_conflict);
     break;

lib/Sema/TypeCheckProtocol.h

@@ -79,6 +79,9 @@ class CheckTypeWitnessResult {
   bool isConformanceRequirement() const {
     return Requirement->isExistentialType();
   }
+  bool isSuperclassRequirement() const {
+    return !isConformanceRequirement();
+  }
   bool isError() const {
     return Requirement->is<ErrorType>();
   }
@@ -168,6 +171,9 @@ enum class MatchKind : uint8_t {
   /// \brief The types conflict.
   TypeConflict,
 
+  /// \brief The witness would match if an additional requirement were met.
+  MissingRequirement,
+
   /// The witness throws, but the requirement does not.
   ThrowsConflict,
 
@@ -352,6 +358,17 @@ struct RequirementMatch {
            "Should (or should not) have witness type");
   }
 
+  RequirementMatch(ValueDecl *witness, MatchKind kind, Requirement requirement,
+                   Optional<RequirementEnvironment> env = None,
+                   ArrayRef<OptionalAdjustment> optionalAdjustments = {})
+      : Witness(witness), Kind(kind), WitnessType(requirement.getFirstType()),
+        MissingRequirement(requirement), ReqEnv(std::move(env)),
+        OptionalAdjustments(optionalAdjustments.begin(),
+                            optionalAdjustments.end()) {
+    assert(hasWitnessType() && hasRequirement() &&
+           "Should have witness type and requirement");
+  }
+
   /// \brief The witness that matches the (implied) requirement.
   ValueDecl *Witness;
 
@@ -361,6 +378,9 @@ struct RequirementMatch {
   /// \brief The type of the witness when it is referenced.
   Type WitnessType;
 
+  /// \brief Requirement not met.
+  Optional<Requirement> MissingRequirement;
+
   /// \brief The requirement environment to use for the witness thunk.
   Optional<RequirementEnvironment> ReqEnv;
 
@@ -382,6 +402,7 @@ struct RequirementMatch {
     case MatchKind::WitnessInvalid:
     case MatchKind::KindConflict:
     case MatchKind::TypeConflict:
+    case MatchKind::MissingRequirement:
     case MatchKind::StaticNonStaticConflict:
     case MatchKind::SettableConflict:
     case MatchKind::PrefixNonPrefixConflict:
@@ -404,6 +425,7 @@ struct RequirementMatch {
     case MatchKind::ExactMatch:
     case MatchKind::RenamedMatch:
     case MatchKind::TypeConflict:
+    case MatchKind::MissingRequirement:
     case MatchKind::OptionalityConflict:
       return true;
 
@@ -425,6 +447,9 @@ struct RequirementMatch {
     llvm_unreachable("Unhandled MatchKind in switch.");
   }
 
+  /// \brief Determine whether this requirement match has a requirement.
+  bool hasRequirement() { return Kind == MatchKind::MissingRequirement; }
+
   swift::Witness getWitness(ASTContext &ctx) const;
 };
 

lib/Sema/TypeCheckProtocolInference.cpp

@@ -1747,9 +1747,27 @@ bool AssociatedTypeInference::diagnoseNoSolutions(
           if (failed.Result.isError())
             continue;
 
+          if ((!failed.TypeWitness->getAnyNominal() ||
+               failed.TypeWitness->isExistentialType()) &&
+              failed.Result.isConformanceRequirement()) {
+            diags.diagnose(failed.Witness,
+                           diag::associated_type_witness_conform_impossible,
+                           assocType->getName(), failed.TypeWitness,
+                           failed.Result.getRequirement());
+            continue;
+          }
+          if (!failed.TypeWitness->getClassOrBoundGenericClass() &&
+              failed.Result.isSuperclassRequirement()) {
+            diags.diagnose(failed.Witness,
+                           diag::associated_type_witness_inherit_impossible,
+                           assocType->getName(), failed.TypeWitness,
+                           failed.Result.getRequirement());
+            continue;
+          }
+
           diags.diagnose(failed.Witness,
                          diag::associated_type_deduction_witness_failed,
-                         failed.Requirement->getFullName(),
+                         assocType->getName(),
                          failed.TypeWitness,
                          failed.Result.getRequirement(),
                          failed.Result.isConformanceRequirement());

test/Generics/associated_types_inherit.swift

@@ -19,7 +19,7 @@ struct X1 : P {
 }
 
 struct X2 : P { // expected-error{{type 'X2' does not conform to protocol 'P'}}
-  func getAssoc() -> E { return E() } // expected-note{{inferred type 'E' (by matching requirement 'getAssoc()') is invalid: does not inherit from 'C'}}
+  func getAssoc() -> E { return E() } // expected-note{{candidate would match and infer 'Assoc' = 'E' if 'E' inherited from 'C'}}
 }
 
 func testP<T:P>(_ t: T) {

test/decl/ext/protocol_as_witness.swift

@@ -8,8 +8,7 @@ protocol P1 {
 protocol Q1 {}
 
 extension P1 where Self : Q1 {
-  // FIXME: Poor QoI
-  func f() {} // expected-note{{candidate has non-matching type '<Self> () -> ()'}}
+  func f() {} // expected-note{{candidate would match if 'X1' conformed to 'Q1'}}
 }
 
 struct X1 : P1 {} // expected-error{{type 'X1' does not conform to protocol 'P1'}}
@@ -33,7 +32,7 @@ protocol P3 {
 }
 
 extension P3 where Self : Equatable {
-  func f() {} // expected-note{{candidate has non-matching type '<Self> () -> ()'}}
+  func f() {} // expected-note{{candidate would match if 'X3' conformed to 'Equatable'}}
 }
 
 struct X3 : P3 {} // expected-error{{type 'X3' does not conform to protocol 'P3'}}

test/decl/protocol/conforms/self.swift

@@ -61,12 +61,7 @@ class SeriousClass {}
 
 extension HasDefault where Self : SeriousClass {
   func foo() {}
-  // expected-note@-1 {{candidate has non-matching type '<Self> () -> ()'}}
-
-  // FIXME: the above diangostic is from trying to check conformance for
-  // 'SillyClass' and not 'SeriousClass'. Evidently name lookup finds members
-  // from all constrained extensions, and then if any don't have a matching
-  // generic signature, diagnostics doesn't really know what to do about it.
+  // expected-note@-1 {{candidate would match if 'SillyClass' subclassed 'SeriousClass'}}
 }
 
 extension SeriousClass : HasDefault {}