CSApply: Fix unbound ref to class method on protocol composition metatype

AnthonyLatsis · AnthonyLatsis · commit c91690b05398 · 2022-04-12T23:49:33.000+03:00
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -1002,7 +1002,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 +1162,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 +1290,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 +1328,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 +1342,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 +1359,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,
@@ -1452,24 +1492,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 (!isPartialApplication ||
+            (!member->getDeclContext()->getSelfProtocolDecl() &&
+             !isUnboundInstanceMember)) {
           // Open the existential before performing the member reference.
           base = openExistentialReference(base, knownOpened->second, member);
           baseTy = knownOpened->second;
@@ -1508,13 +1552,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)
@@ -1635,7 +1681,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',
@@ -1672,17 +1718,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 +1752,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()) {
diff --git a/test/SILGen/subclass_existentials.swift b/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 }
 
diff --git a/test/Sema/dynamic_self_implicit_conversions.swift b/test/Sema/dynamic_self_implicit_conversions.swift
@@ -1,5 +1,6 @@
 // RUN: %target-swift-frontend -typecheck -dump-ast %s | %FileCheck %s
 
+// FIXME: Make this a SILGen test instead.
 // Even though redundant conversions are eventually optimized away, test from
 // the get-go that we build these implicit conversions only when necessary.
 protocol P {}
@@ -35,9 +36,9 @@ class B: A {
     _ = super.staticMethod()
     // CHECK: covariant_function_conversion_expr implicit type='() -> Self' location={{.*}}.swift:[[@LINE+1]]
     _ = super.staticMethod
-    // FIXME: This could be a single conversion.
-    // CHECK: function_conversion_expr implicit type='(Self) -> () -> Self' location={{.*}}.swift:[[@LINE+2]]
-    // CHECK: covariant_function_conversion_expr implicit type='(A) -> () -> Self' location={{.*}}.swift:[[@LINE+1]]
+    // CHECK-NOT: function_conversion_expr {{.*}} location={{.*}}.swift:[[@LINE+3]]
+    // CHECK-NOT: covariant_function_conversion_expr {{.*}} location={{.*}}.swift:[[@LINE+2]]
+    // CHECK: covariant_return_conversion_expr implicit type='Self' location={{.*}}.swift:[[@LINE+1]]
     _ = self.method
     // CHECK: covariant_function_conversion_expr implicit type='() -> Self' location={{.*}}.swift:[[@LINE+1]]
     _ = self.init
