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

Improve checking of macro-generated accessors against documented names

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -7233,10 +7233,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/TypeCheckMacros.cpp

@@ -1358,7 +1358,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.
@@ -1409,24 +1411,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
@@ -1447,11 +1453,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.

test/Macros/Inputs/syntax_macro_definitions.swift

@@ -506,6 +506,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@+1 2{{in expansion of macro 'myPropertyWrapper' on property 'birthDate' here}}
   @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 ('CannotHaveAccessors')}}
 @myPropertyWrapper
 struct CannotHaveAccessors {}
 // CHECK-DIAGS: 'accessor' macro cannot be attached to struct ('CannotHaveAccessors')
 #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/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

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