Skip to content

Commit ded451f

Browse files
committed
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. (cherry picked from commit 5d6746d)
1 parent a189be3 commit ded451f

File tree

6 files changed

+110
-23
lines changed

6 files changed

+110
-23
lines changed

include/swift/AST/DiagnosticsSema.def

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7187,10 +7187,13 @@ ERROR(invalid_macro_role_for_macro_syntax,none,
71877187
(unsigned))
71887188
ERROR(macro_cannot_introduce_names,none,
71897189
"'%0' macros are not allowed to introduce names", (StringRef))
7190-
ERROR(macro_accessor_missing_from_expansion,none,
7191-
"expansion of macro %0 did not produce a %select{non-|}1observing "
7192-
"accessor",
7193-
(DeclName, bool))
7190+
ERROR(macro_nonobserving_accessor_missing_from_expansion,none,
7191+
"expansion of macro %0 did not produce a non-observing accessor "
7192+
"(such as 'get') as expected",
7193+
(DeclName))
7194+
ERROR(macro_nonobserver_unexpected_in_expansion,none,
7195+
"expansion of macro %0 produced an unexpected %1",
7196+
(DeclName, DescriptiveDeclKind))
71947197
ERROR(macro_init_accessor_not_documented,none,
71957198
"expansion of macro %0 produced an unexpected 'init' accessor",
71967199
(DeclName))

