Merge pull request #20426 from rjmccall/class-vs-protocol-borrowed-fix

Fix a bug with class vs. protocol @_borrowed mismatches

Author: rjmccall

lib/AST/ASTVerifier.cpp

@@ -3064,10 +3064,9 @@ class Verifier : public ASTWalker {
           abort();
         }
 
-        if (AD->getAccessorKind() != AccessorKind::Read &&
-            AD->getAccessorKind() != AccessorKind::Modify) {
-          Out << "hasForcedStaticDispatch() set on accessor other than "
-                 "read or modify\n";
+        if (AD->getStorage()->requiresOpaqueAccessor(AD->getAccessorKind())) {
+          Out << "hasForcedStaticDispatch() set on accessor that's opaque "
+                 "for its storage\n";
           abort();
         }
       }

lib/Sema/CodeSynthesis.cpp

@@ -255,6 +255,9 @@ static AccessorDecl *createGetterPrototype(TypeChecker &TC,
   if (storage->isStatic())
     getter->setStatic();
 
+  if (!storage->requiresOpaqueAccessor(AccessorKind::Get))
+    getter->setForcedStaticDispatch(true);
+
   // Always add the getter to the context immediately after the storage.
   addMemberToContextIfNeeded(getter, storage->getDeclContext(), storage);
 
@@ -298,6 +301,9 @@ static AccessorDecl *createSetterPrototype(TypeChecker &TC,
   if (isStatic)
     setter->setStatic();
 
+  // All mutable storage requires a setter.
+  assert(storage->requiresOpaqueAccessor(AccessorKind::Set));
+
   // Always add the setter to the context immediately after the getter.
   if (!getter) getter = storage->getGetter();
   if (!getter) getter = storage->getReadCoroutine();
@@ -307,14 +313,6 @@ static AccessorDecl *createSetterPrototype(TypeChecker &TC,
   return setter;
 }
 
-// True if the storage is dynamic or imported from Objective-C. In these cases,
-// we need to emit static coroutine accessors that dynamically dispatch
-// to 'get' and 'set', rather than the normal dynamically dispatched
-// opaque accessors that peer dispatch to 'get' and 'set'.
-static bool needsDynamicCoroutineAccessors(AbstractStorageDecl *storage) {
-  return storage->isObjCDynamic() || storage->hasClangNode();
-}
-
 /// Mark the accessor as transparent if we can.
 ///
 /// If the storage is inside a fixed-layout nominal type, we can mark the
@@ -418,11 +416,11 @@ createCoroutineAccessorPrototype(TypeChecker &TC,
   if (storage->isFinal())
     makeFinal(ctx, accessor);
 
-  // If the storage is dynamic or ObjC-native, we can't add a dynamically-
-  // dispatched method entry for the accessor, so force it to be
-  // statically dispatched. ("final" would be inappropriate because the
-  // property can still be overridden.)
-  if (needsDynamicCoroutineAccessors(storage))
+  // If the storage does not provide this accessor as an opaque accessor,
+  // we can't add a dynamically-dispatched method entry for the accessor,
+  // so force it to be statically dispatched. ("final" would be inappropriate
+  // because the property can still be overridden.)
+  if (!storage->requiresOpaqueAccessor(kind))
     accessor->setForcedStaticDispatch(true);
 
   // Make sure the coroutine is available enough to access
@@ -2093,6 +2091,11 @@ void swift::maybeAddAccessorsToStorage(TypeChecker &TC,
 }
 
 static void synthesizeGetterBody(TypeChecker &TC, AccessorDecl *getter) {
+  if (getter->hasForcedStaticDispatch()) {
+    synthesizeTrivialGetterBody(TC, getter, TargetImpl::Ordinary);
+    return;
+  }
+
   switch (getter->getStorage()->getReadImpl()) {
   case ReadImplKind::Stored:
     synthesizeTrivialGetterBody(TC, getter);

lib/Sema/TypeCheckDeclOverride.cpp

@@ -1776,21 +1776,15 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
       auto baseAccessor = baseASD->getAccessor(kind);
       if (!baseAccessor) continue;
 
-      switch (kind) {
-      case AccessorKind::Read:
-        if (baseASD->getReadCoroutine()->hasForcedStaticDispatch())
-          continue;
-        LLVM_FALLTHROUGH;
+      if (baseAccessor->hasForcedStaticDispatch())
+        continue;
 
+      switch (kind) {
       case AccessorKind::Get:
+      case AccessorKind::Read:
         break;
 
       case AccessorKind::Modify:
-        if (baseASD->getModifyCoroutine()->hasForcedStaticDispatch())
-          continue;
-
-        LLVM_FALLTHROUGH;
-
       case AccessorKind::Set:
         // For setter accessors, we need the base's setter to be
         // accessible from the overriding context, or it's not an override.

test/SILGen/read_accessor.swift

@@ -173,3 +173,35 @@ public struct TestBorrowedProperty {
   @_borrowed
   public var borrowedString = ""
 }
+
+protocol ReadableTitle {
+  @_borrowed
+  var title: String { get }
+}
+class OverridableGetter : ReadableTitle {
+  var title: String = ""
+}
+//   The concrete read accessor is generated on-demand and does a class dispatch to the getter.
+// CHECK-LABEL: sil shared @$s13read_accessor17OverridableGetterC5titleSSvr
+// CHECK:       class_method %0 : $OverridableGetter, #OverridableGetter.title!getter.1
+// CHECK-LABEL: // end sil function '$s13read_accessor17OverridableGetterC5titleSSvr'
+//   The read witness thunk does a direct call to the concrete read accessor.
+// CHECK-LABEL: sil private [transparent] [thunk] @$s13read_accessor17OverridableGetterCAA13ReadableTitleA2aDP5titleSSvrTW
+// CHECK:       function_ref @$s13read_accessor17OverridableGetterC5titleSSvr
+// CHECK-LABEL: // end sil function '$s13read_accessor17OverridableGetterCAA13ReadableTitleA2aDP5titleSSvrTW'
+
+protocol GettableTitle {
+  var title: String { get }
+}
+class OverridableReader : GettableTitle {
+  @_borrowed
+  var title: String = ""
+}
+//   The concrete getter is generated on-demand and does a class dispatch to the read accessor.
+// CHECK-LABEL: sil shared @$s13read_accessor17OverridableReaderC5titleSSvg
+// CHECK:       class_method %0 : $OverridableReader, #OverridableReader.title!read.1
+// CHECK-LABEL: // end sil function '$s13read_accessor17OverridableReaderC5titleSSvg'
+//   The getter witness thunk does a direct call to the concrete getter.
+// CHECK-LABEL: sil private [transparent] [thunk] @$s13read_accessor17OverridableReaderCAA13GettableTitleA2aDP5titleSSvgTW
+// CHECK:       function_ref @$s13read_accessor17OverridableReaderC5titleSSvg
+// CHECK-LABEL: // end sil function '$s13read_accessor17OverridableReaderCAA13GettableTitleA2aDP5titleSSvgTW'