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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[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
d11f348
[fix][Observation]: further attempts to resolve macro expansion
jamieQ Oct 10, 2025
File filter

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

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

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?

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
51 changes: 51 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,51 @@
// 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
Comment on lines +1 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

import Observation

// Test cases for comment handling with Observable macro

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

// CHECK-LABEL: {{.*}}CommentOnLineBeforeOtherAnnotation{{.*}}ObservationTracked{{.*}}.swift
// CHECK: @MainActor // Innocent comment
// CHECK-NEXT: @ObservationIgnored
// CHECK-NEXT: private var _it = 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: {{.*}}CommentOnSameLineAsOtherAnnotation{{.*}}ObservationTracked{{.*}}.swift
// CHECK: @MainActor /* Innocent comment */
// CHECK-NEXT: @ObservationIgnored
// CHECK-NEXT: private var _it = 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: {{.*}}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)?
// CHECK-NEXT: private final /*2*/ var _it /*4*/ /*4*/ = 0