Merge pull request #36427 from xedin/rdar-74711236

[ConstraintSystem] Re-activate constraints if, due to incorrect reference, member type has been bound before base type

Author: xedin

include/swift/Sema/ConstraintLocator.h

@@ -200,9 +200,13 @@ class ConstraintLocator : public llvm::FoldingSetNode {
   /// via key path dynamic member lookup.
   bool isForKeyPathDynamicMemberLookup() const;
 
-  /// Determine whether this locator points to one of the key path
-  /// components.
-  bool isForKeyPathComponent() const;
+  /// Determine whether this locator points to element inside
+  /// of a key path component.
+  bool isInKeyPathComponent() const;
+
+  /// Determine whether this locator points to a result type of
+  /// a key path component.
+  bool isForKeyPathComponentResult() const;
 
   /// Determine whether this locator points to the generic parameter.
   bool isForGenericParameter() const;

lib/Sema/CSBindings.cpp

@@ -1612,6 +1612,13 @@ bool TypeVarBindingProducer::computeNext() {
       addNewBinding(binding.withType(LValueType::get(binding.BindingType)));
     }
 
+    // There is a tailored fix for optional key path root references,
+    // let's not create ambiguity by attempting unwrap when it's
+    // not allowed.
+    if (binding.Kind != BindingKind::Subtypes &&
+        getLocator()->isKeyPathRoot() && type->getOptionalObjectType())
+      continue;
+
     // Allow solving for T even for a binding kind where that's invalid
     // if fixes are allowed, because that gives us the opportunity to
     // match T? values to the T binding by adding an unwrap fix.

lib/Sema/CSDiagnostics.cpp

@@ -3975,7 +3975,7 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
     // If this is a reference to a static member by one of the key path
     // components, let's provide a tailored diagnostic and return because
     // that is unsupported so there is no fix-it.
-    if (locator->isForKeyPathComponent()) {
+    if (locator->isInKeyPathComponent()) {
       InvalidStaticMemberRefInKeyPath failure(getSolution(), Member, locator);
       return failure.diagnoseAsError();
     }

lib/Sema/CSDiagnostics.h

@@ -1576,7 +1576,7 @@ class InvalidMemberRefInKeyPath : public FailureDiagnostic {
                             ConstraintLocator *locator)
       : FailureDiagnostic(solution, locator), Member(member) {
     assert(member->hasName());
-    assert(locator->isForKeyPathComponent() ||
+    assert(locator->isInKeyPathComponent() ||
            locator->isForKeyPathDynamicMemberLookup());
   }
 

lib/Sema/CSSimplify.cpp

@@ -7751,8 +7751,21 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
 
       // Record a hole for memberTy to make it possible to form solutions
       // when contextual result type cannot be deduced e.g. `let _ = x.foo`.
-      if (auto *memberTypeVar = memberTy->getAs<TypeVariableType>())
-        recordPotentialHole(memberTypeVar);
+      if (auto *memberTypeVar = memberTy->getAs<TypeVariableType>()) {
+        if (getFixedType(memberTypeVar)) {
+          // If member has been bound before the base and the base was
+          // incorrect at that (e.g. fallback to default `Any` type),
+          // then we need to re-activate all of the constraints
+          // associated with this member reference otherwise some of
+          // the constraints could be left unchecked in inactive state.
+          // This is especially important for key path expressions because
+          // `key path` constraint can't be retired until all components
+          // are simplified.
+          addTypeVariableConstraintsToWorkList(memberTypeVar);
+        } else {
+          recordPotentialHole(memberTypeVar);
+        }
+      }
 
       return SolutionKind::Solved;
     };
@@ -8871,15 +8884,28 @@ ConstraintSystem::simplifyKeyPathConstraint(
       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 'isPlaceholder' 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.
+    // would either be marked as a potential hole or would have a fix.
     if (llvm::any_of(componentTypeVars, [&](TypeVariableType *tv) {
-          return tv->getImpl().getLocator()->isForKeyPathComponent() &&
+          auto *locator = tv->getImpl().getLocator();
+
+          // Result type of a component could be bound to a contextual
+          // (concrete) type if it's the last component in the chain,
+          // so the only way to detect errors is to check for fixes.
+          if (locator->isForKeyPathComponentResult()) {
+            auto path = locator->getPath();
+            auto *componentLoc =
+                getConstraintLocator(locator->getAnchor(), path.drop_back());
+
+            if (hasFixFor(componentLoc, FixKind::DefineMemberBasedOnUse) ||
+                hasFixFor(componentLoc, FixKind::UnwrapOptionalBase) ||
+                hasFixFor(componentLoc,
+                          FixKind::UnwrapOptionalBaseWithOptionalResult))
+              return true;
+          }
+
+          // If something inside of a component is marked as a hole,
+          // let's consider while component to be invalid.
+          return locator->isInKeyPathComponent() &&
                  tv->getImpl().canBindToHole();
         })) {
       return SolutionKind::Solved;

lib/Sema/ConstraintLocator.cpp

@@ -171,12 +171,16 @@ bool ConstraintLocator::isForKeyPathDynamicMemberLookup() const {
   return !path.empty() && path.back().isKeyPathDynamicMember();
 }
 
-bool ConstraintLocator::isForKeyPathComponent() const {
+bool ConstraintLocator::isInKeyPathComponent() const {
   return llvm::any_of(getPath(), [&](const LocatorPathElt &elt) {
     return elt.isKeyPathComponent();
   });
 }
 
+bool ConstraintLocator::isForKeyPathComponentResult() const {
+  return isLastElement<LocatorPathElt::KeyPathComponentResult>();
+}
+
 bool ConstraintLocator::isForGenericParameter() const {
   return isLastElement<LocatorPathElt::GenericParameter>();
 }

test/expr/unary/keypath/keypath.swift

@@ -1052,3 +1052,45 @@ func testSyntaxErrors() {
   _ = \A.a?;
   _ = \A.a!;
 }
+
+// SR-13364 - keypath missing optional crashes compiler: "Inactive constraints left over?"
+func sr13364() {
+  let _: KeyPath<String?, Int?> = \.utf8.count // expected-error {{no exact matches in reference to property 'count'}}
+  // expected-note@-1 {{found candidate with type 'Int'}}
+}
+
+// rdar://74711236 - crash due to incorrect member access in key path
+func rdar74711236() {
+  struct S {
+    var arr: [V] = []
+  }
+
+  struct V : Equatable {
+  }
+
+  enum Type {
+  case store
+  }
+
+  struct Context {
+    func supported() -> [Type] {
+      return []
+    }
+  }
+
+  func test(context: Context?) {
+    var s = S()
+
+    s.arr = {
+      // FIXME: Missing member reference is pattern needs a better diagnostic
+      if let type = context?.store { // expected-error {{type of expression is ambiguous without more context}}
+        // `isSupported` should be an invalid declaration to trigger a crash in `map(\.option)`
+        let isSupported = context!.supported().contains(type) // expected-error {{missing argument label 'where:' in call}} expected-error {{converting non-escaping value to '(Type) throws -> Bool' may allow it to escape}}
+        return (isSupported ? [type] : []).map(\.option)
+        // expected-error@-1 {{value of type 'Any' has no member 'option'}}
+        // expected-note@-2 {{cast 'Any' to 'AnyObject' or use 'as!' to force downcast to a more specific type to access members}}
+      }
+      return []
+    }()
+  }
+}