[Macros] When a macro defines a getter or setter, remove didSet/willSet

DougGregor · DougGregor · commit 8dc52ec016d5 · 2023-07-05T23:42:01.000-07:00
As with the initial value of a property that is converted from a stored
property to a computed property by an accessor macro, remove
didSet/willSet. It is the macro's responsibility to incorporate their
code or diagnose them.

Fixes rdar://111101833.
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -5238,6 +5238,8 @@ class AbstractStorageDecl : public ValueDecl {
 
     void addOpaqueAccessor(AccessorDecl *accessor);
 
+    void removeAccessor(AccessorDecl *accessor);
+
   private:
     MutableArrayRef<AccessorDecl *> getAccessorsBuffer() {
       return { getTrailingObjects<AccessorDecl*>(), NumAccessors };
@@ -5401,6 +5403,11 @@ class AbstractStorageDecl : public ValueDecl {
     return {};
   }
 
+  void removeAccessor(AccessorDecl *accessor) {
+    if (auto *info = Accessors.getPointer())
+      return info->removeAccessor(accessor);
+  }
+
   /// This is the primary mechanism by which we can easily determine whether
   /// this storage decl has any effects.
   ///
@@ -8912,6 +8919,9 @@ 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);
+
 void simple_display(llvm::raw_ostream &out,
                     OptionSet<NominalTypeDecl::LookupDirectFlags> options);
 
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -6411,6 +6411,38 @@ bool ProtocolDecl::hasCircularInheritedProtocols() const {
       ctx.evaluator, HasCircularInheritedProtocolsRequest{mutableThis}, true);
 }
 
