[Observation]: fix @Observable macro producing invalid code with certain comments #84833

jamieQ · 2025-10-10T23:57:31Z

follow-up to #84224 in an attempt to address: #84145

bnbarham · 2025-10-11T02:43:41Z

lib/Macros/Sources/ObservationMacros/ObservableMacro.swift

@@ -272,18 +272,33 @@ extension PatternBindingListSyntax {

 extension VariableDeclSyntax {
  func privatePrefixed(_ prefix: String, addingAttribute attribute: AttributeSyntax, removingAttribute toRemove: AttributeSyntax, in context: LocalMacroExpansionContext<some MacroExpansionContext>) -> VariableDeclSyntax {
+    var newAttribute = attribute
+    newAttribute.leadingTrivia = .newline


This removes any existing leading trivia, as does the leadingTrivia:... in the bindingSpecifier below. I'm not sure that's a bad thing though.

IMO in an ideal world we'd just strip all trivia for the added var, then let BasicFormat handle adding the appropriate whitespace on the resulting var. Any opinions @hamishknight / @rintaro / @phausler?

IIUC in this particular case, we'd be stripping the leading trivia from the addingAttribute parameter, which is currently only ever ObservableMacro.ignoredAttribute (AFAICT). an alternative i played with was just making the ignoredAttribute (and its sibling) automatically be surrounded by newline trivia to 'break them off' to an extent from wherever they get inserted. ultimately went with this strategy first per your earlier suggestions. the leadingTrivia: passed to the new binding specifier token syntax should still basically have the same behavior as before just with different whitespace, right?

Hmm, fair enough. I'd really prefer:

IMO in an ideal world we'd just strip all trivia for the added var, then let BasicFormat handle adding the appropriate whitespace on the resulting var

here, but it's up to you if you want to tackle that or not. Adding the newline at least fixes the trailing comment case, so if you'd like to stop there that's okay with me :)

@bnbarham thanks for the feedback – i'm happy to take a shot at implementing your preferred solution, but i may be stumbling around for a bit (and have further questions) before i get there.

It's a much larger change - we can take this one either way. Want to fix up the -target and then I'll run tests?

jamieQ · 2025-10-11T14:22:05Z

test/stdlib/Observation/Macros/observation_macro_expansion_observable.swift

+// REQUIRES: swift_swift_parser, asserts
+//
+// UNSUPPORTED: back_deploy_concurrency
+// REQUIRES: concurrency
+// REQUIRES: observation
+//
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t-scratch)
+
+// RUN: %target-swift-frontend -typecheck -target %target-swift-5.7-abi-triple -plugin-path %swift-plugin-dir -I %t -dump-macro-expansions %s 2>&1 | %FileCheck %s --color


if anyone has suggestions about the right preamble to use here i'd be glad to learn more. i basically just randomly copied things from other tests that included the -dump-macro-expansions option until things seemed to work okay when running with lit. in particular, is there some way to avoid having to add the availability annotations on everything below?

You shouldn't need -target - it'll be added by %target-swift-frontend already. Probably want a -swift-version 5 as we default to 4 otherwise, but the rest LGTM

thanks! updated the setup, and made some minor test adjustments.

tangential, but i also noticed that there is a weird issue with the fact that the dump-macro-expansion output can end up re-emitting source comments, and those comments can themselves be FileCheck directives... so i added some 'dummy' expressions to ensure the FileCheck trivia doesn't accidentally show up in the expansion patterns that are verified. if there's a better way to deal with that problem, i can take a look as a follow-up.

ah, i may have misunderstood this suggestion... i removed the entire -target %target-swift-5.7-abi-triple options, but perhaps you meant that only the -target was redundant. like i said i just copied this from some other test (@Distributed macro stuff maybe?), and don't have a great understanding of why you'd pick a particular target substitution of that form... now that i look at the Testing.md docs a bit more closely it seems i likely should have used %target-typecheck-verify-swift here. still a bit unclear on exactly what the extra 'abi-triple' stuff controls and when you'd want to use it though. please lmk if you think any of this should be changed before merging.

jamieQ · 2025-10-11T14:24:07Z

test/stdlib/Observation/Macros/observation_macro_expansion_observable.swift

+
+// CHECK-LABEL: {{.*}}CommentOnSameLineNoAnnotation{{.*}}ObservationTracked{{.*}}.swift
+// CHECK:       /* Innocent comment */
+// CHECK-NEXT:  @ObservationIgnored


in 'reality' the dump macro expansion output had some leading whitespace here, but i guess that is ignored in this form of filecheck expectation. is there a way to test for that? i.e. the 'true' output looked something like:

------------------------------ @__swiftmacro_38observation_macro_expansion_observable29CommentOnSameLineNoAnnotationC2it18ObservationTrackedfMp_.swift ------------------------------ /* Innocent comment */ @ObservationIgnored private final /*2*/ var _it /*4*/ /*4*/ = 0

edit: also, note how the whitespace between the comments also appears to be slightly different from the dumped output and what we're testing against (2 spaces in the actual output b/w the '/4/' comments vs only 1 in the tests)... anyone know why that is (and why the test works in this case)?

For the latter question, file check canonicalizes all whitespace by default - you can turn that off and test for it with --strict-whitespace when running FileCheck.

As for why we're getting 2 here, I assume it's because we're getting a copy of the /*4*/ trivia from somewhere 😅

…n invalid macro expansion. The comment was 'eating' the generated variable declaration, preventing it from being visible to the compiler.

jamieQ · 2025-10-11T21:57:18Z

@swift-ci please smoke test macos

jamieQ · 2025-10-17T13:51:03Z

@bnbarham / @hamishknight / @rintaro / @phausler – any further thoughts or feedback on this approach, or better alternative ways to solve this?

bnbarham

Sorry for the late reply @jamieQ and thanks again for looking at this!

bnbarham · 2025-10-24T00:23:49Z

test/stdlib/Observation/Macros/observation_macro_expansion_observable.swift

+
+// CHECK-LABEL: {{.*}}CommentOnSameLineNoAnnotation{{.*}}ObservationTracked{{.*}}.swift
+// CHECK:       /* Innocent comment */
+// CHECK-NEXT:  @ObservationIgnored


For the latter question, file check canonicalizes all whitespace by default - you can turn that off and test for it with --strict-whitespace when running FileCheck.

As for why we're getting 2 here, I assume it's because we're getting a copy of the /*4*/ trivia from somewhere 😅

bnbarham · 2025-10-24T00:25:57Z

lib/Macros/Sources/ObservationMacros/ObservableMacro.swift

@@ -272,18 +272,33 @@ extension PatternBindingListSyntax {

 extension VariableDeclSyntax {
  func privatePrefixed(_ prefix: String, addingAttribute attribute: AttributeSyntax, removingAttribute toRemove: AttributeSyntax, in context: LocalMacroExpansionContext<some MacroExpansionContext>) -> VariableDeclSyntax {
+    var newAttribute = attribute
+    newAttribute.leadingTrivia = .newline


Hmm, fair enough. I'd really prefer:

IMO in an ideal world we'd just strip all trivia for the added var, then let BasicFormat handle adding the appropriate whitespace on the resulting var

here, but it's up to you if you want to tackle that or not. Adding the newline at least fixes the trailing comment case, so if you'd like to stop there that's okay with me :)

bnbarham · 2025-10-24T00:28:43Z

test/stdlib/Observation/Macros/observation_macro_expansion_observable.swift

+// REQUIRES: swift_swift_parser, asserts
+//
+// UNSUPPORTED: back_deploy_concurrency
+// REQUIRES: concurrency
+// REQUIRES: observation
+//
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t-scratch)
+
+// RUN: %target-swift-frontend -typecheck -target %target-swift-5.7-abi-triple -plugin-path %swift-plugin-dir -I %t -dump-macro-expansions %s 2>&1 | %FileCheck %s --color


You shouldn't need -target - it'll be added by %target-swift-frontend already. Probably want a -swift-version 5 as we default to 4 otherwise, but the rest LGTM

jamieQ · 2025-10-25T14:35:11Z

@swift-ci please smoke test

jamieQ · 2025-10-25T15:55:19Z

@swift-ci please test source compatibility

interaction with comments Adds logic to insert newlines in various places to try and resolve the fact that the current expansion produces invalid code in some cases depending on comment location. Adds some basic tests of the expansion output.

jamieQ · 2025-10-26T01:41:47Z

@swift-ci please smoke test

jamieQ · 2025-10-26T01:42:28Z

@swift-ci Please Test Source Compatibility Debug

jamieQ · 2025-10-26T10:45:35Z

@swift-ci please clean smoke test macos

jamieQ · 2025-10-27T10:32:03Z

@bnbarham i made some minor changes to the tests per your feedback (and some additional discoveries). if this looks okay to you please merge when convenient, or let me know if there's anything else you think needs to be addressed first. thanks!

bnbarham

Thanks @jamieQ!

jamieQ requested a review from a team as a code owner October 10, 2025 23:57

jamieQ mentioned this pull request Oct 10, 2025

[Observation] #84145: comment after annotation causing invalid macro expansion #84224

Open

bnbarham reviewed Oct 11, 2025

View reviewed changes

jamieQ commented Oct 11, 2025

View reviewed changes

[Observation] fix bug where a comment in an annotation would result i…

0d1e482

…n invalid macro expansion. The comment was 'eating' the generated variable declaration, preventing it from being visible to the compiler.

jamieQ force-pushed the obs-macro-ii branch from 8bf1d49 to d11f348 Compare October 11, 2025 21:56

bnbarham reviewed Oct 24, 2025

View reviewed changes

jamieQ force-pushed the obs-macro-ii branch from d11f348 to 0daacdc Compare October 25, 2025 14:34

jamieQ force-pushed the obs-macro-ii branch from 0daacdc to 21b57f7 Compare October 26, 2025 01:40

bnbarham approved these changes Oct 31, 2025

View reviewed changes

bnbarham merged commit c978386 into swiftlang:main Oct 31, 2025
4 checks passed

jamieQ deleted the obs-macro-ii branch November 1, 2025 09:39

[Observation]: fix @Observable macro producing invalid code with certain comments #84833

[Observation]: fix @Observable macro producing invalid code with certain comments #84833

Conversation

jamieQ commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamieQ Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamieQ commented Oct 11, 2025

Uh oh!

jamieQ commented Oct 17, 2025

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamieQ commented Oct 25, 2025

Uh oh!

jamieQ commented Oct 25, 2025

Uh oh!

jamieQ commented Oct 26, 2025

Uh oh!

jamieQ commented Oct 26, 2025

Uh oh!

jamieQ commented Oct 26, 2025

Uh oh!

jamieQ commented Oct 27, 2025

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bnbarham Oct 24, 2025 •

edited

Loading

jamieQ Oct 11, 2025 •

edited

Loading

bnbarham Oct 24, 2025 •

edited

Loading