[CS] Don't set parsed paths for dynamic member key paths

The logic here could form AST loops due to passing
in `anchor` for the key path's parsed path.

However setting a parsed path here seems to be a
holdover from the CSDiag days, so set the path to
`nullptr` and rip out and the rest of the synthesis
and SanitizeExpr logic for it.

rdar://85236369

Author: hamishknight

lib/Sema/CSApply.cpp

@@ -2218,23 +2218,31 @@ namespace {
     Expr *buildKeyPathDynamicMemberArgExpr(BoundGenericType *keyPathTy,
                                            SourceLoc dotLoc,
                                            ConstraintLocator *memberLoc) {
+      using Component = KeyPathExpr::Component;
       auto &ctx = cs.getASTContext();
       auto *anchor = getAsExpr(memberLoc->getAnchor());
 
-      SmallVector<KeyPathExpr::Component, 2> components;
-
-      // Let's create a KeyPath expression and fill in "parsed path"
-      // after component is built.
-      auto *keyPath = new (ctx) KeyPathExpr(/*backslashLoc=*/dotLoc,
-                                            /*parsedRoot=*/nullptr,
-                                            /*parsedPath=*/anchor,
-                                            /*hasLeadingDot=*/false,
-                                            /*isImplicit=*/true);
-      // Type of the keypath expression we are forming is known
-      // in advance, so let's set it right away.
-      keyPath->setType(keyPathTy);
-      cs.cacheType(keyPath);
+      auto makeKeyPath = [&](ArrayRef<Component> components) -> Expr * {
+        // Create a new implicit key path. We pass in anchor as the parsed path
+        // to set the end loc, but don't want to keep it as the parsed path,
+        // so clear it out after.
+        auto *kp = new (ctx) KeyPathExpr(/*backslashLoc*/ dotLoc,
+                                         /*parsedRoot*/ nullptr,
+                                         /*parsedPath*/ anchor,
+                                         /*hasLeadingDot*/ false,
+                                         /*isImplicit*/ true);
+        kp->setParsedPath(nullptr);
+        kp->setComponents(ctx, components);
+        kp->setType(keyPathTy);
+        cs.cacheExprTypes(kp);
+
+        // See whether there's an equivalent ObjC key path string we can produce
+        // for interop purposes.
+        checkAndSetObjCKeyPathString(kp);
+        return kp;
+      };
 
+      SmallVector<Component, 2> components;
       auto *componentLoc = cs.getConstraintLocator(
           memberLoc,
           LocatorPathElt::KeyPathDynamicMember(keyPathTy->getAnyNominal()));
@@ -2247,95 +2255,44 @@ namespace {
       case OverloadChoiceKind::KeyPathDynamicMemberLookup: {
         buildKeyPathSubscriptComponent(overload, dotLoc, /*args=*/nullptr,
                                        componentLoc, components);
-        keyPath->setComponents(ctx, components);
-        cs.cacheExprTypes(keyPath);
-        return keyPath;
+        return makeKeyPath(components);
       }
 
       default:
         break;
       }
 
-      // We can't reuse existing expression because type-check
-      // based diagnostics could hold the reference to original AST.
-      Expr *componentExpr = nullptr;
-      auto *dotExpr = new (ctx) KeyPathDotExpr(dotLoc);
-
-      // Determines whether this index is built to be used for
-      // one of the existing keypath components e.g. `\Lens<[Int]>.count`
-      // instead of a regular expression e.g. `lens[0]`.
-      bool forKeyPathComponent = false;
-      // Looks like keypath dynamic member lookup was used inside
-      // of a keypath expression e.g. `\Lens<[Int]>.count` where
-      // `count` is referenced using dynamic lookup.
       if (auto *KPE = dyn_cast<KeyPathExpr>(anchor)) {
+        // Looks like keypath dynamic member lookup was used inside
+        // of a keypath expression e.g. `\Lens<[Int]>.count` where
+        // `count` is referenced using dynamic lookup.
         auto kpElt = memberLoc->findFirst<LocatorPathElt::KeyPathComponent>();
         assert(kpElt && "no keypath component node");
-        auto &origComponent = KPE->getComponents()[kpElt->getIndex()];
-
-        using ComponentKind = KeyPathExpr::Component::Kind;
-        if (origComponent.getKind() == ComponentKind::UnresolvedProperty) {
-          anchor = new (ctx) UnresolvedDotExpr(
-              dotExpr, dotLoc, origComponent.getUnresolvedDeclName(),
-              DeclNameLoc(origComponent.getLoc()),
-              /*Implicit=*/true);
-        } else if (origComponent.getKind() ==
-                   ComponentKind::UnresolvedSubscript) {
-          anchor = SubscriptExpr::create(
-              ctx, dotExpr, origComponent.getSubscriptArgs(), ConcreteDeclRef(),
-              /*implicit=*/true, AccessSemantics::Ordinary);
+        auto &comp = KPE->getComponents()[kpElt->getIndex()];
+
+        if (comp.getKind() == Component::Kind::UnresolvedProperty) {
+          buildKeyPathPropertyComponent(overload, comp.getLoc(), componentLoc,
+                                        components);
+        } else if (comp.getKind() == Component::Kind::UnresolvedSubscript) {
+          buildKeyPathSubscriptComponent(overload, comp.getLoc(),
+                                         comp.getSubscriptArgs(), componentLoc,
+                                         components);
         } else {
           return nullptr;
         }
-
-        anchor->setType(simplifyType(overload.openedType));
-        cs.cacheType(anchor);
-        forKeyPathComponent = true;
+        return makeKeyPath(components);
       }
 
       if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor)) {
-        componentExpr =
-            forKeyPathComponent
-                ? UDE
-                : new (ctx) UnresolvedDotExpr(dotExpr, dotLoc, UDE->getName(),
-                                              UDE->getNameLoc(),
-                                              /*Implicit=*/true);
-
         buildKeyPathPropertyComponent(overload, UDE->getLoc(), componentLoc,
                                       components);
       } else if (auto *SE = dyn_cast<SubscriptExpr>(anchor)) {
-        componentExpr = SE;
-        // If this is not for a keypath component, we have to copy
-        // original subscript expression because expression based
-        // diagnostics might have a reference to it, so it couldn't
-        // be modified.
-        if (!forKeyPathComponent) {
-          componentExpr = SubscriptExpr::create(
-              ctx, dotExpr, SE->getArgs(),
-              SE->hasDecl() ? SE->getDecl() : ConcreteDeclRef(),
-              /*implicit=*/true, SE->getAccessSemantics());
-        }
-
         buildKeyPathSubscriptComponent(overload, SE->getLoc(), SE->getArgs(),
                                        componentLoc, components);
       } else {
         return nullptr;
       }
-
-      assert(componentExpr);
-      Type ty = simplifyType(cs.getType(anchor));
-      componentExpr->setType(ty);
-      cs.cacheType(componentExpr);
-
-      keyPath->setParsedPath(componentExpr);
-      keyPath->setComponents(ctx, components);
-      cs.cacheExprTypes(keyPath);
-
-      // See whether there's an equivalent ObjC key path string we can produce
-      // for interop purposes.
-      checkAndSetObjCKeyPathString(keyPath);
-
-      return keyPath;
+      return makeKeyPath(components);
     }
 
     /// Bridge the given value (which is an error type) to NSError.

