[Sema] Fix inconsistent mutatingness diagnostic.

Refer to the correct problematic accessor or accessors.

Author: nate-chandler

include/swift/AST/DiagnosticsSema.def

@@ -1710,9 +1710,10 @@ ERROR(functions_mutating_and_not,none,
 ERROR(static_functions_not_mutating,none,
       "static functions must not be declared mutating", ())
 
-ERROR(modify_mutatingness_differs_from_setter,none,
-      "'modify' accessor cannot be %0 when the setter is %1",
-      (SelfAccessKind, SelfAccessKind))
+ERROR(readwriter_mutatingness_differs_from_reader_or_writer_mutatingness,none,
+      "%0 cannot be %1 when "
+      "%select{both the %3 is %4 and the %5 is not %6|either the %3 is not %4 or the %5 is %6|the %3 is not %4|the %5 is %6}2",
+      (StringRef, SelfAccessKind, unsigned, StringRef, SelfAccessKind, StringRef, SelfAccessKind))
 
 ERROR(transparent_in_protocols_not_supported,none,
       "'@_transparent' attribute is not supported on declarations within protocols", ())

lib/Sema/TypeCheckStorage.cpp

@@ -697,28 +697,70 @@ IsGetterMutatingRequest::evaluate(Evaluator &evaluator,
 /// the implied value.
 static void diagnoseReadWriteMutatingnessMismatch(
     AbstractStorageDecl *storage, bool isWriterMutating, WriteImplKind write,
-    AccessorKind writeAccesor, ReadWriteImplKind readWrite,
-    AccessorKind readWriteAccessor) {
+    AccessorKind writerAccesor, ReadWriteImplKind readWrite,
+    AccessorKind readWriterAccessor) {
   if (storage->getImplInfo().getReadWriteImpl() != readWrite)
     return;
-  auto modifyAccessor = storage->getParsedAccessor(readWriteAccessor);
+  auto modifyAccessor = storage->getParsedAccessor(readWriterAccessor);
   if (!modifyAccessor)
     return;
 
   auto isModifierMutating = modifyAccessor->isMutating();
-  auto isReaderOrWriterMutating =
-      isWriterMutating || storage->isGetterMutating();
+  auto isReaderMutating = storage->isGetterMutating();
+  auto isReaderOrWriterMutating = isWriterMutating || isReaderMutating;
   if (isReaderOrWriterMutating == isModifierMutating)
     return;
 
-  modifyAccessor->diagnose(diag::modify_mutatingness_differs_from_setter,
-                           isModifierMutating ? SelfAccessKind::Mutating
-                                              : SelfAccessKind::NonMutating,
-                           isModifierMutating ? SelfAccessKind::NonMutating
-                                              : SelfAccessKind::Mutating);
-  auto *setter = storage->getParsedAccessor(writeAccesor);
-  if (setter)
-    setter->diagnose(diag::previous_accessor, "setter", 0);
+  auto disagreesWithReader = (isReaderMutating != isModifierMutating);
+  auto disagreesWithWriter = (isWriterMutating != isModifierMutating);
+  auto disagreesWithBoth = disagreesWithReader && disagreesWithWriter;
+
+  auto readerAccessor = directAccessorKindForReadImpl(storage->getReadImpl());
+  StringRef readerAccessorName =
+      readerAccessor.has_value()
+          ? getAccessorNameForDiagnostic(*readerAccessor, /*article=*/false)
+          : "the inherited accessor";
+  StringRef writerAccessorName =
+      getAccessorNameForDiagnostic(writerAccesor, /*article=*/false);
+  unsigned diagnosticForm;
+  if (isModifierMutating) {
+    // modifier can't be mutating when both the setter is nonmutating and the
+    // getter isn't mutating
+    assert(disagreesWithBoth);
+    diagnosticForm = 0;
+  } else if (disagreesWithBoth) {
+    // modify can't be nonmutating when either the setter isn't nonmutating or
+    // the getter is mutating
+    diagnosticForm = 1;
+  } else if (disagreesWithWriter) {
+    // modify can't be nonmutating when the setter isn't nonmutating
+    diagnosticForm = 2;
+  } else {
+    // modify can't be nonmutating when the getter is mutating
+    assert(disagreesWithReader);
+    diagnosticForm = 3;
+  }
+
+  modifyAccessor->diagnose(
+      diag::readwriter_mutatingness_differs_from_reader_or_writer_mutatingness,
+      getAccessorNameForDiagnostic(readWriterAccessor, /*article=*/false),
+      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);
+  }
+  AccessorDecl *reader = nullptr;
+  if (disagreesWithReader && readerAccessor.has_value() &&
+      (reader = storage->getParsedAccessor(*readerAccessor))) {
+    StringRef readerAccessorName =
+        getAccessorNameForDiagnostic(reader, /*article=*/false);
+    reader->diagnose(diag::previous_accessor, readerAccessorName, 0);
+  }
   modifyAccessor->setInvalid();
 }
 

test/Sema/coroutine_accessors.swift

@@ -33,17 +33,16 @@ var ingnsn_m: Int {
 var ignsn_m: Int {
   mutating get { 0 }
   nonmutating set {}
-                        // FIXME: Bad diagnostic!  The writer is non-mutating--the reader is mutating.
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
-                        // expected-note@-3{{setter defined here}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the getter is 'mutating'}}
+                        // expected-note@-3{{getter defined here}}
     var fake: Int
     yield &fake
   }
 }
 var ingsn_m: Int {
   get { 0 }
   set {}
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is not 'nonmutating'}}
                         // expected-note@-2{{setter defined here}}
     var fake: Int
     yield &fake
