Merge pull request #77546 from nate-chandler/general-coro/20241110/1

[CoroutineAccessors] Ban read+_read and modify+_modify.

Author: nate-chandler

include/swift/AST/Decl.h

@@ -9705,8 +9705,11 @@ const ParamDecl *getParameterAt(const ValueDecl *source, unsigned index);
 /// nullptr if the source does not have a parameter list.
 const ParamDecl *getParameterAt(const DeclContext *source, unsigned index);
 
-StringRef getAccessorNameForDiagnostic(AccessorDecl *accessor, bool article);
-StringRef getAccessorNameForDiagnostic(AccessorKind accessorKind, bool article);
+StringRef
+getAccessorNameForDiagnostic(AccessorDecl *accessor, bool article,
+                             std::optional<bool> underscored = std::nullopt);
+StringRef getAccessorNameForDiagnostic(AccessorKind accessorKind, bool article,
+                                       bool underscored);
 
 void simple_display(llvm::raw_ostream &out,
                     OptionSet<NominalTypeDecl::LookupDirectFlags> options);

lib/AST/ASTPrinter.cpp

@@ -2381,12 +2381,6 @@ void PrintAST::printSelfAccessKindModifiersIfNeeded(const FuncDecl *FD) {
   }
 }
 
-static bool
-shouldPrintUnderscoredCoroutineAccessors(const AbstractStorageDecl *ASD) {
-  // TODO: CoroutineAccessors: Print only when necessary.
-  return true;
-}
-
 void PrintAST::printAccessors(const AbstractStorageDecl *ASD) {
   if (isa<VarDecl>(ASD) && !Options.PrintPropertyAccessors)
     return;
@@ -2563,7 +2557,7 @@ void PrintAST::printAccessors(const AbstractStorageDecl *ASD) {
       break;
     case ReadImplKind::Read2:
       if (ASD->getAccessor(AccessorKind::Read) &&
-          shouldPrintUnderscoredCoroutineAccessors(ASD)) {
+          Options.SuppressCoroutineAccessors) {
         AddAccessorToPrint(AccessorKind::Read);
       }
       AddAccessorToPrint(AccessorKind::Read2);
@@ -2597,7 +2591,7 @@ void PrintAST::printAccessors(const AbstractStorageDecl *ASD) {
       break;
     case WriteImplKind::Modify2:
       if (ASD->getAccessor(AccessorKind::Modify) &&
-          shouldPrintUnderscoredCoroutineAccessors(ASD)) {
+          Options.SuppressCoroutineAccessors) {
         AddAccessorToPrint(AccessorKind::Modify);
       }
       AddAccessorToPrint(AccessorKind::Modify2);

lib/AST/Decl.cpp

@@ -6863,7 +6863,7 @@ bool ProtocolDecl::hasCircularInheritedProtocols() const {
 
 /// Returns a descriptive name for the given accessor/addressor kind.
 StringRef swift::getAccessorNameForDiagnostic(AccessorKind accessorKind,
-                                              bool article) {
+                                              bool article, bool underscored) {
   switch (accessorKind) {
   case AccessorKind::Get:
     return article ? "a getter" : "getter";
@@ -6876,9 +6876,17 @@ StringRef swift::getAccessorNameForDiagnostic(AccessorKind accessorKind,
   case AccessorKind::MutableAddress:
     return article ? "a mutable addressor" : "mutable addressor";
   case AccessorKind::Read:
+    if (underscored)
+      return article ? "a '_read' accessor" : "'_read' accessor";
+    // Fall through to the non-underscored spelling.
+    LLVM_FALLTHROUGH;
   case AccessorKind::Read2:
     return article ? "a 'read' accessor" : "'read' accessor";
   case AccessorKind::Modify:
+    if (underscored)
+      return article ? "a '_modify' accessor" : "'_modify' accessor";
+    // Fall through to the non-underscored spelling.
+    LLVM_FALLTHROUGH;
   case AccessorKind::Modify2:
     return article ? "a 'modify' accessor" : "'modify' accessor";
   case AccessorKind::WillSet:
@@ -6892,9 +6900,12 @@ StringRef swift::getAccessorNameForDiagnostic(AccessorKind accessorKind,
 }
 
 StringRef swift::getAccessorNameForDiagnostic(AccessorDecl *accessor,
-                                              bool article) {
-  return getAccessorNameForDiagnostic(accessor->getAccessorKind(),
-                                      article);
+                                              bool article,
+                                              std::optional<bool> underscored) {
+  return getAccessorNameForDiagnostic(
+      accessor->getAccessorKind(), article,
+      underscored.value_or(accessor->getASTContext().LangOpts.hasFeature(
+          Feature::CoroutineAccessors)));
 }
 
 bool AbstractStorageDecl::hasStorage() const {

lib/Parse/ParseDecl.cpp

@@ -7932,15 +7932,17 @@ bool Parser::parseAccessorAfterIntroducer(
   if (requiresFeatureCoroutineAccessors(Kind) &&
       !Context.LangOpts.hasFeature(Feature::CoroutineAccessors)) {
     diagnose(Tok, diag::accessor_requires_coroutine_accessors,
-             getAccessorNameForDiagnostic(Kind, /*article*/ false));
+             getAccessorNameForDiagnostic(Kind, /*article*/ false,
+                                          /*underscored*/ false));
   }
 
   // There should be no body in the limited syntax; diagnose unexpected
   // accessor implementations.
   if (parsingLimitedSyntax) {
     if (Tok.is(tok::l_brace))
       diagnose(Tok, diag::unexpected_getset_implementation_in_protocol,
-               getAccessorNameForDiagnostic(Kind, /*article*/ false));
+               getAccessorNameForDiagnostic(Kind, /*article*/ false,
+                                            /*underscored*/ false));
     return false;
   }
 
@@ -8352,16 +8354,45 @@ void Parser::ParsedAccessors::record(Parser &P, AbstractStorageDecl *storage,
   storage->setAccessors(LBLoc, Accessors, RBLoc);
 }
 
+static std::optional<AccessorKind>
+getCorrespondingUnderscoredAccessorKind(AccessorKind kind) {
+  switch (kind) {
+  case AccessorKind::Read2:
+    return {AccessorKind::Read};
+  case AccessorKind::Modify2:
+    return {AccessorKind::Modify};
+  case AccessorKind::Get:
+  case AccessorKind::DistributedGet:
+  case AccessorKind::Set:
+  case AccessorKind::Read:
+  case AccessorKind::Modify:
+  case AccessorKind::WillSet:
+  case AccessorKind::DidSet:
+  case AccessorKind::Address:
+  case AccessorKind::MutableAddress:
+  case AccessorKind::Init:
+    return std::nullopt;
+  }
+}
+
 static void diagnoseConflictingAccessors(Parser &P, AccessorDecl *first,
                                          AccessorDecl *&second) {
   if (!second) return;
-  P.diagnose(second->getLoc(), diag::conflicting_accessor,
-             isa<SubscriptDecl>(first->getStorage()),
-             getAccessorNameForDiagnostic(second, /*article*/ true),
-             getAccessorNameForDiagnostic(first, /*article*/ true));
-  P.diagnose(first->getLoc(), diag::previous_accessor,
-             getAccessorNameForDiagnostic(first, /*article*/ false),
-             /*already*/ false);
+  bool underscored =
+      (getCorrespondingUnderscoredAccessorKind(first->getAccessorKind()) ==
+       second->getAccessorKind()) ||
+      (getCorrespondingUnderscoredAccessorKind(second->getAccessorKind()) ==
+       first->getAccessorKind()) ||
+      first->getASTContext().LangOpts.hasFeature(Feature::CoroutineAccessors);
+  P.diagnose(
+      second->getLoc(), diag::conflicting_accessor,
+      isa<SubscriptDecl>(first->getStorage()),
+      getAccessorNameForDiagnostic(second, /*article*/ true, underscored),
+      getAccessorNameForDiagnostic(first, /*article*/ true, underscored));
+  P.diagnose(
+      first->getLoc(), diag::previous_accessor,
+      getAccessorNameForDiagnostic(first, /*article*/ false, underscored),
+      /*already*/ false);
   second->setInvalid();
 }
 
@@ -8404,12 +8435,13 @@ void Parser::ParsedAccessors::classify(Parser &P, AbstractStorageDecl *storage,
 
   // Okay, observers are out of the way.
 
-  // 'get', 'read', and a non-mutable addressor are all exclusive.
+  // 'get', '_read', 'read' and a non-mutable addressor are all exclusive.
   if (Get) {
     diagnoseConflictingAccessors(P, Get, Read);
     diagnoseConflictingAccessors(P, Get, Read2);
     diagnoseConflictingAccessors(P, Get, Address);
   } else if (Read) {
+    diagnoseConflictingAccessors(P, Read, Read2);
     diagnoseConflictingAccessors(P, Read, Address);
   } else if (Read2) {
     diagnoseConflictingAccessors(P, Read2, Address);
@@ -8438,11 +8470,13 @@ void Parser::ParsedAccessors::classify(Parser &P, AbstractStorageDecl *storage,
     }
   }
 
-  // A mutable addressor is exclusive with 'set' and 'modify', but
-  // 'set' and 'modify' can appear together.
+  // '_modify', 'modify' and 'unsafeMutableAddress' are all mutually exclusive.
+  // 'unsafeMutableAddress' and 'set' are mutually exclusive, but 'set' and
+  // 'modify' can appear together.
   if (Set) {
     diagnoseConflictingAccessors(P, Set, MutableAddress);
   } else if (Modify) {
+    diagnoseConflictingAccessors(P, Modify, Modify2);
     diagnoseConflictingAccessors(P, Modify, MutableAddress);
   }
 

lib/Sema/TypeCheckAttr.cpp

@@ -3644,7 +3644,7 @@ static FuncDecl *findSimilarAccessor(DeclNameRef replacedVarName,
         origStorage->getWriteImpl() == WriteImplKind::Stored)) {
     Diags.diagnose(attr->getLocation(),
                    diag::dynamic_replacement_accessor_not_explicit,
-                   getAccessorNameForDiagnostic(origAccessor->getAccessorKind(),
+                   getAccessorNameForDiagnostic(origAccessor,
                                                 /*article=*/false),
                    origStorage->getName());
     attr->setInvalid();

lib/Sema/TypeCheckStorage.cpp

@@ -717,13 +717,19 @@ static void diagnoseReadWriteMutatingnessMismatch(
   auto disagreesWithWriter = (isWriterMutating != isModifierMutating);
   auto disagreesWithBoth = disagreesWithReader && disagreesWithWriter;
 
+  bool hasCoroutineAccessorFeature =
+      storage->getASTContext().LangOpts.hasFeature(Feature::CoroutineAccessors);
+
   auto readerAccessor = directAccessorKindForReadImpl(storage->getReadImpl());
   StringRef readerAccessorName =
       readerAccessor.has_value()
-          ? getAccessorNameForDiagnostic(*readerAccessor, /*article=*/false)
+          ? getAccessorNameForDiagnostic(
+                *readerAccessor, /*article=*/false,
+                /*underscored=*/hasCoroutineAccessorFeature)
           : "the inherited accessor";
   StringRef writerAccessorName =
-      getAccessorNameForDiagnostic(writerAccesor, /*article=*/false);
+      getAccessorNameForDiagnostic(writerAccesor, /*article=*/false,
+                                   /*underscored=*/hasCoroutineAccessorFeature);
   unsigned diagnosticForm;
   if (isModifierMutating) {
     // modifier can't be mutating when both the setter is nonmutating and the
@@ -745,16 +751,20 @@ static void diagnoseReadWriteMutatingnessMismatch(
 
   modifyAccessor->diagnose(
       diag::readwriter_mutatingness_differs_from_reader_or_writer_mutatingness,
-      getAccessorNameForDiagnostic(readWriterAccessor, /*article=*/false),
+      getAccessorNameForDiagnostic(readWriterAccessor, /*article=*/false,
+                                   /*underscored=*/hasCoroutineAccessorFeature),
       isModifierMutating ? SelfAccessKind::Mutating
                          : SelfAccessKind::NonMutating,
       diagnosticForm, writerAccessorName, SelfAccessKind::NonMutating,
       readerAccessorName, SelfAccessKind::Mutating);
   auto *writer = storage->getParsedAccessor(writerAccesor);
   if (disagreesWithWriter && writer) {
-    writer->diagnose(
-        diag::previous_accessor,
-        getAccessorNameForDiagnostic(writerAccesor, /*article=*/false), 0);
+    writer->diagnose(diag::previous_accessor,
+                     getAccessorNameForDiagnostic(
+                         writerAccesor,
+                         /*article=*/false,
+                         /*underscored=*/hasCoroutineAccessorFeature),
+                     0);
   }
   AccessorDecl *reader = nullptr;
   if (disagreesWithReader && readerAccessor.has_value() &&
@@ -3960,6 +3970,8 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
 
   bool hasWillSet = storage->getParsedAccessor(AccessorKind::WillSet);
   bool hasDidSet = storage->getParsedAccessor(AccessorKind::DidSet);
+  bool hasCoroutineAccessorFeature =
+      storage->getASTContext().LangOpts.hasFeature(Feature::CoroutineAccessors);
   if ((hasWillSet || hasDidSet) && !isa<SubscriptDecl>(storage)) {
     // Observers conflict with non-observers.
     AccessorDecl *firstNonObserver = nullptr;
@@ -3972,19 +3984,21 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
 
     if (firstNonObserver) {
       if (auto willSet = storage->getParsedAccessor(AccessorKind::WillSet)) {
-        willSet->diagnose(
-            diag::observing_accessor_conflicts_with_accessor, 0,
-            getAccessorNameForDiagnostic(
-                firstNonObserver->getAccessorKind(), /*article=*/ true));
+        willSet->diagnose(diag::observing_accessor_conflicts_with_accessor, 0,
+                          getAccessorNameForDiagnostic(
+                              firstNonObserver->getAccessorKind(),
+                              /*article=*/true,
+                              /*underscored=*/hasCoroutineAccessorFeature));
         willSet->setInvalid();
         hasWillSet = false;
       }
 
       if (auto didSet = storage->getParsedAccessor(AccessorKind::DidSet)) {
-        didSet->diagnose(
-            diag::observing_accessor_conflicts_with_accessor, 1,
-            getAccessorNameForDiagnostic(
-              firstNonObserver->getAccessorKind(), /*article=*/ true));
+        didSet->diagnose(diag::observing_accessor_conflicts_with_accessor, 1,
+                         getAccessorNameForDiagnostic(
+                             firstNonObserver->getAccessorKind(),
+                             /*article=*/true,
+                             /*underscored=*/hasCoroutineAccessorFeature));
         didSet->setInvalid();
         hasDidSet = false;
       }

test/ModuleInterface/coroutine_accessors.swift

@@ -12,9 +12,7 @@ var _i: Int = 0
 
 // CHECK:      #if compiler(>=5.3) && $CoroutineAccessors
 // CHECK-NEXT: public var i: Swift.Int {
-// CHECK-NEXT:   _read
 // CHECK-NEXT:   read
-// CHECK-NEXT:   _modify
 // CHECK-NEXT:   modify
 // CHECK-NEXT: }
 // CHECK-NEXT: #else
@@ -24,15 +22,9 @@ var _i: Int = 0
 // CHECK-NEXT: }
 // CHECK-NEXT: #endif
 public var i: Int {
-  _read {
-    yield _i
-  }
   read {
     yield _i
   }
-  _modify {
-    yield &_i
-  }
   modify {
     yield &_i
   }