From ceb17b7f2d462582e71e3fe33b1ac765ee250cda Mon Sep 17 00:00:00 2001
From: Joe Groff <jgroff@apple.com>
Date: Wed, 1 Jun 2022 16:54:38 -0700
Subject: [PATCH] SILGen: Don't reference external property descriptors for
 @_alwaysEmitIntoClient properties.

Their definition is fully visible to clients to be copied into them, and there isn't necessarily
a symbol for the property descriptor in the defining module, so it isn't necessary or desirable to
try to use a property descriptor with them. Trying to reference the descriptor leads to missing
symbol errors at load time when trying to use keypaths with older versions of the defining dylib,
which goes against the purpose of `@_alwaysEmitIntoClient` meaning "no ABI liabilities for the
defining module". Fixes rdar://94049160.
---
 lib/SILGen/SILGenExpr.cpp                     | 56 ++++++++++++++-----
 .../Evolution/Inputs/keypaths.swift.gyb       |  6 ++
 .../Evolution/test_keypaths.swift.gyb         | 12 ++++
 3 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/lib/SILGen/SILGenExpr.cpp b/lib/SILGen/SILGenExpr.cpp
index 96044f671134d..aa6b9a5b3bd5f 100644
--- a/lib/SILGen/SILGenExpr.cpp
+++ b/lib/SILGen/SILGenExpr.cpp
@@ -3580,21 +3580,47 @@ SILGenModule::emitKeyPathComponentForDecl(SILLocation loc,
   /// Returns true if a key path component for the given property or
   /// subscript should be externally referenced.
   auto shouldUseExternalKeyPathComponent = [&]() -> bool {
-    return (!forPropertyDescriptor &&
-            (baseDecl->getModuleContext() != SwiftModule ||
-             baseDecl->isResilient(SwiftModule, expansion)) &&
-            // Protocol requirements don't have nor need property descriptors.
-            !isa<ProtocolDecl>(baseDecl->getDeclContext()) &&
-            // Properties that only dispatch via ObjC lookup do not have nor
-            // need property descriptors, since the selector identifies the
-            // storage.
-            // Properties that are not public don't need property descriptors
-            // either.
-            (!baseDecl->requiresOpaqueAccessors() ||
-             (!getAccessorDeclRef(getRepresentativeAccessorForKeyPath(baseDecl))
-                   .isForeign &&
-              getAccessorDeclRef(getRepresentativeAccessorForKeyPath(baseDecl))
-                      .getLinkage(ForDefinition) <= SILLinkage::PublicNonABI)));
+    // The property descriptor has the canonical key path component information
+    // so doesn't have to refer to another external descriptor.
+    if (forPropertyDescriptor) {
+      return false;
+    }
+    
+    // Don't need to use the external component if we're inside the resilience
+    // domain of its defining module.
+    if (baseDecl->getModuleContext() == SwiftModule
+        && !baseDecl->isResilient(SwiftModule, expansion)) {
+      return false;
+    }
+
+    // Protocol requirements don't have nor need property descriptors.
+    if (isa<ProtocolDecl>(baseDecl->getDeclContext())) {
+      return false;
+    }
+    
+    // Always-emit-into-client properties can't reliably refer to a property
+    // descriptor that may not exist in older versions of their home dylib.
+    // Their definition is also always entirely visible to clients so it isn't
+    // needed.
+    if (baseDecl->getAttrs().hasAttribute<AlwaysEmitIntoClientAttr>()) {
+      return false;
+    }
+
+    // Properties that only dispatch via ObjC lookup do not have nor
+    // need property descriptors, since the selector identifies the
+    // storage.
+    // Properties that are not public don't need property descriptors
+    // either.
+    if (baseDecl->requiresOpaqueAccessors()) {
+      auto representative = getAccessorDeclRef(
+                           getRepresentativeAccessorForKeyPath(baseDecl));
+      if (representative.isForeign)
+        return false;
+      if (representative.getLinkage(ForDefinition) > SILLinkage::PublicNonABI)
+        return false;
+    }
+    
+    return true;
   };
 
   auto strategy = storage->getAccessStrategy(AccessSemantics::Ordinary,
diff --git a/validation-test/Evolution/Inputs/keypaths.swift.gyb b/validation-test/Evolution/Inputs/keypaths.swift.gyb
index 113c942be4a89..a2f05e8a8c505 100644
--- a/validation-test/Evolution/Inputs/keypaths.swift.gyb
+++ b/validation-test/Evolution/Inputs/keypaths.swift.gyb
@@ -123,3 +123,9 @@ public final class AFinalClass {
 
 }
 
+public struct AEIC {
+#if AFTER
+    @_alwaysEmitIntoClient
+    public var aeic: Int { return 0 }
+#endif
+}
diff --git a/validation-test/Evolution/test_keypaths.swift.gyb b/validation-test/Evolution/test_keypaths.swift.gyb
index c67cb46f5b47e..3be7d30c57af3 100644
--- a/validation-test/Evolution/test_keypaths.swift.gyb
+++ b/validation-test/Evolution/test_keypaths.swift.gyb
@@ -102,4 +102,16 @@ tests.test("AFinalClass.${name}") {
 
 % end
 
+#if AFTER
+tests.test("AlwaysEmitIntoClient") {
+  // AEIC declarations are vendored into their importing module, so
+  // equality doesn't currently work reliably with them. We can however
+  // test that the reference to the property works and is backward-compatible
+  // when using the `after` test suite with the `before` dylib.
+  let kp = \AEIC.aeic
+
+  expectEqual(kp, kp)
+}
+#endif
+
 runAllTests()