lib/Sema/TypeCheckMacros.cpp

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1362,7 +1362,9 @@ bool swift::accessorMacroOnlyIntroducesObservers(
13621362
for (auto name : attr->getNames()) {
13631363
if (name.getKind() == MacroIntroducedDeclNameKind::Named &&
13641364
(name.getName().getBaseName().userFacingName() == "willSet" ||
1365-
name.getName().getBaseName().userFacingName() == "didSet")) {
1365+
name.getName().getBaseName().userFacingName() == "didSet" ||
1366+
name.getName().getBaseName().getKind() ==
1367+
DeclBaseName::Kind::Constructor)) {
13661368
foundObserver = true;
13671369
} else {
13681370
// Introduces something other than an observer.
@@ -1413,24 +1415,28 @@ llvm::Optional<unsigned> swift::expandAccessors(AbstractStorageDecl *storage,
14131415
// Trigger parsing of the sequence of accessor declarations. This has the
14141416
// side effect of registering those accessor declarations with the storage
14151417
// declaration, so there is nothing further to do.
1416-
bool foundNonObservingAccessor = false;
1417-
bool foundNonObservingAccessorInMacro = false;
1418-
bool foundInitAccessor = false;
1418+
AccessorDecl *foundNonObservingAccessor = nullptr;
1419+
AccessorDecl *foundNonObservingAccessorInMacro = nullptr;
1420+
AccessorDecl *foundInitAccessor = nullptr;
14191421
for (auto accessor : storage->getAllAccessors()) {
1420-
if (accessor->isInitAccessor())
1421-
foundInitAccessor = true;
1422+
if (accessor->isInitAccessor()) {
1423+
if (!foundInitAccessor)
1424+
foundInitAccessor = accessor;
1425+
continue;
1426+
}
14221427

14231428
if (!accessor->isObservingAccessor()) {
1424-
foundNonObservingAccessor = true;
1429+
if (!foundNonObservingAccessor)
1430+
foundNonObservingAccessor = accessor;
14251431

1426-
if (accessor->isInMacroExpansionInContext())
1427-
foundNonObservingAccessorInMacro = true;
1432+
if (!foundNonObservingAccessorInMacro &&
1433+
accessor->isInMacroExpansionInContext())
1434+
foundNonObservingAccessorInMacro = accessor;
14281435
}
14291436
}
14301437

14311438
auto roleAttr = macro->getMacroRoleAttr(MacroRole::Accessor);
1432-
bool expectedNonObservingAccessor =
1433-
!accessorMacroOnlyIntroducesObservers(macro, roleAttr);
1439+
bool expectObservers = accessorMacroOnlyIntroducesObservers(macro, roleAttr);
14341440
if (foundNonObservingAccessorInMacro) {
14351441
// If any non-observing accessor was added, mark the initializer as
14361442
// subsumed unless it has init accessor, because the initializer in
@@ -1451,11 +1457,24 @@ llvm::Optional<unsigned> swift::expandAccessors(AbstractStorageDecl *storage,
14511457
storage->removeAccessor(accessor);
14521458
}
14531459

1454-
// Make sure we got non-observing accessors exactly where we expected to.
1455-
if (foundNonObservingAccessor != expectedNonObservingAccessor) {
1460+
// If the macro told us to expect only observing accessors, but the macro
1461+
// produced a non-observing accessor, it could have converted a stored
1462+
// property into a computed one without telling us pre-expansion. Produce
1463+
// an error to prevent this.
1464+
if (expectObservers && foundNonObservingAccessorInMacro) {
1465+
storage->diagnose(
1466+
diag::macro_nonobserver_unexpected_in_expansion, macro->getName(),
1467+
foundNonObservingAccessorInMacro->getDescriptiveKind());
1468+
}
1469+
1470+
// We expected to get a non-observing accessor, but there isn't one (from
1471+
// the macro or elsewhere), meaning that we counted on this macro to make
1472+
// this stored property into a a computed property... but it didn't.
1473+
// Produce an error.
1474+
if (!expectObservers && !foundNonObservingAccessor) {
14561475
storage->diagnose(
1457-
diag::macro_accessor_missing_from_expansion, macro->getName(),
1458-
!expectedNonObservingAccessor);
1476+
diag::macro_nonobserving_accessor_missing_from_expansion,
1477+
macro->getName());
14591478
}
14601479

14611480
// 'init' accessors must be documented in the macro role attribute.

test/Macros/Inputs/syntax_macro_definitions.swift

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,26 @@ extension PropertyWrapperSkipsComputedMacro: AccessorMacro, Macro {
516516
}
517517
}
518518

519+
public struct WillSetMacro: AccessorMacro {
520+
public static func expansion(
521+
of node: AttributeSyntax,
522+
providingAccessorsOf declaration: some DeclSyntaxProtocol,
523+
in context: some MacroExpansionContext
524+
) throws -> [AccessorDeclSyntax] {
525+
guard let varDecl = declaration.as(VariableDeclSyntax.self),
526+
let binding = varDecl.bindings.first,
527+
let identifier = binding.pattern.as(IdentifierPatternSyntax.self)?.identifier else {
528+
return []
529+
}
530+
531+
return [
532+
"""
533+
willSet { }
534+
"""
535+
]
536+
}
537+
}
538+
519539
public struct WrapAllProperties: MemberAttributeMacro {
520540
public static func expansion(
521541
of node: AttributeSyntax,

test/Macros/accessor_macros.swift

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,8 @@
33
// RUN: %empty-directory(%t)
44
// 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
55

6-
// First check for no errors.
7-
// RUN: %target-typecheck-verify-swift -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition)
8-
96
// Check for expected errors.
7+
// RUN: %target-typecheck-verify-swift -swift-version 5 -load-plugin-library %t/%target-library-name(MacroDefinition) -DTEST_DIAGNOSTICS -verify-ignore-unknown
108
// 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
119
// RUN: %FileCheck -check-prefix=CHECK-DIAGS %s < %t/diags.txt
1210

@@ -98,21 +96,62 @@ ms.favoriteColor = "Yellow"
9896
struct MyBrokenStruct {
9997
var _birthDate: MyWrapperThingy<Date?> = .init(storage: nil)
10098

99+
// expected-note@+1 2{{in expansion of macro 'myPropertyWrapper' on property 'birthDate' here}}
101100
@myPropertyWrapper
102101
var birthDate: Date? {
103102
// CHECK-DIAGS: variable already has a getter
104103
// CHECK-DIAGS: in expansion of macro
105104
// CHECK-DIAGS: previous definition of getter here
106105
get { fatalError("Boom") }
106+
// expected-note @-1{{previous definition of getter here}}
107107

108108
// CHECK-DIAGS: variable already has a setter
109109
// CHECK-DIAGS: in expansion of macro
110110
// CHECK-DIAGS: previous definition of setter here
111111
set { fatalError("Boom") }
112+
// expected-note @-1{{previous definition of setter here}}
112113
}
113114
}
114115

116+
// expected-error@+1{{'accessor' macro cannot be attached to struct ('CannotHaveAccessors')}}
115117
@myPropertyWrapper
116118
struct CannotHaveAccessors {}
117119
// CHECK-DIAGS: 'accessor' macro cannot be attached to struct
118120
#endif
121+
122+
123+
124+
@attached(accessor, names: named(willSet))
125+
macro SkipsComputed() =
126+
#externalMacro(module: "MacroDefinition", type: "PropertyWrapperSkipsComputedMacro")
127+
128+
struct HasComputed {
129+
@SkipsComputed
130+
var value: Int { 17 }
131+
}
132+
133+
@attached(accessor, names: named(willSet))
134+
macro AddWillSet() =
135+
#externalMacro(module: "MacroDefinition", type: "WillSetMacro")
136+
137+
@attached(accessor)
138+
macro AddWillSetSneakily() =
139+
#externalMacro(module: "MacroDefinition", type: "WillSetMacro")
140+
141+
@attached(accessor, names: named(willSet))
142+
macro MakeComputedSneakily() =
143+
#externalMacro(module: "MacroDefinition", type: "PropertyWrapperMacro")
144+
145+
struct HasStoredTests {
146+
@AddWillSet var x: Int = 0
147+
148+
#if TEST_DIAGNOSTICS
149+
@AddWillSetSneakily var y: Int = 0
150+
// expected-error@-1{{expansion of macro 'AddWillSetSneakily()' did not produce a non-observing accessor (such as 'get') as expected}}
151+
152+
@MakeComputedSneakily var z: Int = 0
153+
// expected-error@-1{{expansion of macro 'MakeComputedSneakily()' produced an unexpected getter}}
154+
// expected-note@-2 2{{in expansion of macro}}
155+
// expected-note@-3 2{{'z' declared here}}
156+
#endif
157+
}

test/Serialization/macros.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ func test(a: Int, b: Int) {
2121

2222
struct TestStruct {
2323
@myWrapper var x: Int
24-
// expected-error@-1{{expansion of macro 'myWrapper()' did not produce a non-observing accessor}}
24+
// expected-error@-1{{expansion of macro 'myWrapper()' did not produce a non-observing accessor (such as 'get') as expected}}
2525
}
2626

2727
@ArbitraryMembers

test/stdlib/Observation/Observable.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,12 @@ class IsolatedInstance {
155155
var test = "hello"
156156
}
157157

158+
@Observable
159+
class IgnoredComputed {
160+
@ObservationIgnored
161+
var message: String { "hello" }
162+
}
163+
158164
@Observable
159165
class ClassHasExistingConformance: Observable { }
160166

0 commit comments

Comments
 (0)