Merge pull request #31862 from hamishknight/solved-holistically-5.3

[5.3] [CS] Diagnostic improvements for key paths

Author: tkremenek

include/swift/AST/DiagnosticsSema.def

@@ -71,6 +71,11 @@ ERROR(could_not_find_value_dynamic_member_corrected,none,
 ERROR(could_not_find_value_dynamic_member,none,
       "value of type %0 has no dynamic member %2 using key path from root type %1",
       (Type, Type, DeclNameRef))
+ERROR(cannot_infer_contextual_keypath_type_specify_root,none,
+      "cannot infer key path type from context; consider explicitly specifying a root type", ())
+ERROR(cannot_infer_keypath_root_anykeypath_context,none,
+     "'AnyKeyPath' does not provide enough context for root type to be inferred; "
+     "consider explicitly specifying a root type", ())
 
 ERROR(could_not_find_type_member,none,
       "type %0 has no member %1", (Type, DeclNameRef))

lib/Sema/CSBindings.cpp

@@ -1105,6 +1105,8 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
       } else if (srcLocator->getAnchor() &&
                  isa<ObjectLiteralExpr>(srcLocator->getAnchor())) {
         fix = SpecifyObjectLiteralTypeImport::create(cs, dstLocator);
+      } else if (srcLocator->isKeyPathRoot()) {
+        fix = SpecifyKeyPathRootType::create(cs, dstLocator);
       }
 
       if (fix && cs.recordFix(fix))

lib/Sema/CSDiagnostics.cpp

@@ -6347,3 +6347,25 @@ bool MultiArgFuncKeyPathFailure::diagnoseAsError() {
                  resolveType(functionType));
   return true;
 }
