RFC: Multiple modifiers in `AccessorDeclSyntax` (for SE-0474)

public struct AccessorDeclSyntax {
 
-  public var unexpectedBetweenAttributesAndModifier: UnexpectedNodesSyntax? {
+  public var unexpectedBetweenAttributesAndModifiers: UnexpectedNodesSyntax?
 
-  public var modifier: DeclModifierSyntax?
+  public var modifiers: DeclModifierListSyntax
 
-  public var unexpectedBetweenModifierAndAccessorSpecifier: UnexpectedNodesSyntax?
+  public var unexpectedBetweenModifiersAndAccessorSpecifier: UnexpectedNodesSyntax?

and gains a new conformance:

+extension AccessorDeclSyntax: WithModifiersSyntax {}

And of course we have the new keyword:

public enum Keyword {
+  case yielding
  • Impact: This changes the API for code that examines, creates, or modifies AccessorDeclSyntax.

  • Migration: Clients will need to iterate over modifiers when evaluating accessors, and provide a modifiers: DeclParameterListSyntax instead of current modifier: DeclModifierSyntax? when creating one.

4 Likes

I realize that semantically yielding borrow is distinct from mutating get, but does it need to be syntactically? That seems like it would be less of a change. Or is it just that we literally only had mutating and nonmutating there before?

This previously only allowed one modifier keyword (e.g., “mutating”, “nonmutating”) and one keyword identifying the accessor type (e.g., “get”, “borrow”, etc). With SE-0474, we now have to support syntax with multiple modifiers: mutating yielding borrow is a valid construct.

2 Likes

In the interest of future-proofing, I would suggest that any node in the tree that takes modifiers, even just one, should be represented as a modifier list. Fortunately, from a quick code search on GitHub, I think AccessorDeclSyntax.modifier is the only such case.

We'll still need to keep modifier around as deprecated for source compatibility so that clients have an opportunity to migrate. What's the right behavior we want to support in that case—just return the first one?

Or the one that got previously returned ignoring the new modifiers, e.g. mutating in case of mutating yielding borrow? That would also not break existing behaviors and expectations.

1 Like

I’d argue this is breaking the behavior, at least for the initializer/setter - consider a macro that’s mostly copying a node and sets the modifier to the copied node’s modifier. In the past we’ve made this a hard error for that reason, I can see arguments both ways though (eg. having the shim makes it easier to upgrade the macro ecosystem as a whole).

1 Like

I’ve implemented the source-compatibility shims suggested here and plan to merge the PR as soon as tests pass. Thanks to all for the feedback!

3 Likes