lib/Sema/TypeCheckCodeCompletion.cpp

@@ -203,36 +203,6 @@ class SanitizeExpr : public ASTWalker {
         EPE->setSemanticExpr(nullptr);
       }
 
-      // If this expression represents keypath based dynamic member
-      // lookup, let's convert it back to the original form of
-      // member or subscript reference.
-      if (auto *SE = dyn_cast<SubscriptExpr>(expr)) {
-        auto *args = SE->getArgs();
-        auto isImplicitKeyPathExpr = [](Expr *argExpr) -> bool {
-          if (auto *KP = dyn_cast<KeyPathExpr>(argExpr))
-            return KP->isImplicit();
-          return false;
-        };
-
-        if (SE->isImplicit() && args->isUnary() &&
-            args->front().getLabel() == C.Id_dynamicMember &&
-            isImplicitKeyPathExpr(args->front().getExpr())) {
-          auto *keyPathExpr = cast<KeyPathExpr>(args->front().getExpr());
-          auto *componentExpr = keyPathExpr->getParsedPath();
-
-          if (auto *UDE = dyn_cast<UnresolvedDotExpr>(componentExpr)) {
-            UDE->setBase(SE->getBase());
-            return {true, UDE};
-          }
-
-          if (auto *subscript = dyn_cast<SubscriptExpr>(componentExpr)) {
-            subscript->setBase(SE->getBase());
-            return {true, subscript};
-          }
-          llvm_unreachable("unknown keypath component type");
-        }
-      }
-
       // If this is a closure, only walk into its children if they
       // are type-checked in the context of the enclosing expression.
       if (auto closure = dyn_cast<ClosureExpr>(expr)) {

test/expr/unary/keypath/rdar85236369.swift

@@ -0,0 +1,28 @@
+// RUN: %target-swift-frontend -dump-ast %s | %FileCheck %s
+
+struct Q {
+  var x: Int
+}
+
+@dynamicMemberLookup
+struct R {
+  subscript(dynamicMember dynamicMember: KeyPath<Q, Int>) -> Int {
+    fatalError()
+  }
+}
+
+@dynamicMemberLookup
+struct S {
+  subscript(dynamicMember dynamicMember: KeyPath<R, Int>) -> Int {
+    fatalError()
+  }
+}
+
+// rdar://85236369 - We shouldn't synthesize a parsed root or path, we should
+// just have the original parsed root.
+
+// CHECK-NOT: (parsed_root
+// CHECK-NOT: (parsed_path
+// CHECK: (parsed_root
+// CHECK-NOT: (parsed_path
+_ = \S.x