Merge pull request #67999 from DougGregor/observer-macros-on-computed-properties-5.9

[5.9] Improve checking of macro-generated accessors against documented names

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -7187,10 +7187,13 @@ ERROR(invalid_macro_role_for_macro_syntax,none,
       (unsigned))
 ERROR(macro_cannot_introduce_names,none,
       "'%0' macros are not allowed to introduce names", (StringRef))
-ERROR(macro_accessor_missing_from_expansion,none,
-      "expansion of macro %0 did not produce a %select{non-|}1observing "
-      "accessor",
-      (DeclName, bool))
+ERROR(macro_nonobserving_accessor_missing_from_expansion,none,
+      "expansion of macro %0 did not produce a non-observing accessor "
+      "(such as 'get') as expected",
+      (DeclName))
+ERROR(macro_nonobserver_unexpected_in_expansion,none,
+      "expansion of macro %0 produced an unexpected %1",
+      (DeclName, DescriptiveDeclKind))
 ERROR(macro_init_accessor_not_documented,none,
       "expansion of macro %0 produced an unexpected 'init' accessor",
       (DeclName))

lib/Sema/TypeCheckAttr.cpp

@@ -3665,7 +3665,13 @@ void AttributeChecker::visitCustomAttr(CustomAttr *attr) {
     // Try resolving an attached macro attribute.
     if (auto *macro = D->getResolvedMacro(attr)) {
       for (auto *roleAttr : macro->getAttrs().getAttributes<MacroRoleAttr>()) {
-        diagnoseInvalidAttachedMacro(roleAttr->getMacroRole(), D);
+        auto role = roleAttr->getMacroRole();
+        if (isInvalidAttachedMacro(role, D)) {
+          diagnoseAndRemoveAttr(attr,
+                                diag::macro_attached_to_invalid_decl,
+                                getMacroRoleString(role),
+                                D->getDescriptiveKind());
+        }
       }
 
       return;

lib/Sema/TypeCheckMacros.cpp

@@ -543,8 +543,8 @@ static Identifier makeIdentifier(ASTContext &ctx, std::nullptr_t) {
   return Identifier();
 }
 
-bool swift::diagnoseInvalidAttachedMacro(MacroRole role,
-                                         Decl *attachedTo) {
+bool swift::isInvalidAttachedMacro(MacroRole role,
+                                   Decl *attachedTo) {
   switch (role) {
   case MacroRole::Expression:
   case MacroRole::Declaration:
@@ -582,9 +582,6 @@ bool swift::diagnoseInvalidAttachedMacro(MacroRole role,
     break;
   }
 
-  attachedTo->diagnose(diag::macro_attached_to_invalid_decl,
-                       getMacroRoleString(role),
-                       attachedTo->getDescriptiveKind());
   return true;
 }
 
@@ -1362,7 +1359,9 @@ bool swift::accessorMacroOnlyIntroducesObservers(
   for (auto name : attr->getNames()) {
     if (name.getKind() == MacroIntroducedDeclNameKind::Named &&
         (name.getName().getBaseName().userFacingName() == "willSet" ||
-         name.getName().getBaseName().userFacingName() == "didSet")) {
+         name.getName().getBaseName().userFacingName() == "didSet" ||
+         name.getName().getBaseName().getKind() ==
+             DeclBaseName::Kind::Constructor)) {
       foundObserver = true;
     } else {
       // Introduces something other than an observer.
@@ -1413,24 +1412,28 @@ llvm::Optional<unsigned> swift::expandAccessors(AbstractStorageDecl *storage,
   // Trigger parsing of the sequence of accessor declarations. This has the
   // 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;
+  AccessorDecl *foundNonObservingAccessor = nullptr;
+  AccessorDecl *foundNonObservingAccessorInMacro = nullptr;
+  AccessorDecl *foundInitAccessor = nullptr;
   for (auto accessor : storage->getAllAccessors()) {
-    if (accessor->isInitAccessor())
-      foundInitAccessor = true;
+    if (accessor->isInitAccessor()) {
+      if (!foundInitAccessor)
+        foundInitAccessor = accessor;
+      continue;
+    }
 
     if (!accessor->isObservingAccessor()) {
-      foundNonObservingAccessor = true;
+      if (!foundNonObservingAccessor)
+        foundNonObservingAccessor = accessor;
 
-      if (accessor->isInMacroExpansionInContext())
-        foundNonObservingAccessorInMacro = true;
+      if (!foundNonObservingAccessorInMacro &&
+          accessor->isInMacroExpansionInContext())
+        foundNonObservingAccessorInMacro = accessor;
     }
   }
 
   auto roleAttr = macro->getMacroRoleAttr(MacroRole::Accessor);
-  bool expectedNonObservingAccessor =
-    !accessorMacroOnlyIntroducesObservers(macro, roleAttr);
+  bool expectObservers = accessorMacroOnlyIntroducesObservers(macro, roleAttr);
   if (foundNonObservingAccessorInMacro) {
     // If any non-observing accessor was added, mark the initializer as
     // subsumed unless it has init accessor, because the initializer in
@@ -1451,11 +1454,24 @@ llvm::Optional<unsigned> swift::expandAccessors(AbstractStorageDecl *storage,
       storage->removeAccessor(accessor);
   }
 
-  // Make sure we got non-observing accessors exactly where we expected to.
-  if (foundNonObservingAccessor != expectedNonObservingAccessor) {
+  // If the macro told us to expect only observing accessors, but the macro
+  // produced a non-observing accessor, it could have converted a stored
+  // property into a computed one without telling us pre-expansion. Produce
+  // an error to prevent this.
+  if (expectObservers && foundNonObservingAccessorInMacro) {
+    storage->diagnose(
+        diag::macro_nonobserver_unexpected_in_expansion, macro->getName(),
+        foundNonObservingAccessorInMacro->getDescriptiveKind());
+  }
+
+  // We expected to get a non-observing accessor, but there isn't one (from
+  // the macro or elsewhere), meaning that we counted on this macro to make
+  // this stored property into a a computed property... but it didn't.
+  // Produce an error.
+  if (!expectObservers && !foundNonObservingAccessor) {
     storage->diagnose(
-        diag::macro_accessor_missing_from_expansion, macro->getName(),
-        !expectedNonObservingAccessor);
+        diag::macro_nonobserving_accessor_missing_from_expansion,
+        macro->getName());
   }
 
   // 'init' accessors must be documented in the macro role attribute.

lib/Sema/TypeCheckMacros.h

@@ -89,10 +89,10 @@ bool accessorMacroOnlyIntroducesObservers(
 bool accessorMacroIntroducesInitAccessor(
     MacroDecl *macro, const MacroRoleAttr *attr);
 
-/// Diagnose an error if the given macro role does not apply
+/// Return true if the given macro role does not apply
 /// to the declaration kind of \c attachedTo.
-bool diagnoseInvalidAttachedMacro(MacroRole role,
-                                  Decl *attachedTo);
+bool isInvalidAttachedMacro(MacroRole role,
+                            Decl *attachedTo);
 
 } // end namespace swift
 

test/Macros/Inputs/syntax_macro_definitions.swift

@@ -516,6 +516,26 @@ extension PropertyWrapperSkipsComputedMacro: AccessorMacro, Macro {
   }
 }
 
+public struct WillSetMacro: AccessorMacro {
+  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 else {
+      return []
+    }
+
+    return [
+      """
+        willSet { }
+      """
+    ]
+  }
+}
+
 public struct WrapAllProperties: MemberAttributeMacro {
   public static func expansion(
     of node: AttributeSyntax,

test/Macros/accessor_macros.swift

@@ -3,10 +3,8 @@
 // RUN: %empty-directory(%t)
 // RUN: %host-build-swift -swift-version 5 -emit-library -o %t/%target-library-name(MacroDefinition) -module-name=MacroDefinition %S/Inputs/syntax_macro_definitions.swift -g -no-toolchain-stdlib-rpath
 
-// First check for no errors.
-// RUN: %target-typecheck-verify-swift -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition)
-
 // Check for expected errors.
+// RUN: %target-typecheck-verify-swift -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition) -DTEST_DIAGNOSTICS -verify-ignore-unknown
 // RUN: not %target-swift-frontend -typecheck -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition) -DTEST_DIAGNOSTICS %s > %t/diags.txt 2>&1
 // RUN: %FileCheck -check-prefix=CHECK-DIAGS %s < %t/diags.txt
 
@@ -98,21 +96,62 @@ ms.favoriteColor = "Yellow"
 struct MyBrokenStruct {
   var _birthDate: MyWrapperThingy<Date?> = .init(storage: nil)
 
+  // expected-note@+2 2{{in expansion of macro 'myPropertyWrapper'}}
   @myPropertyWrapper
   var birthDate: Date? {
     // CHECK-DIAGS: variable already has a getter
     // CHECK-DIAGS: in expansion of macro
     // CHECK-DIAGS: previous definition of getter here
     get { fatalError("Boom") }
+    // expected-note @-1{{previous definition of getter here}}
 
     // CHECK-DIAGS: variable already has a setter
     // CHECK-DIAGS: in expansion of macro
     // CHECK-DIAGS: previous definition of setter here
     set { fatalError("Boom") }
+    // expected-note @-1{{previous definition of setter here}}
   }
 }
 
+// expected-error@+1{{'accessor' macro cannot be attached to struct}}
 @myPropertyWrapper
 struct CannotHaveAccessors {}
 // CHECK-DIAGS: 'accessor' macro cannot be attached to struct
 #endif
+
+
+
+@attached(accessor, names: named(willSet))
+macro SkipsComputed() =
+    #externalMacro(module: "MacroDefinition", type: "PropertyWrapperSkipsComputedMacro")
+
+struct HasComputed {
+  @SkipsComputed
+  var value: Int { 17 }
+}
+
+@attached(accessor, names: named(willSet))
+macro AddWillSet() =
+    #externalMacro(module: "MacroDefinition", type: "WillSetMacro")
+
+@attached(accessor)
+macro AddWillSetSneakily() =
+    #externalMacro(module: "MacroDefinition", type: "WillSetMacro")
+
+@attached(accessor, names: named(willSet))
+macro MakeComputedSneakily() =
+    #externalMacro(module: "MacroDefinition", type: "PropertyWrapperMacro")
+
+struct HasStoredTests {
+  @AddWillSet var x: Int = 0
+
+#if TEST_DIAGNOSTICS
+  @AddWillSetSneakily var y: Int = 0
+  // expected-error@-1{{expansion of macro 'AddWillSetSneakily()' did not produce a non-observing accessor (such as 'get') as expected}}
+
+  @MakeComputedSneakily var z: Int = 0
+  // expected-error@-1{{expansion of macro 'MakeComputedSneakily()' produced an unexpected getter}}
+  // expected-note@-2 2{{in expansion of macro}}
+  // expected-note@-3 2{{'z' declared here}}
+#endif
+}

test/Macros/macro_expand_attributes.swift

@@ -144,9 +144,9 @@ expansionOrder.originalMember = 28
 #if TEST_DIAGNOSTICS
 @wrapAllProperties
 typealias A = Int
-// expected-error@-1{{'memberAttribute' macro cannot be attached to type alias}}
+// expected-error@-2{{'memberAttribute' macro cannot be attached to type alias}}
 
 @wrapAllProperties
 func noMembers() {}
-// expected-error@-1{{'memberAttribute' macro cannot be attached to global function}}
+// expected-error@-2{{'memberAttribute' macro cannot be attached to global function}}
 #endif

test/Macros/macro_expand_extensions.swift

@@ -113,11 +113,11 @@ func testLocal() {
 
 @DelegatedConformance
 typealias A = Int
-// expected-error@-1 {{'extension' macro cannot be attached to type alias}}
+// expected-error@-2 {{'extension' macro cannot be attached to type alias}}
 
 @DelegatedConformance
 extension Int {}
-// expected-error@-1 {{'extension' macro cannot be attached to extension}}
+// expected-error@-2 {{'extension' macro cannot be attached to extension}}
 
 @attached(extension, conformances: P)
 macro UndocumentedNamesInExtension() = #externalMacro(module: "MacroDefinition", type: "DelegatedConformanceViaExtensionMacro")

test/Macros/macro_expand_synthesized_members.swift

@@ -22,7 +22,7 @@ macro addMembersQuotedInit() = #externalMacro(module: "MacroDefinition", type: "
 #if TEST_DIAGNOSTICS
 @addMembers
 import Swift
-// expected-error@-1 {{'member' macro cannot be attached to import}}
+// expected-error@-2 {{'member' macro cannot be attached to import}}
 #endif
 
 @addMembers

test/Serialization/macros.swift

@@ -21,7 +21,7 @@ func test(a: Int, b: Int) {
 
 struct TestStruct {
   @myWrapper var x: Int
-  // expected-error@-1{{expansion of macro 'myWrapper()' did not produce a non-observing accessor}}
+  // expected-error@-1{{expansion of macro 'myWrapper()' did not produce a non-observing accessor (such as 'get') as expected}}
 }
 
 @ArbitraryMembers

test/stdlib/Observation/Observable.swift

@@ -155,6 +155,12 @@ class IsolatedInstance {
   var test = "hello"
 }
 
+@Observable
+class IgnoredComputed {
+  @ObservationIgnored
+  var message: String { "hello" }
+}
+
 @Observable
 class ClassHasExistingConformance: Observable { }