Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 31, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d1e482
[Observation] fix bug where a comment in an annotation would result i…
ZevEisenberg Sep 11, 2025
21b57f7
[fix][Observation]: further attempts to resolve macro expansion
jamieQ Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions lib/Macros/Sources/ObservationMacros/ObservableMacro.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@bnbarham bnbarham Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jamieQ jamieQ Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@bnbarham bnbarham Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

@jamieQ jamieQ Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@bnbarham bnbarham Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


let newAttributes = attributes.filter { attribute in
switch attribute {
case .attribute(let attr):
attr.attributeName.identifier != toRemove.attributeName.identifier
default: true
}
} + [.attribute(attribute)]
} + [.attribute(newAttribute)]

var newModifiers = modifiers.privatePrefixed(prefix, in: context)
let hasModifiers = !newModifiers.isEmpty
if hasModifiers {
newModifiers.leadingTrivia += .newline
}

return VariableDeclSyntax(
leadingTrivia: leadingTrivia,
attributes: newAttributes,
modifiers: modifiers.privatePrefixed(prefix, in: context),
bindingSpecifier: TokenSyntax(bindingSpecifier.tokenKind, leadingTrivia: .space, trailingTrivia: .space, presence: .present),
modifiers: newModifiers,
bindingSpecifier: TokenSyntax(
bindingSpecifier.tokenKind,
leadingTrivia: hasModifiers ? .space : .newline,
trailingTrivia: .space,
presence: .present
),
bindings: bindings.privatePrefixed(prefix, in: context),
trailingTrivia: trailingTrivia
)
Expand Down
69 changes: 69 additions & 0 deletions test/stdlib/Observation/Macros/observation_macro_expansion_observable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// 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 -swift-version 5 -typecheck -plugin-path %swift-plugin-dir -I %t -dump-macro-expansions %s 2>&1 | %FileCheck %s --color

import Observation

// Test cases for comment handling with Observable macro
_ = 0 // absorbs trivia so file check expectations don't leak into macro expansions

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
@Observable
final class CommentAfterGlobalActorAnnotation {
@MainActor // Innocent comment
internal var it = 0
}

// CHECK-LABEL: @__swiftmacro{{.*}}CommentAfterGlobalActorAnnotation{{.*}}ObservationTracked{{.*}}.swift
// CHECK: @MainActor // Innocent comment
// CHECK-NEXT: @ObservationIgnored
// CHECK-NEXT: private var _it = 0
_ = 0

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
@Observable
final class CommentAfterAvailabilityAnnotation {
@available(*, deprecated) // Innocent comment
internal var it = 0
}

// CHECK-LABEL: @__swiftmacro{{.*}}CommentAfterAvailabilityAnnotation{{.*}}ObservationTracked{{.*}}.swift
// CHECK-NEXT: {{-+}}
// CHECK-NEXT: @available(*, deprecated) // Innocent comment
// CHECK-NEXT: @ObservationIgnored
// CHECK-NEXT: private var _it = 0
_ = 0

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
@Observable
final class CommentOnSameLineAsOtherAnnotation {
@MainActor /* Innocent comment */ public var it = 0
}

// CHECK-LABEL: @__swiftmacro{{.*}}CommentOnSameLineAsOtherAnnotation{{.*}}ObservationTracked{{.*}}.swift
// CHECK: @MainActor /* Innocent comment */
// CHECK-NEXT: @ObservationIgnored
// CHECK-NEXT: private var _it = 0
_ = 0

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
@Observable
final class CommentOnSameLineNoAnnotation {
/* Innocent comment */ public /*1*/ final /*2*/ var /*3*/ it /*4*/ = 0
}

// Note: seems there's some weirdness with the existing macro eating/duplicating trivia in some
// cases but we'll just test for the current behavior since it doesn't seem to be covered elsewhere:

// CHECK-LABEL: @__swiftmacro{{.*}}CommentOnSameLineNoAnnotation{{.*}}ObservationTracked{{.*}}.swift
// CHECK: /* Innocent comment */
// CHECK-NEXT: @ObservationIgnored
Copy link
Contributor Author

@jamieQ jamieQ Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

@bnbarham bnbarham Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

// CHECK-NEXT: private final /*2*/ var _it /*4*/ /*4*/ = 0
_ = 0