Make all syntax collections non-optional

[allevato]

Description

Let me know if I'm missing something else here, but I was updating swift-format to remove deprecation warnings around add* methods after #1958 and the code seems worse as a result.

The example I'm struggling with is when manipulating modifiers on a *DeclSyntax. For example:

return DeclSyntax(
  newEnumDecl
    .addModifier(newModifier)
    .with(\.memberBlock, newMemberBlock))

The benefit of the addModifier method was that it worked both in cases where newEnumDecl did have existing modifiers and when it didn't. But since newEnumDecl.modifiers returns DeclModifierListSyntax?, which is nil when empty. So I ended up with this:

return DeclSyntax(
  newEnumDecl
    .with(
      \.modifiers,
       (newEnumDecl.modifiers ?? DeclModifierListSyntax([])).append(newModifier))
    .with(\.memberBlock, newMemberBlock))

(Granted, I should probably look at SwiftSyntaxBuilder and string interpolation for a lot more tasks, but they didn't exist at the time most of these rules were built, so I'm trying to make smaller incremental changes.)

Would it make sense for properties that return node collections to always be non-optional, returning empty collections instead of nil? Then I could at least write:

return DeclSyntax(
  newEnumDecl
    .with(\.modifiers, newEnumDecl.modifiers.append(newModifier))
    .with(\.memberBlock, newMemberBlock))

which gets us at least closer to the original code.

Steps to Reproduce

No response

[allevato]

Sigh, I just realized part of this was being influenced by extensions to DeclModifierListSyntax that we had added in swift-format; specifically append functions that actually returned a new collection rather than mutating it in place. I should absolutely remove those, which I think now that leaves me with the following code:

    return DeclSyntax(
      newEnumDecl
        .with(\.modifiers, (newEnumDecl.modifiers ?? DeclModifierListSyntax([])) + [newModifier]))
        .with(\.memberBlock, newMemberBlock))

Making the collection non-optional would still provide a nice simplification to:

    return DeclSyntax(
      newEnumDecl
        .with(\.modifiers, newEnumDecl.modifiers + [newModifier])
        .with(\.memberBlock, newMemberBlock))

[ahoppen]

I have to admit that I didn’t have the optional case in mind when removing the add* methods. But I think they are not enough to justify the existence of the add* methods.

If you import SwiftSyntaxBuilder you can create DeclModifierListSyntax from an array literal, simplifying your example a little bit to something that isn’t a huge step down from the original.

return DeclSyntax(
  newEnumDecl
    .with(\.modifiers, (newEnumDecl.modifiers ?? []) + [newModifier]))
    .with(\.memberBlock, newMemberBlock))

And also just my personal opinion but I think in many cases it’s more readable if you modify the node instead of using with

var newEnumDecl: EnumDeclSyntax
newEnumDecl.modifiers = (newEnumDecl.modifiers ?? []) + [newModifier]
newEnumDecl.memberBlock = newMemberBlock
return DeclSyntax(newEnumDecl)

[allevato]

Thanks! Yeah, I'm going to revisit a lot of these operations with SwiftSyntaxBuilder once I get the initial warnings cleared out. I want to do that a bit more holistically.

What do you think about the idea of never returning nil collections? Is there a reason we (either SwiftSyntax, or clients) need to distinguish between a node.modifiers that is nil vs. one that is present but empty? I'm assuming the parser will never produce an empty one, but users can still construct nodes manually that end up in that state (most easily by removing modifiers from a collection that's already present), and so if possible it would be nice to coalesce those two states into one.

[ahoppen]

Tracked in Apple’s issue tracker as rdar://113638893

[ahoppen]

Very good idea. I’m making all syntax collections non-optional in #2043.