Merge pull request #67148 from DougGregor/accessor-macros-finishing-touches

DougGregor · web-flow · commit f5427eaea2fe · 2023-07-05T22:56:46.000-07:00
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.
   ///
@@ -8915,6 +8922,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
@@ -6421,6 +6421,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,
@@ -6488,7 +6520,8 @@ void AbstractStorageDecl::setAccessors(SourceLoc lbraceLoc,
   auto record = Accessors.getPointer();
   if (record) {
     for (auto accessor : accessors) {
-      (void) record->addOpaqueAccessor(accessor);
+      if (!record->getAccessor(accessor->getAccessorKind()))
+        (void) record->addOpaqueAccessor(accessor);
     }
   } else {
     record = AccessorRecord::create(getASTContext(),
@@ -6582,6 +6615,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
@@ -7010,38 +7010,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,
@@ -7096,15 +7064,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;
@@ -7521,8 +7480,14 @@ void Parser::parseTopLevelAccessors(
 
   bool hadLBrace = consumeIf(tok::l_brace);
 
-  ParserStatus status;
+  // Prepopulate the field for any accessors that were already parsed parsed accessors
   ParsedAccessors accessors;
+#define ACCESSOR(ID)                                            \
+    if (auto accessor = storage->getAccessor(AccessorKind::ID)) \
+      accessors.ID = accessor;
+#include "swift/AST/AccessorKinds.def"
+
+  ParserStatus status;
   bool hasEffectfulGet = false;
   bool parsingLimitedSyntax = false;
   while (!Tok.isAny(tok::r_brace, tok::eof)) {
@@ -7794,16 +7759,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
@@ -1345,23 +1345,24 @@ llvm::Optional<unsigned> swift::expandAccessors(AbstractStorageDecl *storage,
   // 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 decl : macroSourceFile->getTopLevelItems()) {
-    auto accessor = dyn_cast_or_null<AccessorDecl>(decl.dyn_cast<Decl *>());
-    if (!accessor)
-      continue;
-
+  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.
@@ -1372,6 +1373,13 @@ llvm::Optional<unsigned> swift::expandAccessors(AbstractStorageDecl *storage,
         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
@@ -3626,6 +3626,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/Inputs/syntax_macro_definitions.swift b/test/Macros/Inputs/syntax_macro_definitions.swift
@@ -422,8 +422,7 @@ extension PropertyWrapperMacro: AccessorMacro, Macro {
   ) throws -> [AccessorDeclSyntax] {
     guard let varDecl = declaration.as(VariableDeclSyntax.self),
       let binding = varDecl.bindings.first,
-      let identifier = binding.pattern.as(IdentifierPatternSyntax.self)?.identifier,
-      binding.accessor == nil
+      let identifier = binding.pattern.as(IdentifierPatternSyntax.self)?.identifier
     else {
       return []
     }
@@ -468,6 +467,55 @@ extension PropertyWrapperMacro: PeerMacro {
   }
 }
 
+extension PatternBindingSyntax.Accessor {
+  var hasGetter: Bool {
+    switch self {
+    case .accessors(let accessors):
+      for accessor in accessors.accessors {
+        if accessor.accessorKind.text == "get" {
+          return true
+        }
+      }
+
+      return false
+    case .getter:
+      return true
+    }
+  }
+}
+
+public struct PropertyWrapperSkipsComputedMacro {}
+
+extension PropertyWrapperSkipsComputedMacro: AccessorMacro, Macro {
+  public static func expansion(
+    of node: AttributeSyntax,
+    providingAccessorsOf declaration: some DeclSyntaxProtocol,
+    in context: some MacroExpansionContext
+  ) throws -> [AccessorDeclSyntax] {
+    guard let varDecl = declaration.as(VariableDeclSyntax.self),
+      let binding = varDecl.bindings.first,
+      let identifier = binding.pattern.as(IdentifierPatternSyntax.self)?.identifier, !(binding.accessor?.hasGetter ?? false)
+    else {
+      return []
+    }
+
+    return [
+      """
+
+        get {
+          _\(identifier).wrappedValue
+        }
+      """,
+      """
+
+        set {
+          _\(identifier).wrappedValue = newValue
+        }
+      """,
+    ]
+  }
+}
+
 public struct WrapAllProperties: MemberAttributeMacro {
   public static func expansion(
     of node: AttributeSyntax,
diff --git a/test/Macros/accessor_macros.swift b/test/Macros/accessor_macros.swift
