Merge pull request #80889 from nate-chandler/rdar149352777

[CoroutineAccessors] Require underscored override.

Author: nate-chandler

include/swift/AST/AvailabilityContext.h

@@ -75,6 +75,9 @@ class AvailabilityContext {
   /// of the given declaration.
   static AvailabilityContext forDeclSignature(const Decl *decl);
 
+  /// Returns the unconstrained availability context.
+  static AvailabilityContext forAlwaysAvailable(const ASTContext &ctx);
+
   /// Returns the range of platform versions which may execute code in the
   /// availability context, starting at its introduction version.
   // FIXME: [availability] Remove; superseded by getAvailableRange().

lib/AST/AvailabilityContext.cpp

@@ -214,6 +214,13 @@ AvailabilityContext AvailabilityContext::forDeclSignature(const Decl *decl) {
   return forLocation(decl->getLoc(), decl->getInnermostDeclContext());
 }
 
+AvailabilityContext
+AvailabilityContext::forAlwaysAvailable(const ASTContext &ctx) {
+  return AvailabilityContext(Storage::get(AvailabilityRange::alwaysAvailable(),
+                                          /*isDeprecated=*/false,
+                                          /*domainInfos=*/{}, ctx));
+}
+
 AvailabilityRange AvailabilityContext::getPlatformRange() const {
   return storage->platformRange;
 }

lib/Sema/TypeCheckStorage.cpp

@@ -2670,25 +2670,42 @@ RequiresOpaqueAccessorsRequest::evaluate(Evaluator &evaluator,
 ///
 /// The underscored accessor could, however, still be required for ABI
 /// stability.
-bool AbstractStorageDecl::requiresCorrespondingUnderscoredCoroutineAccessor(
-    AccessorKind kind, AccessorDecl const *decl) const {
-  auto &ctx = getASTContext();
+static bool requiresCorrespondingUnderscoredCoroutineAccessorImpl(
+    AbstractStorageDecl const *storage, AccessorKind kind,
+    AccessorDecl const *decl, AbstractStorageDecl const *derived) {
+  auto &ctx = storage->getASTContext();
   assert(ctx.LangOpts.hasFeature(Feature::CoroutineAccessors));
   assert(kind == AccessorKind::Modify2 || kind == AccessorKind::Read2);
 
+  // If any overridden decl requires the underscored version, then this decl
+  // does too.  Otherwise dispatch to the underscored version on a value
+  // statically the super but dynamically this subtype would not dispatch to an
+  // override of the underscored version but rather (incorrectly) the
+  // supertype's implementation.
+  if (storage == derived) {
+    auto *current = storage;
+    while ((current = current->getOverriddenDecl())) {
+      auto *currentDecl = cast_or_null<AccessorDecl>(
+          decl ? decl->getOverriddenDecl() : nullptr);
+      if (requiresCorrespondingUnderscoredCoroutineAccessorImpl(
+              current, kind, currentDecl, derived)) {
+        return true;
+      }
+    }
+  }
+
   // Non-stable modules have no ABI to keep stable.
-  if (getModuleContext()->getResilienceStrategy() !=
+  if (storage->getModuleContext()->getResilienceStrategy() !=
       ResilienceStrategy::Resilient)
     return false;
 
   // Non-exported storage has no ABI to keep stable.
-  if (!isExported(this))
+  if (!isExported(storage))
     return false;
 
   // The non-underscored accessor is not present, the underscored accessor
   // won't be either.
-  // TODO: CoroutineAccessors: What if only the underscored is written out?
-  auto *accessor = decl ? decl : getOpaqueAccessor(kind);
+  auto *accessor = decl ? decl : storage->getOpaqueAccessor(kind);
   if (!accessor)
     return false;
 
@@ -2699,17 +2716,43 @@ bool AbstractStorageDecl::requiresCorrespondingUnderscoredCoroutineAccessor(
   if (!ctx.supportsVersionedAvailability())
     return true;
 
-  auto modifyAvailability = AvailabilityContext::forLocation({}, accessor);
+  AvailabilityContext accessorAvailability = [&] {
+    if (storage->getModuleContext()->isMainModule()) {
+      return AvailabilityContext::forDeclSignature(accessor);
+    }
+    // Calculate the availability of the imported declaration ourselves starting
+    // from always available and constraining by walking the enclosing decl
+    // contexts.
+    auto retval = AvailabilityContext::forAlwaysAvailable(ctx);
+    auto declContext = storage->getInnermostDeclContext();
+    while (declContext) {
+      const Decl *decl = declContext->getInnermostDeclarationDeclContext();
+      if (!decl)
+        break;
+
+      retval.constrainWithDecl(decl);
+      declContext = decl->getDeclContext();
+    }
+    return retval;
+  }();
   auto featureAvailability = ctx.getCoroutineAccessorsAvailability();
   // If accessor was introduced only after the feature was, there's no old ABI
   // to maintain.
-  if (modifyAvailability.getPlatformRange().isContainedIn(featureAvailability))
+  if (accessorAvailability.getPlatformRange().isContainedIn(
+          featureAvailability))
     return false;
 
   // The underscored accessor is required for ABI stability.
   return true;
 }
 
+bool AbstractStorageDecl::requiresCorrespondingUnderscoredCoroutineAccessor(
+    AccessorKind kind, AccessorDecl const *decl) const {
+  return requiresCorrespondingUnderscoredCoroutineAccessorImpl(
+      this, kind, decl,
+      /*derived=*/this);
+}
+
 bool RequiresOpaqueModifyCoroutineRequest::evaluate(
     Evaluator &evaluator, AbstractStorageDecl *storage,
     bool isUnderscored) const {

test/SILGen/coroutine_accessors.swift

@@ -160,6 +160,52 @@ mutating func update(irm newValue: Int) throws -> Int {
   try coroutine_accessors.update(at: &irm, to: newValue)
 }
 
+public var i_r_m: Int {
+// CHECK-LABEL: sil{{.*}} [ossa] @$s19coroutine_accessors1SV5i_r_mSivr :
+// CHECK-SAME:      $@yield_once
+// CHECK-SAME:      @convention(method)
+// CHECK-SAME:      (@guaranteed S)
+// CHECK-SAME:      ->
+// CHECK-SAME:      @yields Int
+// CHECK-SAME:  {
+// CHECK:       } // end sil function '$s19coroutine_accessors1SV5i_r_mSivr'
+
+  _read {
+    yield _i
+  }
+// CHECK-NOT:   sil [ossa] @$s19coroutine_accessors1SV5i_r_mSivx :
+// CHECK-LABEL: sil {{.*}}[ossa] @$s19coroutine_accessors1SV5i_r_mSivM :
+// CHECK-SAME:      $@yield_once
+// CHECK-SAME:      @convention(method)
+// CHECK-SAME:      (@inout S)
+// CHECK-SAME:      ->
+// CHECK-SAME:      @yields @inout Int
+// CHECK-SAME:  {
+// CHECK:       } // end sil function '$s19coroutine_accessors1SV5i_r_mSivM'
+  _modify {
+    yield &_i
+  }
+// CHECK-LABEL: sil {{.*}}[ossa] @$s19coroutine_accessors1SV5i_r_mSivs :
+// CHECK-SAME:      $@convention(method)
+// CHECK-SAME:      (Int, @inout S)
+// CHECK-SAME:      ->
+// CHECK-SAME:      ()
+// CHECK-SAME:  {
+// CHECK:       bb0(
+// CHECK-SAME:      [[NEW_VALUE:%[^,]+]] :
+// CHECK-SAME:      [[SELF:%[^,]+]] :
+// CHECK-SAME:  ):
+// CHECK:         [[SELF_ACCESS:%[^,]+]] = begin_access [modify] [unknown] [[SELF]]
+// CHECK:         [[MODIFY_ACCESSOR:%[^,]+]] = function_ref @$s19coroutine_accessors1SV5i_r_mSivM
+// CHECK:         ([[VALUE_ADDRESS:%[^,]+]],
+// CHECK-SAME:     [[TOKEN:%[^)]+]])
+// CHECK-SAME:    = begin_apply [[MODIFY_ACCESSOR]]([[SELF_ACCESS]])
+// CHECK:         assign [[NEW_VALUE:%[^,]+]] to [[VALUE_ADDRESS]]
+// CHECK:         end_apply [[TOKEN]]
+// CHECK:         end_access [[SELF_ACCESS]]
+// CHECK-LABEL:} // end sil function '$s19coroutine_accessors1SV5i_r_mSivs'
+} // public var irm
+
 } // public struct S
 
 enum E : Error {

test/SILGen/coroutine_accessors_availability.swift

@@ -223,6 +223,21 @@ public func modifyOldNoninlinableNew(_ n: inout StructOld) {
 public func takeInt(_ i: Int) {
 }
 
+open class BaseClassOld {
+  public init(_ i : Int) {
+    self._i = i
+  }
+  var _i: Int
+  open var i: Int {
+    read {
+      yield _i
+    }
+    modify {
+      yield &_i
+    }
+  }
+}
+
 //--- Downstream.swift
 
 import Library
@@ -348,3 +363,28 @@ func modifyOldOld() {
 
   n.i.increment()
 }
+
+public class DerivedOldFromBaseClassOld : BaseClassOld {
+  public init(_ i : Int, _ j : Int) {
+    self._j = j
+    super.init(i)
+  }
+  var _j: Int
+  override public var i: Int {
+    read {
+      yield _j
+    }
+    modify {
+      yield &_j
+    }
+  }
+}
+
+// CHECK-LABEL: sil_vtable [serialized] DerivedOldFromBaseClassOld {
+// CHECK-NEXT:    #BaseClassOld.init!allocator
+// CHECK-NEXT:    #BaseClassOld.i!read
+// CHECK-NEXT:    #BaseClassOld.i!read2
+// CHECK-NEXT:    #BaseClassOld.i!setter
+// CHECK-NEXT:    #BaseClassOld.i!modify
+// CHECK-NEXT:    #BaseClassOld.i!modify2
+// CHECK:       }