@@ -52,18 +51,19 @@ var ingsn_m: Int {
 var igsn_m: Int {
   mutating get { 0 }
   set {}
-                        // TODO: Incomplete diagnostic!  Both the writer AND the reader are mutating.
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
-                        // expected-note@-3{{setter defined here}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when either the setter is not 'nonmutating' or the getter is 'mutating'}}
+                        // expected-note@-2{{setter defined here}}
+                        // expected-note@-4{{getter defined here}}
     var fake: Int
     yield &fake
   }
 }
 var ingns_m: Int {
   get { 0 }
   nonmutating set {}
-  _modify { // expected-error{{'modify' accessor cannot be 'mutating' when the setter is 'nonmutating'}}
+  _modify { // expected-error{{'modify' accessor cannot be 'mutating' when both the setter is 'nonmutating' and the getter is not 'mutating'}}
             // expected-note@-2{{setter defined here}}
+            // expected-note@-4{{getter defined here}}
     yield &i
   }
 }
@@ -115,17 +115,16 @@ var in_rnsn_m: Int {
 var i_rnsn_m: Int {
   mutating _read { yield i }
   nonmutating set {}
-                        // FIXME: Bad diagnostic!  The writer is non-mutating--the reader is mutating.
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
-                        // expected-note@-3{{setter defined here}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the 'read' accessor is 'mutating'}}
+                        // expected-note@-3{{'read' accessor defined here}}
     var fake: Int
     yield &fake
   }
 }
 var in_rsn_m: Int {
   _read { yield i }
   set {}
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is not 'nonmutating'}}
                         // expected-note@-2{{setter defined here}}
     var fake: Int
     yield &fake
@@ -134,18 +133,19 @@ var in_rsn_m: Int {
 var i_rsn_m: Int {
   mutating _read { yield i }
   set {}
-                        // TODO: Incomplete diagnostic!  Both the writer AND the reader are mutating.
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
-                        // expected-note@-3{{setter defined here}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when either the setter is not 'nonmutating' or the 'read' accessor is 'mutating'}}
+                        // expected-note@-2{{setter defined here}}
+                        // expected-note@-4{{'read' accessor defined here}}
     var fake: Int
     yield &fake
   }
 }
 var in_rns_m: Int {
   _read { yield i }
   nonmutating set {}
-  _modify { // expected-error{{'modify' accessor cannot be 'mutating' when the setter is 'nonmutating'}}
+  _modify { // expected-error{{'modify' accessor cannot be 'mutating' when both the setter is 'nonmutating' and the 'read' accessor is not 'mutating'}}
             // expected-note@-2{{setter defined here}}
+            // expected-note@-4{{'read' accessor defined here}}
     yield &i
   }
 }
@@ -197,17 +197,16 @@ var inuansn_m: Int {
 var iuansn_m: Int {
   mutating unsafeAddress { UnsafePointer(bitPattern: 0x0)! }
   nonmutating set {}
-                        // FIXME: Bad diagnostic!  The writer is non-mutating--the reader is mutating.
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
-                        // expected-note@-3{{setter defined here}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the addressor is 'mutating'}}
+                        // expected-note@-3{{addressor defined here}}
     var fake: Int
     yield &fake
   }
 }
 var inuasn_m: Int {
   unsafeAddress { UnsafePointer(bitPattern: 0x0)! }
   set {}
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is not 'nonmutating'}}
                         // expected-note@-2{{setter defined here}}
     var fake: Int
     yield &fake
@@ -216,18 +215,19 @@ var inuasn_m: Int {
 var iuasn_m: Int {
   mutating unsafeAddress { UnsafePointer(bitPattern: 0x0)! }
   set {}
-                        // TODO: Incomplete diagnostic!  Both the writer AND the reader are mutating.
-  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
-                        // expected-note@-3{{setter defined here}}
+  nonmutating _modify { // expected-error{{'modify' accessor cannot be 'nonmutating' when either the setter is not 'nonmutating' or the addressor is 'mutating'}}
+                        // expected-note@-2{{setter defined here}}
+                        // expected-note@-4{{addressor defined here}}
     var fake: Int
     yield &fake
   }
 }
 var inuans_m: Int {
   unsafeAddress { UnsafePointer(bitPattern: 0x0)! }
   nonmutating set {}
-  _modify { // expected-error{{'modify' accessor cannot be 'mutating' when the setter is 'nonmutating'}}
+  _modify { // expected-error{{'modify' accessor cannot be 'mutating' when both the setter is 'nonmutating' and the addressor is not 'mutating'}}
             // expected-note@-2{{setter defined here}}
+            // expected-note@-4{{addressor defined here}}
     yield &i
   }
 }

test/Sema/generalized_accessors.swift

@@ -94,13 +94,13 @@ struct Modify {
 
   var getAndNonMutatingModifyAndSet: Int {
     get {}
-    nonmutating _modify {} // expected-error {{'modify' accessor cannot be 'nonmutating' when the setter is 'mutating'}}
+    nonmutating _modify {} // expected-error {{'modify' accessor cannot be 'nonmutating' when the setter is not 'nonmutating'}}
     set {} // expected-note {{setter defined here}}
   }
 
   var getAndModifyAndNonMutatingSet: Int {
-    get {}
-    _modify {}// expected-error {{'modify' accessor cannot be 'mutating' when the setter is 'nonmutating'}}
+    get {} // expected-note{{getter defined here}}
+    _modify {}// expected-error {{'modify' accessor cannot be 'mutating' when both the setter is 'nonmutating' and the getter is not 'mutating'}}
     nonmutating set {} // expected-note {{setter defined here}}
   }
 }