[CoroutineAccessors] Fix overriding.

include/swift/AST/Decl.h

@@ -5908,7 +5908,7 @@ class AbstractStorageDecl : public ValueDecl {
   friend class SetterAccessLevelRequest;
   friend class IsGetterMutatingRequest;
   friend class IsSetterMutatingRequest;
-  friend class OpaqueReadOwnershipRequest;
+  friend class DirectOpaqueReadOwnershipRequest;
   friend class StorageImplInfoRequest;
   friend class RequiresOpaqueAccessorsRequest;
   friend class RequiresOpaqueModifyCoroutineRequest;
@@ -6254,9 +6254,7 @@ class AbstractStorageDecl : public ValueDecl {
   bool requiresOpaqueAccessor(AccessorKind kind) const;
 
   /// Does this storage require a 'get' accessor in its opaque-accessors set?
-  bool requiresOpaqueGetter() const {
-    return getOpaqueReadOwnership() != OpaqueReadOwnership::Borrowed;
-  }
+  bool requiresOpaqueGetter() const;
 
   /// Does this storage require a '_read' accessor in its opaque-accessors set?
   bool requiresOpaqueReadCoroutine() const;
@@ -6275,10 +6273,9 @@ class AbstractStorageDecl : public ValueDecl {
   /// set?
   bool requiresOpaqueModify2Coroutine() const;
 
-  /// Given that CoroutineAccessors is enabled, is _read/_modify required for
-  /// ABI stability?
-  bool requiresCorrespondingUnderscoredCoroutineAccessor(
-      AccessorKind kind, AccessorDecl const *decl = nullptr) const;
+  /// Given that CoroutineAccessors is enabled, are the accessors still
+  /// required that would have been emitted when its not?
+  bool requiresAccessorsForABICompatibilityWithPreCoroutineAccessors() const;
 
   /// Does this storage have any explicit observers (willSet or didSet) attached
   /// to it?

include/swift/AST/TypeCheckRequests.h

@@ -1750,10 +1750,10 @@ class IsSetterMutatingRequest :
 };
 
 /// Request whether reading the storage yields a borrowed value.
-class OpaqueReadOwnershipRequest :
-    public SimpleRequest<OpaqueReadOwnershipRequest,
-                         OpaqueReadOwnership(AbstractStorageDecl *),
-                         RequestFlags::SeparatelyCached> {
+class DirectOpaqueReadOwnershipRequest
+    : public SimpleRequest<DirectOpaqueReadOwnershipRequest,
+                           OpaqueReadOwnership(AbstractStorageDecl *),
+                           RequestFlags::SeparatelyCached> {
 public:
   using SimpleRequest::SimpleRequest;
 

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -255,7 +255,7 @@ SWIFT_REQUEST(TypeChecker, EnumElementExprPatternRequest,
 SWIFT_REQUEST(TypeChecker, ResolvePatternRequest,
               Pattern *(Pattern *, DeclContext *, bool),
               Cached, NoLocationInfo)
-SWIFT_REQUEST(TypeChecker, OpaqueReadOwnershipRequest,
+SWIFT_REQUEST(TypeChecker, DirectOpaqueReadOwnershipRequest,
               OpaqueReadOwnership(AbstractStorageDecl *), SeparatelyCached,
               NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, OpaqueResultTypeRequest,

lib/AST/ASTPrinter.cpp

@@ -2672,6 +2672,10 @@ void PrintAST::printAccessors(const AbstractStorageDecl *ASD) {
           Options.SuppressCoroutineAccessors) {
         AddAccessorToPrint(AccessorKind::Read);
       }
+      if (ASD->getAccessor(AccessorKind::Get) &&
+          Options.SuppressCoroutineAccessors) {
+        AddAccessorToPrint(AccessorKind::Get);
+      }
       AddAccessorToPrint(AccessorKind::Read2);
       break;
     case ReadImplKind::Borrow:

lib/AST/Decl.cpp

@@ -3250,14 +3250,10 @@ static AccessStrategy getOpaqueReadAccessStrategy(
     ResilienceExpansion expansion,
     std::optional<std::pair<SourceRange, const DeclContext *>> location,
     bool useOldABI) {
-  if (useOldABI) {
-    assert(storage->requiresOpaqueRead2Coroutine());
-    assert(storage->requiresOpaqueReadCoroutine());
-    return AccessStrategy::getAccessor(AccessorKind::Read, dispatch);
-  }
   if (storage->requiresOpaqueRead2Coroutine() &&
       mayReferenceUseCoroutineAccessorOnStorage(module, expansion, location,
-                                                storage))
+                                                storage) &&
+      !useOldABI)
     return AccessStrategy::getAccessor(AccessorKind::Read2, dispatch);
   if (storage->requiresOpaqueReadCoroutine())
     return AccessStrategy::getAccessor(AccessorKind::Read, dispatch);
@@ -3276,14 +3272,10 @@ static AccessStrategy getOpaqueReadWriteAccessStrategy(
     ResilienceExpansion expansion,
     std::optional<std::pair<SourceRange, const DeclContext *>> location,
     bool useOldABI) {
-  if (useOldABI) {
-    assert(storage->requiresOpaqueModify2Coroutine());
-    assert(storage->requiresOpaqueModifyCoroutine());
-    return AccessStrategy::getAccessor(AccessorKind::Modify, dispatch);
-  }
   if (storage->requiresOpaqueModify2Coroutine() &&
       mayReferenceUseCoroutineAccessorOnStorage(module, expansion, location,
-                                                storage))
+                                                storage) &&
+      !useOldABI)
     return AccessStrategy::getAccessor(AccessorKind::Modify2, dispatch);
   if (storage->requiresOpaqueModifyCoroutine())
     return AccessStrategy::getAccessor(AccessorKind::Modify, dispatch);
@@ -3440,17 +3432,23 @@ bool AbstractStorageDecl::requiresOpaqueSetter() const {
   return true;
 }
 
+bool AbstractStorageDecl::requiresOpaqueGetter() const {
+  return getOpaqueReadOwnership() != OpaqueReadOwnership::Borrowed;
+}
+
 bool AbstractStorageDecl::requiresOpaqueReadCoroutine() const {
   ASTContext &ctx = getASTContext();
-  if (ctx.LangOpts.hasFeature(Feature::CoroutineAccessors))
-    return requiresCorrespondingUnderscoredCoroutineAccessor(
-        AccessorKind::Read2);
+  if (ctx.LangOpts.hasFeature(Feature::CoroutineAccessors) &&
+      !requiresAccessorsForABICompatibilityWithPreCoroutineAccessors()) {
+    return false;
+  }
 
   // If a borrow accessor is present, we don't need a read coroutine.
   if (getAccessor(AccessorKind::Borrow)) {
     return false;
   }
-  return getOpaqueReadOwnership() != OpaqueReadOwnership::Owned;
+
+  return getOpaqueReadOwnership() == OpaqueReadOwnership::Borrowed;
 }
 
 bool AbstractStorageDecl::requiresOpaqueRead2Coroutine() const {
@@ -3722,10 +3720,25 @@ AbstractStorageDecl::mutabilityInSwift(
   return mutability(useDC, base);
 }
 
+static OpaqueReadOwnership
+getDirectOpaqueReadOwnership(AbstractStorageDecl const *self) {
+  ASTContext &ctx = self->getASTContext();
+  return evaluateOrDefault(
+      ctx.evaluator,
+      DirectOpaqueReadOwnershipRequest{const_cast<AbstractStorageDecl *>(self)},
+      {});
+}
+
 OpaqueReadOwnership AbstractStorageDecl::getOpaqueReadOwnership() const {
-  ASTContext &ctx = getASTContext();
-  return evaluateOrDefault(ctx.evaluator,
-    OpaqueReadOwnershipRequest{const_cast<AbstractStorageDecl *>(this)}, {});
+  auto *root = this;
+  while (true) {
+    auto *super = root->getOverriddenDecl();
+    if (!super)
+      break;
+    root = super;
+  }
+
+  return getDirectOpaqueReadOwnership(root);
 }
 
 bool ValueDecl::isInstanceMember() const {
@@ -11342,8 +11355,8 @@ bool AccessorDecl::isRequirementWithSynthesizedDefaultImplementation() const {
   if (!requiresNewWitnessTableEntry()) {
     return false;
   }
-  return getStorage()->requiresCorrespondingUnderscoredCoroutineAccessor(
-      getAccessorKind(), this);
+  return getStorage()
+      ->requiresAccessorsForABICompatibilityWithPreCoroutineAccessors();
 }
 
 bool AccessorDecl::doesAccessorHaveBody() const {

lib/AST/TypeCheckRequests.cpp

@@ -774,18 +774,19 @@ void IsSetterMutatingRequest::cacheResult(bool value) const {
 }
 
 //----------------------------------------------------------------------------//
-// OpaqueReadOwnershipRequest computation.
+// DirectOpaqueReadOwnershipRequest computation.
 //----------------------------------------------------------------------------//
 
 std::optional<OpaqueReadOwnership>
-OpaqueReadOwnershipRequest::getCachedResult() const {
+DirectOpaqueReadOwnershipRequest::getCachedResult() const {
   auto *storage = std::get<0>(getStorage());
   if (storage->LazySemanticInfo.OpaqueReadOwnershipComputed)
     return OpaqueReadOwnership(storage->LazySemanticInfo.OpaqueReadOwnership);
   return std::nullopt;
 }
 
-void OpaqueReadOwnershipRequest::cacheResult(OpaqueReadOwnership value) const {
+void DirectOpaqueReadOwnershipRequest::cacheResult(
+    OpaqueReadOwnership value) const {
   auto *storage = std::get<0>(getStorage());
   storage->setOpaqueReadOwnership(value);
 }

lib/SILGen/SILGenType.cpp

@@ -1159,13 +1159,16 @@ void SILGenModule::emitDefaultWitnessTable(ProtocolDecl *protocol) {
 
 namespace {
 
-std::optional<AccessorKind>
-originalAccessorKindForReplacementKind(AccessorKind kind) {
+bool originalAccessorKindsForReplacementKind(
+    AccessorKind kind, SmallVectorImpl<AccessorKind> &kinds) {
   switch (kind) {
   case AccessorKind::Read2:
-    return {AccessorKind::Read};
+    kinds.push_back(AccessorKind::Read);
+    kinds.push_back(AccessorKind::Get);
+    return true;
   case AccessorKind::Modify2:
-    return {AccessorKind::Modify};
+    kinds.push_back(AccessorKind::Modify);
+    return true;
   case AccessorKind::Get:
   case AccessorKind::DistributedGet:
   case AccessorKind::Set:
@@ -1178,7 +1181,7 @@ originalAccessorKindForReplacementKind(AccessorKind kind) {
   case AccessorKind::Init:
   case AccessorKind::Borrow:
   case AccessorKind::Mutate:
-    return std::nullopt;
+    return false;
   }
 }
 
@@ -1213,13 +1216,21 @@ class SILGenDefaultOverrideTable
     if (!accessor) {
       return std::nullopt;
     }
-    // Specifically, read2 can replace _read and modify2 can replace _modify.
-    auto originalKind =
-        originalAccessorKindForReplacementKind(accessor->getAccessorKind());
-    if (!originalKind) {
+    // Specifically, read2 can replace _read or get and modify2 can replace
+    // _modify.
+    SmallVector<AccessorKind, 2> originalKinds;
+    if (!originalAccessorKindsForReplacementKind(accessor->getAccessorKind(),
+                                                 originalKinds)) {
       return std::nullopt;
     }
-    auto *originalDecl = accessor->getStorage()->getAccessor(*originalKind);
+    assert(originalKinds.size() > 0);
+    AccessorDecl *originalDecl = nullptr;
+    for (auto originalKind : originalKinds) {
+      originalDecl = accessor->getStorage()->getAccessor(originalKind);
+      if (originalDecl) {
+        break;
+      }
+    }
     if (!originalDecl) {
       return std::nullopt;
     }
@@ -1326,9 +1337,7 @@ SILFunction *SILGenModule::emitDefaultOverride(SILDeclRef replacement,
   assert(indirectErrors.size() == 0 &&
          "coroutine accessor with indirect error!?");
   SmallVector<SILValue> args;
-  for (auto result : indirectResults) {
-    args.push_back(result.forward(SGF));
-  }
+  ASSERT(indirectResults.empty());
   // Indirect errors would go here, but we don't currently support
   // throwing coroutines.
   if (implicitIsolationParam.isValid()) {
@@ -1337,15 +1346,68 @@ SILFunction *SILGenModule::emitDefaultOverride(SILDeclRef replacement,
   for (auto param : params) {
     args.push_back(param.forward(SGF));
   }
-  auto *bai = SGF.B.createBeginApply(loc, originalFn, subs, args);
-  auto *token = bai->getTokenResult();
-  auto yieldedValues = bai->getYieldedValues();
+  struct Get {
+    struct Direct {
+      SILValue value;
+    };
+    struct Indirect {
+      AllocStackInst *asi;
+    };
+    using Result = TaggedUnion<Direct, Indirect>;
+    SmallVector<Result, 2> results;
+  };
+  struct Read {
+    BeginApplyInst *bai;
+  };
+  SmallVector<SILValue> yields;
+  using Apply = TaggedUnion<Get, Read>;
+  Apply apply = [&] {
+    if (originalTy->getCoroutineKind() == SILCoroutineKind::None) {
+      auto get = Get{{}};
+      SmallVector<AllocStackInst *, 2> indirectResults;
+      auto indirectResultIterator = args.begin();
+      for (auto result : originalConvention.getIndirectSILResults()) {
+        auto ty = originalConvention.getSILType(
+            result, function->getTypeExpansionContext());
+        ty = function->mapTypeIntoContext(ty);
+        auto *asi = SGF.B.createAllocStack(loc, ty);
+        args.insert(indirectResultIterator, asi);
+        indirectResultIterator = std::next(indirectResultIterator);
+        indirectResults.push_back(asi);
+      }
+      auto *ai = SGF.B.createApply(loc, originalFn, subs, args);
+      auto directResults = ai->getResults();
+      size_t directIndex = 0;
+      size_t indirectIndex = 0;
+      for (auto result : originalConvention.getResults()) {
+        if (result.isFormalIndirect()) {
+          ASSERT(indirectIndex < indirectResults.size());
+          auto indirectResult = indirectResults[indirectIndex];
+          get.results.push_back(Get::Indirect{indirectResult});
+          yields.push_back(indirectResult);
+          indirectIndex += 1;
+          continue;
+        }
+        ASSERT(directIndex < directResults.size());
+        auto directResult = directResults[directIndex];
+        get.results.push_back(Get::Direct{directResult});
+        yields.push_back(directResult);
+        directIndex += 1;
+      }
+      return Apply{get};
+    }
+
+    assert(originalTy->getCoroutineKind() == SILCoroutineKind::YieldOnce);
+    auto *bai = SGF.B.createBeginApply(loc, originalFn, subs, args);
+    auto yieldedValues = bai->getYieldedValues();
+    for (auto yielded : yieldedValues) {
+      yields.push_back(yielded);
+    }
+    return Apply{Read{bai}};
+  }();
+
   auto *normalBlock = function->createBasicBlockAfter(SGF.B.getInsertionBB());
   auto *errorBlock = function->createBasicBlockAfter(normalBlock);
-  SmallVector<SILValue> yields;
-  for (auto yielded : yieldedValues) {
-    yields.push_back(yielded);
-  }
   SGF.B.createYield(loc, yields, normalBlock, errorBlock);
 
   SGF.B.setInsertionPoint(normalBlock);
@@ -1374,12 +1436,38 @@ SILFunction *SILGenModule::emitDefaultOverride(SILDeclRef replacement,
     break;
   }
   }
-  SGF.B.createEndApply(loc, token, resultTy);
+  if (auto *get = apply.dyn_cast<Get>()) {
+    for (auto result : llvm::reverse(get->results)) {
+      if (auto *direct = result.dyn_cast<Get::Direct>()) {
+        SGF.B.emitDestroyValueOperation(loc, direct->value);
+        continue;
+      }
+      auto indirect = result.get<Get::Indirect>();
+      SGF.B.emitDestroyAddr(loc, indirect.asi);
+      SGF.B.createDeallocStack(loc, indirect.asi);
+    }
+  } else {
+    auto read = apply.get<Read>();
+    SGF.B.createEndApply(loc, read.bai->getTokenResult(), resultTy);
+  }
   auto *retval = SGF.B.createTuple(loc, {});
   SGF.B.createReturn(loc, retval);
 
   SGF.B.setInsertionPoint(errorBlock);
-  SGF.B.createAbortApply(loc, token);
+  if (auto *get = apply.dyn_cast<Get>()) {
+    for (auto result : llvm::reverse(get->results)) {
+      if (auto *direct = result.dyn_cast<Get::Direct>()) {
+        SGF.B.emitDestroyValueOperation(loc, direct->value);
+        continue;
+      }
+      auto indirect = result.get<Get::Indirect>();
+      SGF.B.emitDestroyAddr(loc, indirect.asi);
+      SGF.B.createDeallocStack(loc, indirect.asi);
+    }
+  } else {
+    auto read = apply.get<Read>();
+    SGF.B.createAbortApply(loc, read.bai->getTokenResult());
+  }
   SGF.B.createUnwind(loc);
   return function;
 }