+
+bool UnableToInferKeyPathRootFailure::diagnoseAsError() {
+  assert(isExpr<KeyPathExpr>(getAnchor()) && "Expected key path expression");
+  auto &ctx = getASTContext();
+  auto contextualType = getContextualType(getAnchor());
+  auto *keyPathExpr = castToExpr<KeyPathExpr>(getAnchor());
+
+  auto emitKeyPathDiagnostic = [&]() {
+    if (contextualType &&
+        contextualType->getAnyNominal() == ctx.getAnyKeyPathDecl()) {
+      return emitDiagnostic(
+          diag::cannot_infer_keypath_root_anykeypath_context);
+    }
+    return emitDiagnostic(
+        diag::cannot_infer_contextual_keypath_type_specify_root);
+  };
+
+  emitKeyPathDiagnostic()
+      .highlight(keyPathExpr->getLoc())
+      .fixItInsertAfter(keyPathExpr->getStartLoc(), "<#Root#>");
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -2069,6 +2069,21 @@ class MultiArgFuncKeyPathFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose a failure to infer a KeyPath type by context.
+///
+/// ```swift
+/// _ = \.x
+/// let _ : AnyKeyPath = \.x
+/// ```
+class UnableToInferKeyPathRootFailure final : public FailureDiagnostic {
+public:
+  UnableToInferKeyPathRootFailure(const Solution &solution,
+                                  ConstraintLocator *locator)
+  : FailureDiagnostic(solution, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -1353,3 +1353,17 @@ AllowKeyPathRootTypeMismatch::create(ConstraintSystem &cs, Type lhs, Type rhs,
   return new (cs.getAllocator())
       AllowKeyPathRootTypeMismatch(cs, lhs, rhs, locator);
 }
+
+SpecifyKeyPathRootType *
+SpecifyKeyPathRootType::create(ConstraintSystem &cs,
+                               ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      SpecifyKeyPathRootType(cs, locator);
+}
+
+bool SpecifyKeyPathRootType::diagnose(const Solution &solution,
+                                      bool asNote) const {
+  UnableToInferKeyPathRootFailure failure(solution, getLocator());
+  
+  return failure.diagnose(asNote);
+}

lib/Sema/CSFix.h

@@ -261,7 +261,11 @@ enum class FixKind : uint8_t {
   AllowKeyPathRootTypeMismatch,
 
   /// Allow key path to be bound to a function type with more than 1 argument
-  AllowMultiArgFuncKeyPathMismatch
+  AllowMultiArgFuncKeyPathMismatch,
+  
+  /// Specify key path root type when it cannot be infered from context.
+  SpecifyKeyPathRootType,
+
 };
 
 class ConstraintFix {
@@ -1820,7 +1824,7 @@ class AllowCoercionToForceCast final : public ContextualMismatch {
 ///   bar[keyPath: keyPath]
 /// }
 /// \endcode
-class AllowKeyPathRootTypeMismatch : public ContextualMismatch {
+class AllowKeyPathRootTypeMismatch final : public ContextualMismatch {
 protected:
   AllowKeyPathRootTypeMismatch(ConstraintSystem &cs, Type lhs, Type rhs,
                                ConstraintLocator *locator)
@@ -1838,6 +1842,21 @@ class AllowKeyPathRootTypeMismatch : public ContextualMismatch {
   create(ConstraintSystem &cs, Type lhs, Type rhs, ConstraintLocator *locator);
 };
 
+class SpecifyKeyPathRootType final : public ConstraintFix {
+    SpecifyKeyPathRootType(ConstraintSystem &cs, ConstraintLocator *locator)
+        : ConstraintFix(cs, FixKind::SpecifyKeyPathRootType, locator) {}
+
+  public:
+    std::string getName() const {
+      return "specify key path root type";
+    }
+
+    bool diagnose(const Solution &solution, bool asNote = false) const;
+
+    static SpecifyKeyPathRootType *create(ConstraintSystem &cs,
+                                          ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSGen.cpp

@@ -3427,7 +3427,8 @@ namespace {
       auto rootLocator =
           CS.getConstraintLocator(E, ConstraintLocator::KeyPathRoot);
       auto locator = CS.getConstraintLocator(E);
-      Type root = CS.createTypeVariable(rootLocator, TVO_CanBindToNoEscape);
+      Type root = CS.createTypeVariable(rootLocator, TVO_CanBindToNoEscape |
+                                        TVO_CanBindToHole);
 
       // If a root type was explicitly given, then resolve it now.
       if (auto rootRepr = E->getRootType()) {
@@ -3569,7 +3570,8 @@ namespace {
       // path components.
       auto typeLoc =
           CS.getConstraintLocator(locator, ConstraintLocator::KeyPathType);
-      Type kpTy = CS.createTypeVariable(typeLoc, TVO_CanBindToNoEscape);
+      Type kpTy = CS.createTypeVariable(typeLoc, TVO_CanBindToNoEscape |
+                                        TVO_CanBindToHole);
       CS.addKeyPathConstraint(kpTy, root, rvalueBase, componentTypeVars,
                               locator);
       return kpTy;

lib/Sema/CSSimplify.cpp

@@ -7696,6 +7696,32 @@ ConstraintSystem::simplifyKeyPathConstraint(
 
     return true;
   };
+  
+  // If we have a hole somewhere in the key path, the solver won't be able to
+  // infer the key path type. So let's just assume this is solved.
+  if (shouldAttemptFixes()) {
+    if (keyPathTy->isHole())
+      return SolutionKind::Solved;
+
+    // If the root type has been bound to a hole, we cannot infer it.
+    if (getFixedTypeRecursive(rootTy, /*wantRValue*/ true)->isHole())
+      return SolutionKind::Solved;
+
+    // If we have e.g a missing member somewhere, a component type variable
+    // will have been marked as a potential hole.
+    // FIXME: This relies on the fact that we only mark an overload type
+    // variable as a potential hole once we've added a corresponding fix. We
+    // can't use 'isHole' instead, as that doesn't handle cases where the
+    // overload type variable gets bound to another type from the context rather
+    // than a hole. We need to come up with a better way of handling the
+    // relationship between key paths and overloads.
+    if (llvm::any_of(componentTypeVars, [&](TypeVariableType *tv) {
+          return tv->getImpl().getLocator()->isForKeyPathComponent() &&
+                 tv->getImpl().canBindToHole();
+        })) {
+      return SolutionKind::Solved;
+    }
+  }
 
   // If we're fixed to a bound generic type, trying harvesting context from it.
   // However, we don't want a solution that fixes the expression type to
@@ -7745,34 +7771,11 @@ ConstraintSystem::simplifyKeyPathConstraint(
       // to determine whether the result will be a function type vs BGT KeyPath
       // type, so continue through components to create new constraint at the
       // end.
-      if (!overload || anyComponentsUnresolved) {
+      if (!overload) {
         if (flags.contains(TMF_GenerateConstraints)) {
           anyComponentsUnresolved = true;
           continue;
         }
-
-        if (shouldAttemptFixes()) {
-          auto typeVar =
-              llvm::find_if(componentTypeVars, [&](TypeVariableType *typeVar) {
-                auto *locator = typeVar->getImpl().getLocator();
-                auto elt = locator->findLast<LocatorPathElt::KeyPathComponent>();
-                return elt && elt->getIndex() == i;
-              });
-
-          // If one of the components haven't been resolved, let's check
-          // whether it has been determined to be a "hole" and if so,
-          // let's allow component validation to contiunue.
-          //
-          // This helps to, for example, diagnose problems with missing
-          // members used as part of a key path.
-          if (typeVar != componentTypeVars.end() &&
-              (*typeVar)->getImpl().canBindToHole()) {
-            anyComponentsUnresolved = true;
-            capability = ReadOnly;
-            continue;
-          }
-        }
-
         return SolutionKind::Unsolved;
       }
 
@@ -9502,7 +9505,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::CoerceToCheckedCast:
   case FixKind::SpecifyObjectLiteralTypeImport:
   case FixKind::AllowKeyPathRootTypeMismatch:
-  case FixKind::AllowCoercionToForceCast: {
+  case FixKind::AllowCoercionToForceCast:
+  case FixKind::SpecifyKeyPathRootType: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
 

test/Constraints/rdar62201037.swift

@@ -0,0 +1,31 @@
+// RUN: %target-swift-frontend %s -verify -emit-sil -o /dev/null
+
+struct R<T> {
+  var str: String?
+}
+
+func map<A, B>(e: (A) -> B) -> () -> R<B> {
+  fatalError()
+}
+func map<A, B>(_ : (A) -> B) -> (A?) -> B? {
+  fatalError()
+}
+
+infix operator |>
+func |> <A, B> (g: A, h: (A) -> B) -> B { h(g) }
+
+infix operator ^^^
+func ^^^ <A, B, C>(j: ((B) -> C) -> A, k: String) {}
+
+extension WritableKeyPath {
+  static func ^^^ (l: WritableKeyPath, m: Value) -> (Root) -> Root {
+    fatalError()
+  }
+}
+
+func foo<T>(_ s: String, _ rt: R<T>?) -> String? {
+  return rt.flatMap { _ in
+    rt |> map(\.str ^^^ s)
+  }
+  .flatMap(\.str)
+}

test/expr/unary/keypath/keypath.swift

@@ -195,9 +195,9 @@ func testKeyPath(sub: Sub, optSub: OptSub,
 
   let _: AnyKeyPath = \A.property
   let _: AnyKeyPath = \C<A>.value
-  let _: AnyKeyPath = \.property // expected-error{{ambiguous}}
+  let _: AnyKeyPath = \.property // expected-error {{'AnyKeyPath' does not provide enough context for root type to be inferred; consider explicitly specifying a root type}} {{24-24=<#Root#>}}
   let _: AnyKeyPath = \C.value // expected-error{{generic parameter 'T' could not be inferred}}
-  let _: AnyKeyPath = \.value // expected-error{{ambiguous}}
+  let _: AnyKeyPath = \.value // expected-error {{'AnyKeyPath' does not provide enough context for root type to be inferred; consider explicitly specifying a root type}} {{24-24=<#Root#>}}
 
   let _ = \Prop.[nonHashableSub] // expected-error{{subscript index of type 'NonHashableSub' in a key path must be Hashable}}
   let _ = \Prop.[sub, sub]
@@ -893,6 +893,44 @@ struct SR_12290 {
   }
 }
 
+func testKeyPathHole() {
+  _ = \.x // expected-error {{cannot infer key path type from context; consider explicitly specifying a root type}} {{8-8=<#Root#>}}
+  _ = \.x.y // expected-error {{cannot infer key path type from context; consider explicitly specifying a root type}} {{8-8=<#Root#>}}
+
+  let _ : AnyKeyPath = \.x 
+  // expected-error@-1 {{'AnyKeyPath' does not provide enough context for root type to be inferred; consider explicitly specifying a root type}} {{25-25=<#Root#>}}
+  let _ : AnyKeyPath = \.x.y
+  // expected-error@-1 {{'AnyKeyPath' does not provide enough context for root type to be inferred; consider explicitly specifying a root type}} {{25-25=<#Root#>}}
+
+  func f(_ i: Int) {}
+  f(\.x) // expected-error {{cannot infer key path type from context; consider explicitly specifying a root type}} {{6-6=<#Root#>}}
+  f(\.x.y) // expected-error {{cannot infer key path type from context; consider explicitly specifying a root type}} {{6-6=<#Root#>}}
+
+  // FIXME(SR-12827): Instead of "generic parameter 'T' could not be inferred",
+  // we should offer the same diagnostic as above.
+  func provideValueButNotRoot<T>(_ fn: (T) -> String) {} // expected-note 2{{in call to function 'provideValueButNotRoot'}}
+  provideValueButNotRoot(\.x) // expected-error {{generic parameter 'T' could not be inferred}}
+  provideValueButNotRoot(\.x.y) // expected-error {{generic parameter 'T' could not be inferred}}
+  provideValueButNotRoot(\String.foo) // expected-error {{value of type 'String' has no member 'foo'}}
+
+  func provideKPValueButNotRoot<T>(_ kp: KeyPath<T, String>) {} // expected-note 3{{in call to function 'provideKPValueButNotRoot'}}
+  provideKPValueButNotRoot(\.x) // expected-error {{generic parameter 'T' could not be inferred}}
+  provideKPValueButNotRoot(\.x.y) // expected-error {{generic parameter 'T' could not be inferred}}
+  provideKPValueButNotRoot(\String.foo)
+  // expected-error@-1 {{value of type 'String' has no member 'foo'}}
+  // expected-error@-2 {{generic parameter 'T' could not be inferred}}
+}
+
+func testMissingMember() {
+  let _: KeyPath<String, String> = \.foo // expected-error {{value of type 'String' has no member 'foo'}}
+  let _: KeyPath<String, String> = \.foo.bar // expected-error {{value of type 'String' has no member 'foo'}}
+
+  let _: PartialKeyPath<String> = \.foo // expected-error {{value of type 'String' has no member 'foo'}}
+  let _: PartialKeyPath<String> = \.foo.bar // expected-error {{value of type 'String' has no member 'foo'}}
+
+  _ = \String.x.y // expected-error {{value of type 'String' has no member 'x'}}
+}
+
 func testSyntaxErrors() { // expected-note{{}}
   _ = \.  ; // expected-error{{expected member name following '.'}}
   _ = \.a ;