[CSSolver] Don't generate implicit argument for keypath dynamic lookup

xedin · xedin · commit 9eed0135fa0c · 2019-04-01T12:40:39.000-07:00
Implicit argument expression was necessary to generate keypath
constraint which is used to validate a choice picked for the member.
But since read-only check has been factored out it's now possible
to validate choice directly in combination with new 'keypath dynamic lookup'
locator associated with member type variable which represents result
of the dynamic lookup.
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -1515,18 +1515,44 @@ namespace {
       return ctorRef;
     }
 
-    Expr *buildKeyPathDynamicMemberIndexExpr(ConstraintLocator *memberLoc) {
-      auto arg = solution.DynamicMemberArguments.find(memberLoc);
-      assert(arg != solution.DynamicMemberArguments.end() &&
-             "cannot find argument for keypath dynamic member lookup");
-
-      auto *keyPath = arg->getSecond();
-      keyPath->forEachChildExpr([&](Expr *childExpr) -> Expr * {
-        simplifyExprType(childExpr);
-        return childExpr;
-      });
+    /// Build an implicit argument for keypath based dynamic lookup,
+    /// which consists of KeyPath expression and a single component.
+    ///
+    /// \param keyPathTy The type of the keypath argument.
+    /// \param dotLoc The location of the '.' preceding member name.
+    /// \param memberLoc The locator to be associated with new argument.
+    Expr *buildKeyPathDynamicMemberIndexExpr(BoundGenericType *keyPathTy,
+                                             SourceLoc dotLoc,
+                                             ConstraintLocator *memberLoc) {
+      auto &ctx = cs.getASTContext();
+      // Only properties are supported at the moment.
+      auto *UDE = dyn_cast<UnresolvedDotExpr>(memberLoc->getAnchor());
+      if (!UDE)
+        return nullptr;
+
+      simplifyExprType(UDE);
+      UDE->setType(cs.getType(UDE));
 
-      return visitKeyPathExpr(keyPath);
+      // Let's re-use existinng expression but switch its base
+      // to keypath special dot expression.
+      UDE->setBase(new (ctx) KeyPathDotExpr(dotLoc));
+
+      // Now, let's create a KeyPath expression itself.
+      auto *keyPath = new (ctx) KeyPathExpr(/*backslashLoc=*/dotLoc,
+                                            /*parsedRoot=*/nullptr,
+                                            /*parsedPath=*/UDE,
+                                            /*isImplicit=*/true);
+
+      auto *propertyLoc = cs.getConstraintLocator(
+          memberLoc,
+          LocatorPathElt::getKeyPathDynamicMember(keyPathTy->getAnyNominal()));
+      auto overload = solution.getOverloadChoice(propertyLoc);
+      keyPath->resolveComponents(
+          ctx, {buildKeyPathPropertyComponent(overload, UDE->getLoc(),
+                                              propertyLoc)});
+      keyPath->setType(keyPathTy);
+      cs.cacheType(keyPath);
+      return keyPath;
     }
 
     /// Bridge the given value (which is an error type) to NSError.
@@ -2697,7 +2723,8 @@ namespace {
           auto fieldName = selected.choice.getName().getBaseIdentifier().str();
           argExpr = buildDynamicMemberLookupIndexExpr(fieldName, loc, dc, cs);
         } else {
-          argExpr = buildKeyPathDynamicMemberIndexExpr(memberLocator);
+          argExpr = buildKeyPathDynamicMemberIndexExpr(
+              paramTy->castTo<BoundGenericType>(), dotLoc, memberLocator);
         }
 
         assert(argExpr);
@@ -4128,10 +4155,6 @@ namespace {
       KeyPathSubscriptComponents;
   public:
     Expr *visitKeyPathExpr(KeyPathExpr *E) {
-      return visitKeyPathExpr(E, cs.getConstraintLocator(E));
-    }
-
-    Expr *visitKeyPathExpr(KeyPathExpr *E, ConstraintLocator *baseLocator) {
       if (E->isObjC()) {
         cs.setType(E, cs.getType(E->getObjCStringLiteralExpr()));
         return E;
@@ -4205,8 +4228,7 @@ namespace {
         Optional<SelectedOverload> foundDecl;
 
         auto locator = cs.getConstraintLocator(
-            baseLocator,
-            ConstraintLocator::PathElement::getKeyPathComponent(i));
+            E, ConstraintLocator::PathElement::getKeyPathComponent(i));
         if (kind == KeyPathExpr::Component::Kind::UnresolvedSubscript) {
           locator =
             cs.getConstraintLocator(locator,
diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp
@@ -172,9 +172,6 @@ Solution ConstraintSystem::finalize() {
   for (auto &e : CheckedConformances)
     solution.Conformances.push_back({e.first, e.second});
 
-  for (auto &arg : DynamicMemberArguments)
-    solution.DynamicMemberArguments.insert(arg);
-
   return solution;
 }
 
@@ -245,11 +242,6 @@ void ConstraintSystem::applySolution(const Solution &solution) {
   // Register any missing members encountered along this path.
   MissingMembers.insert(solution.MissingMembers.begin(),
                         solution.MissingMembers.end());
-
-  // Register any implicitly generated argument used by keypath
-  // based dynamic member lookup.
-  DynamicMemberArguments.append(solution.DynamicMemberArguments.begin(),
-                                solution.DynamicMemberArguments.end());
 }
 
 /// Restore the type variable bindings to what they were before
@@ -433,7 +425,6 @@ ConstraintSystem::SolverScope::SolverScope(ConstraintSystem &cs)
   numMissingMembers = cs.MissingMembers.size();
   numDisabledConstraints = cs.solverState->getNumDisabledConstraints();
   numFavoredConstraints = cs.solverState->getNumFavoredConstraints();
-  numDynamicMemberArguments = cs.DynamicMemberArguments.size();
 
   PreviousScore = cs.CurrentScore;
 
@@ -488,10 +479,6 @@ ConstraintSystem::SolverScope::~SolverScope() {
   // Remove any missing members found along the current path.
   truncate(cs.MissingMembers, numMissingMembers);
 
-  // Remove any implicitly generated arguments used by keypath
-  // based dynamic lookup generated along the current path.
-  truncate(cs.DynamicMemberArguments, numDynamicMemberArguments);
-
   // Reset the previous score.
   cs.CurrentScore = PreviousScore;
 
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -1893,42 +1893,32 @@ void ConstraintSystem::resolveOverload(ConstraintLocator *locator,
 
       refType = fnType->getResult();
 
-      auto *keyPathExpr = buildImplicitDynamicKeyPathArgument(
-          locator->getAnchor(), keyPathTy, locator);
-
-      // If argument couldn't be built, let's fail this choice.
-      if (!keyPathExpr) {
-        failedConstraint = Constraint::create(*this, ConstraintKind::Bind,
-                                              boundType, refType, locator);
-        return;
-      }
+      auto *keyPathDecl = keyPathTy->getAnyNominal();
+      assert(keyPathDecl &&
+             (keyPathDecl == getASTContext().getKeyPathDecl() ||
+              keyPathDecl == getASTContext().getWritableKeyPathDecl()) &&
+             "parameter is supposed to be a keypath");
 
-      auto *keyPathLocator = getConstraintLocator(keyPathExpr);
-      auto *componentLoc = getConstraintLocator(
-          keyPathExpr, LocatorPathElt::getKeyPathComponent(0));
+      auto *keyPathLoc = getConstraintLocator(
+          locator, LocatorPathElt::getKeyPathDynamicMember(keyPathDecl));
 
       auto rootTy = keyPathTy->getGenericArgs()[0];
+      auto leafTy = keyPathTy->getGenericArgs()[1];
+
       // Member would either point to mutable or immutable property, we
       // don't which at the moment, so let's allow its type to be l-value.
-      auto memberTy = createTypeVariable(componentLoc, TVO_CanBindToLValue);
+      auto memberTy = createTypeVariable(keyPathLoc, TVO_CanBindToLValue);
       // Attempt to lookup a member with a give name in the root type and
       // assign result to the leaf type of the keypath.
       addValueMemberConstraint(LValueType::get(rootTy), choice.getName(),
                                memberTy, useDC, FunctionRefKind::Unapplied,
-                               /*outerAlternatives=*/{}, componentLoc);
+                               /*outerAlternatives=*/{}, keyPathLoc);
 
-      auto rvalueMemberTy = createTypeVariable(keyPathLocator);
       // Since member type is going to be bound to "leaf" generic parameter
       // of the keypath, it has to be an r-value always, so let's add a new
-      // constraint to preserve that.
-      addConstraint(ConstraintKind::Equal, memberTy, rvalueMemberTy,
-                    keyPathLocator);
-
-      // For a new keypath constraint which is going to check whether given
-      // overload is correct.
-      addUnsolvedConstraint(Constraint::create(*this, ConstraintKind::KeyPath,
-                                               keyPathTy, rootTy,
-                                               rvalueMemberTy, keyPathLocator));
+      // constraint to represent that conversion instead of loading member
+      // type into "leaf" directly.
+      addConstraint(ConstraintKind::Equal, memberTy, leafTy, keyPathLoc);
     }
     break;
   }
@@ -1998,6 +1988,29 @@ void ConstraintSystem::resolveOverload(ConstraintLocator *locator,
       }
     }
 
+    // Check whether this is overload choice found via keypath
+    // based dynamic member lookup. Since it's unknown upfront
+    // what kind of declaration lookup is going to find, let's
+    // double check here that given keypath is appropriate for it.
+    auto path = locator->getPath();
+    if (!path.empty() && path.back().isKeyPathDynamicMember()) {
+      auto *keyPath = path.back().getKeyPath();
+      if (auto *storage = dyn_cast<AbstractStorageDecl>(decl)) {
+        // If this is an attempt to access read-only member via
+        // writable key path, let's fail this choice early.
+        //
+        // TODO(diagnostics): Add a new fix that is suggests to
+        // add `subscript(dynamicMember: WritableKeyPath<T, U>)`
+        // overload here, instead of failing.
+        if (isReadOnlyKeyPathComponent(storage) &&
+            keyPath == getASTContext().getWritableKeyPathDecl()) {
+          failedConstraint = Constraint::create(*this, ConstraintKind::Bind,
+                                                boundType, refType, locator);
+          return;
+        }
+      }
+    }
+
     // Check whether applying this overload would result in invalid
     // partial function application e.g. partial application of
     // mutating method or initializer.
@@ -2601,35 +2614,3 @@ void ConstraintSystem::generateConstraints(
     recordChoice(constraints, index, choices[index]);
   }
 }
-
-KeyPathExpr *ConstraintSystem::buildImplicitDynamicKeyPathArgument(
-    Expr *member, BoundGenericType *keyPathTy, ConstraintLocator *locator) {
-  // Only properties are support at the moment.
-  auto *UDE = dyn_cast<UnresolvedDotExpr>(member);
-  if (!UDE)
-    return nullptr;
-
-  auto &ctx = getASTContext();
-  auto dotLoc = UDE->getDotLoc();
-
-  auto *property = new (ctx)
-      UnresolvedDotExpr(new (ctx) KeyPathDotExpr(dotLoc), dotLoc,
-                        UDE->getName(), UDE->getNameLoc(), /*isImplicit=*/true);
-
-  // Now, let's create a KeyPath expression itself.
-  auto *keyPath = new (ctx) KeyPathExpr(/*backslashLoc=*/dotLoc,
-                                        /*parsedRoot=*/nullptr,
-                                        /*parsedPath=*/property,
-                                        /*isImplicit=*/true);
-
-  // Let's form a single unrsolved property component this keypath is
-  // going to refer to.
-  keyPath->resolveComponents(
-      ctx, {KeyPathExpr::Component::forUnresolvedProperty(
-               property->getName(), property->getStartLoc())});
-  setType(keyPath, keyPathTy);
-
-  // Register newly generated argument in the constraint system.
-  DynamicMemberArguments.push_back({locator, keyPath});
-  return keyPath;
-}
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
@@ -613,11 +613,6 @@ class Solution {
   llvm::SmallVector<std::pair<ConstraintLocator *, ProtocolConformanceRef>, 8>
       Conformances;
 
-  /// The set of implicitly generated arguments used by keypath based
-  /// dynamic lookup.
-  llvm::SmallDenseMap<ConstraintLocator *, KeyPathExpr *>
-      DynamicMemberArguments;
-
   /// Simplify the given type by substituting all occurrences of
   /// type variables for their fixed types.
   Type simplifyType(Type type) const;
@@ -1081,11 +1076,6 @@ class ConstraintSystem {
   SmallVector<std::pair<ConstraintLocator *, ProtocolConformanceRef>, 8>
       CheckedConformances;
 
-  /// A cache that stores implicitly generated arguments for keypath based
-  /// dynamic lookup.
-  SmallVector<std::pair<ConstraintLocator *, KeyPathExpr *>, 4>
-      DynamicMemberArguments;
-
 public:
   /// The locators of \c Defaultable constraints whose defaults were used.
   SmallVector<ConstraintLocator *, 8> DefaultedConstraints;
@@ -1553,8 +1543,6 @@ class ConstraintSystem {
 
     unsigned numFavoredConstraints;
 
-    unsigned numDynamicMemberArguments;
-
     /// The previous score.
     Score PreviousScore;
 
@@ -2761,16 +2749,6 @@ class ConstraintSystem {
                                          bool includeInaccessibleMembers);
 
 private:
-  /// Build an implicit argument for keypath based dynamic lookup,
-  /// which consists of KeyPath expression and a single component.
-  ///
-  /// \param member The member being dynamically lookuped up.
-  /// \param keyPathTy The type of the keypath argument.
-  /// \param locator The locator to be associated with new argument.
-  KeyPathExpr *buildImplicitDynamicKeyPathArgument(Expr *member,
-                                                   BoundGenericType *keyPathTy,
-                                                   ConstraintLocator *locator);
-
   /// Attempt to simplify the given construction constraint.
   ///
   /// \param valueType The type being constructed.
