🍒 [5.7] CSApply: Handle unbound references to methods found via dynamic lookup

lib/Sema/CSApply.cpp

@@ -903,11 +903,6 @@ namespace {
                                bool baseIsInstance) {
       ValueDecl *member = choice.getDecl();
 
-      // FIXME: We should finish plumbing this through for dynamic
-      // lookup as well.
-      if (choice.getKind() == OverloadChoiceKind::DeclViaDynamic)
-        return false;
-
       // If we're inside a selector expression, don't build the thunk.
       // Were not actually going to emit the member reference, just
       // look at the AST.
@@ -921,9 +916,11 @@ namespace {
       if (!baseIsInstance && member->isInstanceMember())
         return true;
 
-      // Bound optional method references are represented via
-      // DynamicMemberRefExpr instead of a curry thunk.
-      if (member->getAttrs().hasAttribute<OptionalAttr>())
+      // Bound member references that are '@objc optional' or found via dynamic
+      // lookup are always represented via DynamicMemberRefExpr instead of a
+      // curry thunk.
+      if (member->getAttrs().hasAttribute<OptionalAttr>() ||
+          choice.getKind() == OverloadChoiceKind::DeclViaDynamic)
         return false;
 
       // Figure out how many argument lists we need.
@@ -1002,7 +999,10 @@ namespace {
       auto *calleeFnTy = fnTy;
 
       if (baseExpr) {
+        // Coerce the base expression to the container type.
         const auto calleeSelfParam = calleeFnTy->getParams().front();
+        baseExpr =
+            coerceToType(baseExpr, calleeSelfParam.getOldType(), locator);
 
         // If the 'self' parameter has non-trivial ownership, adjust the
         // argument type accordingly.
@@ -1159,7 +1159,33 @@ namespace {
       Expr *thunkBody = buildSingleCurryThunkBodyCall(
           baseExpr, fnExpr, declOrClosure, thunkParamList, locator);
 
-      // Coerce to the result type of the thunk.
+      // If we called a function with a dynamic 'Self' result, we may need some
+      // special handling.
+      if (baseExpr) {
+        if (auto *fnDecl = dyn_cast<AbstractFunctionDecl>(declOrClosure)) {
+          if (fnDecl->hasDynamicSelfResult()) {
+            Type convTy;
+
+            if (cs.getType(baseExpr)->hasOpenedExistential()) {
+              // FIXME: Sometimes we need to convert to an opened existential
+              // first, because CovariantReturnConversionExpr does not support
+              // direct conversions from a class C to an existential C & P.
+              convTy = cs.getType(baseExpr)->getMetatypeInstanceType();
+              convTy =
+                  thunkTy->getResult()->replaceCovariantResultType(convTy, 0);
+            } else {
+              convTy = thunkTy->getResult();
+            }
+
+            if (!thunkBody->getType()->isEqual(convTy)) {
+              thunkBody = cs.cacheType(
+                  new (ctx) CovariantReturnConversionExpr(thunkBody, convTy));
+            }
+          }
+        }
+      }
+
+      // Now, coerce to the result type of the thunk.
       thunkBody = coerceToType(thunkBody, thunkTy->getResult(), locator);
 
       if (thunkTy->getExtInfo().isThrowing()) {
@@ -1261,20 +1287,27 @@ namespace {
         cs.cacheType(selfParamRef);
       }
 
-      auto *const selfCalleeTy = cs.getType(memberRef)->castTo<FunctionType>();
-      const auto selfCalleeParam = selfCalleeTy->getParams().front();
-      const auto selfCalleeParamTy = selfCalleeParam.getPlainType();
-
-      // Open the 'self' parameter reference if warranted.
+      bool hasOpenedExistential = false;
       Expr *selfOpenedRef = selfParamRef;
-      if (selfCalleeParamTy->hasOpenedExistential()) {
+
+      // If the 'self' parameter type is existential, it must be opened.
+      if (selfThunkParamTy->isAnyExistentialType()) {
+        Type openedTy = solution.OpenedExistentialTypes.lookup(
+            cs.getConstraintLocator(memberLocator));
+        assert(openedTy);
+
+        hasOpenedExistential = true;
+
         // If we're opening an existential:
         // - The type of 'memberRef' inside the thunk is written in terms of the
-        //   open existental archetype.
+        //   opened existental archetype.
         // - The type of the thunk is written in terms of the
         //   erased existential bounds.
-        auto opaqueValueTy = selfCalleeParamTy;
-        if (selfCalleeParam.isInOut())
+        Type opaqueValueTy = openedTy;
+        if (selfThunkParamTy->is<ExistentialMetatypeType>())
+          opaqueValueTy = MetatypeType::get(opaqueValueTy);
+
+        if (selfThunkParam.isInOut())
           opaqueValueTy = LValueType::get(opaqueValueTy);
 
         selfOpenedRef = new (ctx) OpaqueValueExpr(SourceLoc(), opaqueValueTy);
@@ -1292,6 +1325,9 @@ namespace {
       // build a dynamic member reference. Otherwise, build a nested
       // "{ args... in self.member(args...) }" thunk that calls the member.
       if (isDynamicLookup || member->getAttrs().hasAttribute<OptionalAttr>()) {
+        auto *const selfCalleeTy =
+            cs.getType(memberRef)->castTo<FunctionType>();
+
         outerThunkBody = new (ctx) DynamicMemberRefExpr(
             selfOpenedRef, SourceLoc(),
             resolveConcreteDeclRef(member, memberLocator), memberLoc);
@@ -1303,10 +1339,11 @@ namespace {
                                       memberLocator);
 
         // Close the existential if warranted.
-        if (selfCalleeParamTy->hasOpenedExistential()) {
+        if (hasOpenedExistential) {
           // If the callee's 'self' parameter has non-trivial ownership, adjust
           // the argument type accordingly.
-          adjustExprOwnershipForParam(selfOpenedRef, selfCalleeParam);
+          adjustExprOwnershipForParam(selfOpenedRef,
+                                      selfCalleeTy->getParams().front());
 
           outerThunkBody = new (ctx) OpenExistentialExpr(
               selfParamRef, cast<OpaqueValueExpr>(selfOpenedRef),
@@ -1319,7 +1356,7 @@ namespace {
             outerThunkTy->getResult()->castTo<FunctionType>(), memberLocator);
 
         // Rewrite the body to close the existential if warranted.
-        if (selfCalleeParamTy->hasOpenedExistential()) {
+        if (hasOpenedExistential) {
           auto *body = innerThunk->getSingleExpressionBody();
           body = new (ctx) OpenExistentialExpr(
               selfParamRef, cast<OpaqueValueExpr>(selfOpenedRef), body,
@@ -1438,7 +1475,7 @@ namespace {
 
       const bool isUnboundInstanceMember =
           (!baseIsInstance && member->isInstanceMember());
-      const bool isPartialApplication =
+      const bool needsCurryThunk =
           shouldBuildCurryThunk(choice, baseIsInstance);
 
       // The formal type of the 'self' value for the member's declaration.
@@ -1452,24 +1489,28 @@ namespace {
       // the base accordingly.
       bool openedExistential = false;
 
-      // For a partial application, we have to open the existential inside
-      // the thunk itself.
       auto knownOpened = solution.OpenedExistentialTypes.find(
                            getConstraintSystem().getConstraintLocator(
                              memberLocator));
       if (knownOpened != solution.OpenedExistentialTypes.end()) {
         // Determine if we're going to have an OpenExistentialExpr around
         // this member reference.
         //
-        // If we have a partial application of a protocol method, we open
-        // the existential in the curry thunk, instead of opening it here,
-        // because we won't have a 'self' value until the curry thunk is
-        // applied.
+        // For an unbound reference to a method, always open the existential
+        // inside the curry thunk, because we won't have a 'self' value until
+        // the curry thunk is applied.
+        //
+        // For a partial application of a protocol method, open the existential
+        // inside the curry thunk as well. This reduces abstraction and
+        // post-factum function type conversions, and results in better SILGen.
         //
-        // However, a partial application of a class method on a subclass
-        // existential does need to open the existential, so that it can be
-        // upcast to the appropriate class reference type.
-        if (!isPartialApplication || !containerTy->hasOpenedExistential()) {
+        // For a partial application of a class method, however, we always want
+        // the thunk to accept a class to avoid potential abstraction, so the
+        // existential base must be opened eagerly in order to be upcast to the
+        // appropriate class reference type before it is passed to the thunk.
+        if (!needsCurryThunk ||
+            (!member->getDeclContext()->getSelfProtocolDecl() &&
+             !isUnboundInstanceMember)) {
           // Open the existential before performing the member reference.
           base = openExistentialReference(base, knownOpened->second, member);
           baseTy = knownOpened->second;
@@ -1508,13 +1549,15 @@ namespace {
                  base, selfParamTy, member,
                  locator.withPathElement(ConstraintLocator::MemberRefBase));
       } else {
-        if (!isExistentialMetatype || openedExistential) {
-          // Convert the base to an rvalue of the appropriate metatype.
-          base = coerceToType(base,
-                              MetatypeType::get(
-                                isDynamic ? selfTy : containerTy),
-                              locator.withPathElement(
-                                ConstraintLocator::MemberRefBase));
+        // The base of an unbound reference is unused, and thus a conversion
+        // is not necessary.
+        if (!isUnboundInstanceMember) {
+          if (!isExistentialMetatype || openedExistential) {
+            // Convert the base to an rvalue of the appropriate metatype.
+            base = coerceToType(
+                base, MetatypeType::get(isDynamic ? selfTy : containerTy),
+                locator.withPathElement(ConstraintLocator::MemberRefBase));
+          }
         }
 
         if (!base)
@@ -1544,8 +1587,8 @@ namespace {
       }
 
       // Handle dynamic references.
-      if (isDynamic || (!isPartialApplication &&
-                        member->getAttrs().hasAttribute<OptionalAttr>())) {
+      if (!needsCurryThunk &&
+          (isDynamic || member->getAttrs().hasAttribute<OptionalAttr>())) {
         base = cs.coerceToRValue(base);
         Expr *ref = new (context) DynamicMemberRefExpr(base, dotLoc, memberRef,
                                                        memberLoc);
@@ -1635,7 +1678,7 @@ namespace {
       //
       // { self in { args... in self.method(args...) } }(foo)
       //
-      // This is done instead of just hoising the expression 'foo' up
+      // This is done instead of just hoisting the expression 'foo' up
       // into the closure, which would change evaluation order.
       //
       // However, for a super method reference, eg, 'let fn = super.foo',
@@ -1644,12 +1687,12 @@ namespace {
       // very specific shape, we only emit a single closure here and
       // capture the original SuperRefExpr, since its evaluation does not
       // have side effects, instead of abstracting out a 'self' parameter.
-      const auto isSuperPartialApplication = isPartialApplication && isSuper;
+      const auto isSuperPartialApplication = needsCurryThunk && isSuper;
       if (isSuperPartialApplication) {
         ref = buildSingleCurryThunk(base, declRefExpr,
                                     cast<AbstractFunctionDecl>(member),
                                     memberLocator);
-      } else if (isPartialApplication) {
+      } else if (needsCurryThunk) {
         // Another case where we want to build a single closure is when
         // we have a partial application of a constructor on a statically-
         // derived metatype value. Again, there are no order of evaluation
@@ -1672,17 +1715,28 @@ namespace {
           return closure;
         }
 
-        auto *curryThunkTy = refTy->castTo<FunctionType>();
-
-        // Check if we need to open an existential stored inside 'self'.
-        auto knownOpened = solution.OpenedExistentialTypes.find(
-                             getConstraintSystem().getConstraintLocator(
-                               memberLocator));
-        if (knownOpened != solution.OpenedExistentialTypes.end()) {
-          curryThunkTy = curryThunkTy
-                             ->typeEraseOpenedArchetypesWithRoot(
-                                 knownOpened->second, dc)
-                             ->castTo<FunctionType>();
+        FunctionType *curryThunkTy = nullptr;
+        if (isUnboundInstanceMember) {
+          // For an unbound reference to a method, all conversions, including
+          // dynamic 'Self' handling, are done within the thunk to support
+          // the edge case of an unbound reference to a 'Self'-returning class
+          // method on a protocol metatype. The result of calling the method
+          // must be downcast to the opened archetype before being erased to the
+          // subclass existential to cope with the expectations placed
+          // on 'CovariantReturnConversionExpr'.
+          curryThunkTy = simplifyType(openedType)->castTo<FunctionType>();
+        } else {
+          curryThunkTy = refTy->castTo<FunctionType>();
+
+          // Check if we need to open an existential stored inside 'self'.
+          auto knownOpened = solution.OpenedExistentialTypes.find(
+              getConstraintSystem().getConstraintLocator(memberLocator));
+          if (knownOpened != solution.OpenedExistentialTypes.end()) {
+            curryThunkTy =
+                curryThunkTy
+                    ->typeEraseOpenedArchetypesWithRoot(knownOpened->second, dc)
+                    ->castTo<FunctionType>();
+          }
         }
 
         // Replace the DeclRefExpr with a closure expression which SILGen
@@ -1695,7 +1749,10 @@ namespace {
       // implicit function type conversion around the resulting expression,
       // with the destination type having 'Self' swapped for the appropriate
       // replacement type -- usually the base object type.
-      if (!member->getDeclContext()->getSelfProtocolDecl()) {
+      //
+      // Note: For unbound references this is handled inside the thunk.
+      if (!isUnboundInstanceMember &&
+          !member->getDeclContext()->getSelfProtocolDecl()) {
         if (auto func = dyn_cast<AbstractFunctionDecl>(member)) {
           if (func->hasDynamicSelfResult() &&
               !baseTy->getOptionalObjectType()) {

test/Interpreter/dynamic_lookup.swift

@@ -39,6 +39,15 @@ func test_dynamic_lookup_f(_ obj: AnyObject) {
   }
 }
 
+func test_dynamic_lookup_f_unbound(_ obj: AnyObject) {
+  var of = AnyObject.f(obj)
+  if of != nil {
+    of!()
+  } else {
+    print("Object does not respond to the selector \"f\".\n", terminator: "")
+  }
+}
+
 func test_dynamic_lookup_g(_ obj: AnyObject) {
   var og = type(of: obj).g
   if og != nil {
@@ -64,6 +73,15 @@ test_dynamic_lookup_f(Y())
 // CHECK: Z.f()
 test_dynamic_lookup_f(Z())
 
+// CHECK-NEXT: (AnyObject) -> Optional<() -> ()>
+print(type(of: AnyObject.f))
+// CHECK-NEXT: X.f()
+test_dynamic_lookup_f_unbound(X())
+// CHECK-NEXT: Object does not respond to the selector "f"
+test_dynamic_lookup_f_unbound(Y())
+// CHECK-NEXT: Z.f()
+test_dynamic_lookup_f_unbound(Z())
+
 // CHECK: Class does not respond to the selector "g"
 test_dynamic_lookup_g(X())
 // CHECK: Y.g()

test/SILGen/dynamic_lookup.swift

@@ -117,6 +117,23 @@ func opt_to_class(_ obj: AnyObject) {
   // CHECK:   return [[RESULT]] : $()
 }
 
+// CHECK-LABEL: sil hidden [ossa] @$s14dynamic_lookup20opt_to_class_unboundyyF : $@convention(thin) () -> () {
+// CHECK: bb0:
+// CHECK:   metatype $@thin AnyObject.Protocol
+// CHECK:   function_ref @$[[THUNK_NAME:[_a-zA-Z0-9]+]]
+// CHECK: } // end sil function '$s14dynamic_lookup20opt_to_class_unboundyyF'
+//
+// CHECK: sil private [ossa] @$[[THUNK_NAME]] : $@convention(thin) (@guaranteed AnyObject) -> @owned Optional<@callee_guaranteed () -> ()> {
+// CHECK: bb0(%0 : @guaranteed $AnyObject):
+// CHECK:   [[OPENED:%[0-9]+]] = open_existential_ref %0 : $AnyObject to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) AnyObject]]
+// CHECK:   [[OPENED_COPY:%[0-9]+]] = copy_value [[OPENED]]
+// CHECK:   alloc_stack $Optional<@callee_guaranteed () -> ()>
+// CHECK:   dynamic_method_br [[OPENED_COPY]] : $[[OPENED_TY]], #X.f!foreign, bb1, bb2
+// CHECK: } // end sil function '$[[THUNK_NAME]]'
+func opt_to_class_unbound() {
+  let f = AnyObject.f
+}
+
 // CHECK-LABEL: sil hidden [ossa] @$s14dynamic_lookup20forced_without_outer{{[_0-9a-zA-Z]*}}F
 func forced_without_outer(_ obj: AnyObject) {
   // CHECK: dynamic_method_br

test/SILGen/subclass_existentials.swift

@@ -179,6 +179,36 @@ func methodCalls(
   // CHECK-NEXT: }
 }
 
+
+// CHECK-LABEL: sil hidden [ossa] @$s21subclass_existentials29methodCallsOnProtocolMetatypeyyF : $@convention(thin) () -> () {
+// CHECK: metatype $@thin (Base<Int> & P).Protocol
+// CHECK: function_ref @$[[THUNK1_NAME:[_a-zA-Z0-9]+]]
+// CHECK: } // end sil function '$s21subclass_existentials29methodCallsOnProtocolMetatypeyyF'
+//
+// CHECK: sil private [ossa] @$[[THUNK1_NAME]] : $@convention(thin) (@guaranteed Base<Int> & P) -> @owned @callee_guaranteed () -> @owned Base<Int> & P {
+// CHECK: bb0(%0 : @guaranteed $Base<Int> & P):
+// CHECK:   [[THUNK2:%[0-9]+]] = function_ref @$[[THUNK2_NAME:[_a-zA-Z0-9]+]]
+// CHECK:   [[SELF_COPY:%[0-9]+]] = copy_value %0
+// CHECK:   [[RESULT:%[0-9]+]] = partial_apply [callee_guaranteed] [[THUNK2]]([[SELF_COPY]])
+// CHECK:  return [[RESULT]]
+// CHECK: } // end sil function '$[[THUNK1_NAME]]'
+//
+// CHECK: sil private [ossa] @$[[THUNK2_NAME]] : $@convention(thin) (@guaranteed Base<Int> & P) -> @owned Base<Int> & P {
+// CHECK: bb0(%0 : @guaranteed $Base<Int> & P):
+// CHECK:  [[OPENED:%[0-9]+]] = open_existential_ref %0 : $Base<Int> & P to $[[OPENED_TY:@opened\("[-A-F0-9]+"\) Base<Int> & P]]
+// CHECK:  [[OPENED_COPY:%[0-9]+]] = copy_value [[OPENED]]
+// CHECK:  [[CLASS:%[0-9]+]] = upcast [[OPENED_COPY]] : $[[OPENED_TY]] to $Base<Int>
+// CHECK:  [[METHOD:%[0-9]+]] = class_method [[CLASS]] : $Base<Int>, #Base.classSelfReturn
+// CHECK:  [[RESULT:%[0-9]+]] = apply [[METHOD]]<Int>([[CLASS]]) : $@convention(method) <τ_0_0> (@guaranteed Base<τ_0_0>) -> @owned Base<τ_0_0>
+// CHECK:  destroy_value [[CLASS]]
+// CHECK:  [[TO_OPENED:%[0-9]+]] = unchecked_ref_cast [[RESULT]] : $Base<Int> to $[[OPENED_TY]]
+// CHECK:  [[ERASED:%[0-9]+]] = init_existential_ref [[TO_OPENED]] : $[[OPENED_TY]]
+// CHECK:  return [[ERASED]]
+// CHECK: } // end sil function '$[[THUNK2_NAME]]'
+func methodCallsOnProtocolMetatype() {
+  let _ = (Base<Int> & P).classSelfReturn
+}
+
 protocol PropertyP {
   var p: PropertyP & PropertyC { get set }