[SE-0474] Support yielding borrow and yielding mutate accessors

[tbkka]

yielding borrow and yielding mutate are the final approved names for the accessors previously known as read and modify (which are themselves improvements over the earlier non-standard _read and _modify accessors).

Since some people are already using these under the read and modify names, we want to support the old and new names as synonyms for a transition period until everyone has had a chance to migrate.

RFC discussion: https://forums.swift.org/t/rfc-multiple-modifiers-in-accessordeclsyntax-for-se-0474/83316

[tbkka]

This isn't working out so well. There seem to be two basic problems:

• Treating yielding borrow as a synonym for read
• Having yielding borrow be a "keyword" that's really two words

[tbkka]

It's not clear from SE-0474 whether yielding mutating borrow should be accepted, or only mutating yielding borrow. @DougGregor @rjmccall -- Any insights here?

If the former is accepted, then it might make sense to model yielding as a modifier here; otherwise, I'll need to do something more complicated.

[tbkka]

Can anyone suggest how this should work? @bnbarham @rintaro @hamishknight ?? I'm completely stumped. What I have here seems entirely wrong, but I have no idea what approach should be used here. In particular:

• Mapping yielding borrow to read breaks round-tripping, so it seems like I somehow need to model yielding borrow as a distinct construct from read
• It seems like yielding borrow must be modeled as two Nodes? Otherwise, trivia between those tokens can't be modeled?
• yielding can't be a "modifier" without other changes, since mutating yielding borrow is legal, and that would have two modifiers, which isn't supported in the current accessor modeling. So either accessors need to support a set of modifiers? Or something different?

[tbkka]

Maybe I could expand AccessorIntroducer with a new yielding field like this?

  struct AccessorIntroducer {
    var attributes: RawAttributeListSyntax
    var modifier: RawDeclModifierSyntax?
    var yielding: RawAccessorYieldingSyntax?
    var kind: AccessorDeclSyntax.AccessorSpecifierOptions
    var unexpectedBeforeToken: RawUnexpectedNodesSyntax?
    var token: RawTokenSyntax
  }

and then I'd have to reverse-engineer the code generator to construct RawAccessorYieldingSyntax and update everywhere an AccessorIntroducer was used to properly handle the new field? Does that make any sense?

[tbkka]

It would be so helpful if the generated code included comments indicating where the data came from for each particular generated type and/or file.

[bnbarham]

It's not clear from SE-0474 whether yielding mutating borrow should be accepted, or only mutating yielding borrow. @DougGregor @rjmccall -- Any insights here?

I haven't been following the proposal closely, but from a quick read, it has eg.

Like a set accessor, a yielding mutate accessor in a struct or enum property is mutating by default (unless explicitly declared a nonmutating yielding mutate)

Confusing set of terms IMO (nonmutating mutate?), but set accessors allow specifying mutating (despite being redundant), so seems reasonable to allow here.

yielding can't be a "modifier" without other changes, since mutating yielding borrow is legal, and that would have two modifiers, which isn't supported in the current accessor modeling. So either accessors need to support a set of modifiers? Or something different?

Given the proposal mentions the possibility of a non-yielding borrow/mutate, seems that yielding is most like a modifier. So yes, I'd go with:

So either accessors need to support a set of modifiers?

The next question is whether there should be an order to these, ie. is yielding nonmutating mutate allowed?

[tbkka]

What does this mean?

Build of product 'generate-swift-syntax' complete! (0.08s)
SyntaxSupport/Child.swift:370: Fatal error: unexpected node has no siblings?

[tbkka]

@swift-ci Please test

[tbkka]

I ended up just making yielding be another modifier, handled the same as mutating or nonmutating. The current PR allows any set of modifiers in any order -- more work will be required downstream in ASTGen or Sema to diagnose conflicts or ordering issues.

[tbkka]

In the RFC discussion, @allevato suggested:

We'll still need to keep modifier around as deprecated for source compatibility so that clients have an opportunity to migrate.

@hamishknight What's the right way to implement this? I suppose I could put back the modifier field in DeclNodes.swift, but that won't get the deprecation right. I could also hand-code the modifier field in an extension, I suppose. Thoughts?

[hamishknight]

We have a compatibility layer in SwiftSyntaxCompatibility.swift you could add to, I'm a little weary of adding one for modifier though, I think I would rather clients immediately migrate after bumping their swift-syntax version, unless it's particularly prevalent. AFAIK we don't guarantee source compatibility between swift-syntax versions, the compatibility overloads just serve as a way to ease the burden of migrating. It could potentially be worth adding a compatibility overload for the AccessorDecl constructor though since that can just forward a single modifier. What do you think @bnbarham?

[hamishknight]

Though even if we don't add a compatibility layer for modifier, we could still add an unavailable stub for it that points users towards the new name

[allevato]

Unless it's technically infeasible (e.g., in cases where the entire structure of a node must change), I don't think we should omit compatibility shims in general. Even if swift-syntax doesn't strictly offer source compatibility, it's still often the right thing to do when it's possible, because macro authors get the opportunity to migrate on their terms, and you don't have to pair this change with parallel changes to other swiftlang/swift-* projects that might to be updated, like swift-format and possibly sourcekit-lsp.

In this case, I don't think there are any technical challenges here. The singular property can be marked deprecated but return either the first modifier or it could look through the list for mutating and return that if present or nil if not.

[hamishknight]

you don't have to pair this change with parallel changes to other swiftlang/swift-* projects that might to be updated

Well that's the thing, I think we definitely want to make sure we update any uses in swiftlang projects since otherwise there's a good chance we're not handling this new language feature correctly. I think the same logic also extends to macros when they choose to adopt a new version of the language, as long as this isn't prevalent enough for that to be a burden. I'll let @bnbarham decide though.

[tbkka]

@allevato I implemented backwards-compatibility shims. Let me know if this looks right to you.

[bnbarham]

What do you think @bnbarham?

The main issue I see is with the setter + initializer, since a macro that's intending to copy a whole node could now remove a modifier. Maybe best to discuss in the RFC thread?

[ahoppen]

One minor suggestion, otherwise LGTM.

[ahoppen]

We can save a few bytes in the syntax tree by using a shared empty collection. Also for RawAttributeListSyntax now that I look at it.

Suggested change

	attributes: RawAttributeListSyntax(elements: [], arena: arena),
	modifier: nil,
	modifiers: RawDeclModifierListSyntax(elements: [], arena: arena),
	attributes: self.emptyCollection(RawAttributeListSyntax.self),
	modifiers: self.emptyCollection(RawDeclModifierListSyntax.sel),