From 5d6746d974e41beee3127b335909448deec8b12c Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Thu, 17 Aug 2023 16:59:42 -0700 Subject: [PATCH] Improve checking of macro-generated accessors against documented names The checking of the accessors generated by a macro against the documented set of accessors for the macro is slightly too strict and produces misleading error messages. Make the check slightly looser in the case where an observer-producing macro (such as `@ObservationIgnored`) is applied to a computed property. Here, we would diagnose that the observer did not in fact produce any observers, even though it couldn't have: computed properties don't get observers. Remove the diagnostic in this case. While here, add some tests and improve the wording of diagnostics a bit. Fixes rdar://113710199. --- include/swift/AST/DiagnosticsSema.def | 11 +++-- lib/Sema/TypeCheckMacros.cpp | 49 +++++++++++++------ .../Inputs/syntax_macro_definitions.swift | 20 ++++++++ test/Macros/accessor_macros.swift | 45 +++++++++++++++-- test/Serialization/macros.swift | 2 +- test/stdlib/Observation/Observable.swift | 6 +++ 6 files changed, 110 insertions(+), 23 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 95986af4eb4b3..b3a881319ab04 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -7228,10 +7228,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)) diff --git a/lib/Sema/TypeCheckMacros.cpp b/lib/Sema/TypeCheckMacros.cpp index a4797a0323d54..6cdf526be6ee2 100644 --- a/lib/Sema/TypeCheckMacros.cpp +++ b/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 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 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. diff --git a/test/Macros/Inputs/syntax_macro_definitions.swift b/test/Macros/Inputs/syntax_macro_definitions.swift index 03d24126abeac..8652d7fe2013d 100644 --- a/test/Macros/Inputs/syntax_macro_definitions.swift +++ b/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, diff --git a/test/Macros/accessor_macros.swift b/test/Macros/accessor_macros.swift index 8d7ac3bded290..3971df6abe558 100644 --- a/test/Macros/accessor_macros.swift +++ b/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 = .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 +} diff --git a/test/Serialization/macros.swift b/test/Serialization/macros.swift index 1bda24b782c02..48c991ab08075 100644 --- a/test/Serialization/macros.swift +++ b/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 diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index bd64572729a96..f9837957164e7 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/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 { }