Skip to content

Conversation

@ZevEisenberg
Copy link
Contributor

Resolves #84145.

Before

This code would fail to compile:

@Observable
final class MyClass {
  @MainActor // some comment here
  var it = 0
}

This is because the comment after @MainActor was ending up as a prefix on the same line as the generated _it variable in the expanded code.

After

The code compiles because the _it variable is moved to a new line instead of at the end of the line with the comment.

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

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

Thanks for putting this up @ZevEisenberg!

attributes: newAttributes,
modifiers: modifiers.privatePrefixed(prefix, in: context),
bindingSpecifier: TokenSyntax(bindingSpecifier.tokenKind, leadingTrivia: .space, trailingTrivia: .space, presence: .present),
bindingSpecifier: TokenSyntax(bindingSpecifier.tokenKind, leadingTrivia: .newline, trailingTrivia: .space, presence: .present),
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we'd actually want a newline on the modifiers if there are any, then a space on the var/let if there is, and a newline otherwise. Also need trivia on the added attribute, as otherwise it'll still be added after the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way you'd recommend making a test case to confirm what the output looks like? Right now I'm just relying on a test that confirms that the output compiles, but not what it actually looks like.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use -dump-macro-expansions. There should be a few tests that do that today

Copy link
Contributor

Choose a reason for hiding this comment

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

@bnbarham i discussed with Zev, and i'll try to push this forward here: #84833 – i took a stab at incorporating your feedback and adding some filecheck tests of (a portion of) the macro output. would appreciate any further input!

@ZevEisenberg
Copy link
Contributor Author

Closed in favor of #84833, which has merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@Observable macro breaks when property has attached macro with a // comment on the same line

3 participants