+/// Returns a descriptive name for the given accessor/addressor kind.
+StringRef swift::getAccessorNameForDiagnostic(AccessorKind accessorKind,
+                                              bool article) {
+  switch (accessorKind) {
+  case AccessorKind::Get:
+    return article ? "a getter" : "getter";
+  case AccessorKind::Set:
+    return article ? "a setter" : "setter";
+  case AccessorKind::Address:
+    return article ? "an addressor" : "addressor";
+  case AccessorKind::MutableAddress:
+    return article ? "a mutable addressor" : "mutable addressor";
+  case AccessorKind::Read:
+    return article ? "a 'read' accessor" : "'read' accessor";
+  case AccessorKind::Modify:
+    return article ? "a 'modify' accessor" : "'modify' accessor";
+  case AccessorKind::WillSet:
+    return "'willSet'";
+  case AccessorKind::DidSet:
+    return "'didSet'";
+  case AccessorKind::Init:
+    return article ? "an init accessor" : "init accessor";
+  }
+  llvm_unreachable("bad accessor kind");
+}
+
+StringRef swift::getAccessorNameForDiagnostic(AccessorDecl *accessor,
+                                              bool article) {
+  return getAccessorNameForDiagnostic(accessor->getAccessorKind(),
+                                      article);
+}
+
 bool AbstractStorageDecl::hasStorage() const {
   ASTContext &ctx = getASTContext();
   return evaluateOrDefault(ctx.evaluator,
@@ -6573,6 +6605,27 @@ void AbstractStorageDecl::AccessorRecord::addOpaqueAccessor(AccessorDecl *decl){
   (void) isUnique;
 }
 
+void AbstractStorageDecl::AccessorRecord::removeAccessor(
+    AccessorDecl *accessor
+) {
+  // Remove this accessor from the list of accessors.
+  assert(getAccessor(accessor->getAccessorKind()) == accessor);
+  std::remove(getAccessorsBuffer().begin(), getAccessorsBuffer().end(),
+              accessor);
+
+  // Clear out the accessor kind -> index mapping.
+  std::memset(AccessorIndices, 0, sizeof(AccessorIndices));
+
+  // Re-add all of the remaining accessors to build up the index mapping.
+  unsigned numAccessorsLeft = NumAccessors - 1;
+  NumAccessors = numAccessorsLeft;
+  auto buffer = getAccessorsBuffer();
+  NumAccessors = 0;
+  for (auto index : range(0, numAccessorsLeft)) {
+    addOpaqueAccessor(buffer[index]);
+  }
+}
+
 /// Register that we have an accessor of the given kind.
 bool AbstractStorageDecl::AccessorRecord::registerAccessor(AccessorDecl *decl,
                                                            AccessorIndex index){
diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
@@ -6937,38 +6937,6 @@ void Parser::skipAnyAttribute() {
   (void)canParseCustomAttribute();
 }
 
-/// Returns a descriptive name for the given accessor/addressor kind.
-static StringRef getAccessorNameForDiagnostic(AccessorKind accessorKind,
-                                              bool article) {
-  switch (accessorKind) {
-  case AccessorKind::Get:
-    return article ? "a getter" : "getter";
-  case AccessorKind::Set:
-    return article ? "a setter" : "setter";
-  case AccessorKind::Address:
-    return article ? "an addressor" : "addressor";
-  case AccessorKind::MutableAddress:
-    return article ? "a mutable addressor" : "mutable addressor";
-  case AccessorKind::Read:
-    return article ? "a 'read' accessor" : "'read' accessor";
-  case AccessorKind::Modify:
-    return article ? "a 'modify' accessor" : "'modify' accessor";
-  case AccessorKind::WillSet:
-    return "'willSet'";
-  case AccessorKind::DidSet:
-    return "'didSet'";
-  case AccessorKind::Init:
-    return article ? "an init accessor" : "init accessor";
-  }
-  llvm_unreachable("bad accessor kind");  
-}
-
-static StringRef getAccessorNameForDiagnostic(AccessorDecl *accessor,
-                                              bool article) {
-  return getAccessorNameForDiagnostic(accessor->getAccessorKind(),
-                                      article);
-}
-
 static void diagnoseRedundantAccessors(Parser &P, SourceLoc loc,
                                        AccessorKind accessorKind,
                                        bool isSubscript,
@@ -7023,15 +6991,6 @@ struct Parser::ParsedAccessors {
         func(accessor);
   }
 
-  /// Find the first accessor that's not an observing accessor.
-  AccessorDecl *findFirstNonObserver() {
-    for (auto accessor : Accessors) {
-      if (!accessor->isObservingAccessor())
-        return accessor;
-    }
-    return nullptr;
-  }
-
   /// Find the first accessor that can be used to perform mutation.
   AccessorDecl *findFirstMutator() const {
     if (Set) return Set;
@@ -7727,16 +7686,10 @@ void Parser::ParsedAccessors::classify(Parser &P, AbstractStorageDecl *storage,
   // The observing accessors have very specific restrictions.
   // Prefer to ignore them.
   if (WillSet || DidSet) {
-    // For now, we don't support the observing accessors on subscripts.
+    // We don't support the observing accessors on subscripts.
     if (isa<SubscriptDecl>(storage)) {
       diagnoseAndIgnoreObservers(P, *this,
                                  diag::observing_accessor_in_subscript);
-
-    // The observing accessors cannot be combined with other accessors.
-    } else if (auto nonObserver = findFirstNonObserver()) {
-      diagnoseAndIgnoreObservers(P, *this,
-                   diag::observing_accessor_conflicts_with_accessor,
-                   getAccessorNameForDiagnostic(nonObserver, /*article*/ true));
     }
   }
 
diff --git a/lib/Sema/TypeCheckMacros.cpp b/lib/Sema/TypeCheckMacros.cpp
@@ -1346,19 +1346,24 @@ Optional<unsigned> swift::expandAccessors(
   // side effect of registering those accessor declarations with the storage
   // declaration, so there is nothing further to do.
   bool foundNonObservingAccessor = false;
+  bool foundNonObservingAccessorInMacro = false;
   bool foundInitAccessor = false;
   for (auto accessor : storage->getAllAccessors()) {
     if (accessor->isInitAccessor())
       foundInitAccessor = true;
 
-    if (!accessor->isObservingAccessor())
+    if (!accessor->isObservingAccessor()) {
       foundNonObservingAccessor = true;
+
+      if (accessor->isInMacroExpansionInContext())
+        foundNonObservingAccessorInMacro = true;
+    }
   }
 
   auto roleAttr = macro->getMacroRoleAttr(MacroRole::Accessor);
   bool expectedNonObservingAccessor =
     !accessorMacroOnlyIntroducesObservers(macro, roleAttr);
-  if (foundNonObservingAccessor) {
+  if (foundNonObservingAccessorInMacro) {
     // If any non-observing accessor was added, mark the initializer as
     // subsumed unless it has init accessor, because the initializer in
     // such cases could be used for memberwise initialization.
@@ -1369,6 +1374,13 @@ Optional<unsigned> swift::expandAccessors(
         binding->setInitializerSubsumed(index);
       }
     }
+
+    // Also remove didSet and willSet, because they are subsumed by a
+    // macro expansion that turns a stored property into a computed one.
+    if (auto accessor = storage->getParsedAccessor(AccessorKind::WillSet))
+      storage->removeAccessor(accessor);
+    if (auto accessor = storage->getParsedAccessor(AccessorKind::DidSet))
+      storage->removeAccessor(accessor);
   }
 
   // Make sure we got non-observing accessors exactly where we expected to.
diff --git a/lib/Sema/TypeCheckStorage.cpp b/lib/Sema/TypeCheckStorage.cpp
@@ -3636,6 +3636,37 @@ StorageImplInfoRequest::evaluate(Evaluator &evaluator,
 
   bool hasWillSet = storage->getParsedAccessor(AccessorKind::WillSet);
   bool hasDidSet = storage->getParsedAccessor(AccessorKind::DidSet);
+  if ((hasWillSet || hasDidSet) && !isa<SubscriptDecl>(storage)) {
+    // Observers conflict with non-observers.
+    AccessorDecl *firstNonObserver = nullptr;
+    for (auto accessor : storage->getAllAccessors()) {
+      if (!accessor->isImplicit() && !accessor->isObservingAccessor()) {
+        firstNonObserver = accessor;
+        break;
+      }
+    }
+
+    if (firstNonObserver) {
+      if (auto willSet = storage->getParsedAccessor(AccessorKind::WillSet)) {
+        willSet->diagnose(
+            diag::observing_accessor_conflicts_with_accessor, 0,
+            getAccessorNameForDiagnostic(
+                firstNonObserver->getAccessorKind(), /*article=*/ true));
+        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->setInvalid();
+        hasDidSet = false;
+      }
+    }
+  }
+
   bool hasSetter = storage->getParsedAccessor(AccessorKind::Set);
   bool hasModify = storage->getParsedAccessor(AccessorKind::Modify);
   bool hasMutableAddress = storage->getParsedAccessor(AccessorKind::MutableAddress);
diff --git a/test/Macros/accessor_macros.swift b/test/Macros/accessor_macros.swift
@@ -48,6 +48,7 @@ struct MyWrapperThingy<T> {
 struct MyStruct {
   var _name: MyWrapperThingy<String> = .init(storage: "Hello")
   var _birthDate: MyWrapperThingy<Date?> = .init(storage: nil)
+  var _favoriteColor: MyWrapperThingy<String> = .init(storage: "Blue")
 
   @myPropertyWrapper
   var name: String
@@ -73,6 +74,11 @@ struct MyStruct {
   var age: Int? {
     get { nil }
   }
+
+  @myPropertyWrapper
+  var favoriteColor: String {
+    didSet { fatalError("Boom") }
+  }
 }
 
 // Test that the fake-property-wrapper-introduced accessors execute properly at
@@ -85,6 +91,8 @@ _ = ms.name
 // CHECK-NEXT: Setting value World
 ms.name = "World"
 
+// CHECK-NEXT: Setting value Yellow
+ms.favoriteColor = "Yellow"
 
 #if TEST_DIAGNOSTICS
 struct MyBrokenStruct {
