[Observation]: fix @Observable macro producing invalid code with certain comments #84833
Conversation
| extension VariableDeclSyntax { | ||
| func privatePrefixed(_ prefix: String, addingAttribute attribute: AttributeSyntax, removingAttribute toRemove: AttributeSyntax, in context: LocalMacroExpansionContext<some MacroExpansionContext>) -> VariableDeclSyntax { | ||
| var newAttribute = attribute | ||
| newAttribute.leadingTrivia = .newline |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| // 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 |
There was a problem hiding this comment.
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?
| | ||
| // CHECK-LABEL: {{.*}}CommentOnSameLineNoAnnotation{{.*}}ObservationTracked{{.*}}.swift | ||
| // CHECK: /* Innocent comment */ | ||
| // CHECK-NEXT: @ObservationIgnored |
There was a problem hiding this comment.
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)?
…n invalid macro expansion. The comment was 'eating' the generated variable declaration, preventing it from being visible to the compiler.
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.
| @swift-ci please smoke test macos |
3 participants
follow-up to #84224 in an attempt to address: #84145