Add Buildable gyb

[kimdv]

From #263

2. Actually create the Buildable types

I think with that we’re already to the main part. Here is a random collection on thoughts, how I would approach it. Again a disclaimer: I haven’t tested any of this and it might be whofully wrong.

First off, I would try and get a raw API up and running. At this stage, I wouldn’t worry about a nice API or result builders yet, I would just want to get an initialiser-based API up and running with which all syntax nodes can be created.

• As a baseline, I would start with SyntaxBuilders.swift and modify it according to our needs.
• I wouldn’t be too focused on the current design of the buildables since we probably can’t automatically re-create a generic modelling like it’s currently done for variables here. I’m fine with any design that’s easy to use and we are not concerned about source breakage here.
• As a first step, I would try modifying the SyntaxBuilders so they are purely initialiser based, that is all children are passed in the initialiser and we don’t need the use* methods anymore.

After this, I am expecting the API to be much worse than it is today, but it should be exhaustive now. This means, we now need to make it easy to use again. I’ve got a couple of (more or less) necessary improvements in my head already, I’m sure there are more once you get to it.

• Of course the entire result builder thing. Any syntax collection should be buildable using a corresponding result builder.

• Any non-optional token without custom text (i.e. a keyword or punctuator) ccan be implicitly synthesized and does not have a corresponding argument in the initialiser

  • Example: We don't need to specify the open { for a struct decl.

• And as far as I can tell we’re now already at a stage, where custom specialisations are needed. These can be made in a separate file through extensions.

  • Example: According to the specification of a VariableDecl, it is (simplified) a var/let keyword with a following PatternBindingList. But that’s not how anybody thinks about a Var decl. We should create an initialiser that’s closer to what we have today, which correctly creates the pattern binding etc.
  • Example: According to the definition of a StructDecl, it takes its members as a MemberDeclBlock, consisting of a pair of braces and decls which might be followed by a semicolon. The braces can already be implicitly synthesized as described above and nobody cares about semicolons. So we should have a specialised initialiser taking a list of decls (through a result builder) that correctly creates the MemberDeclBlock etc.

I think the advantage of this approach (auto-generate a exhaustive, but verbose API and add convenience extensions on top) apposed to the previous approach (hand write everything), is that there is always a fall-back to create every syntax element, even cutting-edge ones. I expect the convenience initialisers to concern the most commonly used syntax elements, which should also be the most stable, so I expect they should be fairly easy to maintain.

What do you think @kimdv? Does this sound good to you?

Okay, now we are ready for the next step. 🚀
(I have modified this file, not sure if it is the right, but then we can start a conversation)

I'm a little confused on how I should approach this.

• As a baseline, I would start with SyntaxBuilders.swift and modify it according to our needs.
• I wouldn’t be too focused on the current design of the buildables since we probably can’t automatically re-create a generic modelling like it’s currently done for variables here. I’m fine with any design that’s easy to use and we are not concerned about source breakage here.
• As a first step, I would try modifying the SyntaxBuilders so they are purely initialiser based, that is all children are passed in the initialiser and we don’t need the use* methods anymore.

So the idea is that inside etc StructDeclSyntaxBuilder should not have add* but all possibilities should be basses within the init method?

[ahoppen]

Disclaimer up front: I had a fairly good idea how the tokens should look like, my ideas further down become a lot more vague – there might be an altogether better design. Don’t feel like you have to stick to what I tell you as you dive into the implementation.

With that said, the way I would approach it, would be to start of by generating something like the following

// Simple syntax element
public struct IntegerLiteral: ExprBuildable {
  let digits: Token

  public init(digits: Token) {
    self.digits = digits
  }

  public func buildExpr(format: Format, leadingTrivia: Trivia) -> ExprSyntax {
    let integerLiteral = SyntaxFactory.makeIntegerLiteralExpr(
      digits: SyntaxFactory.makeIntegerLiteral(String(value))
    ).withLeadingTrivia(leadingTrivia)
    return ExprSyntax(integerLiteral)
  }
}

// More complex element
public struct ImportDecl: DeclBuildable {
  let attributes: AttributeListSyntax
  let modifiers: ModifeirListSyntax
  let importTok: Token
  let importKind: Token
  let path: AccessPathSyntax

  public init(
    attributes: AttributeListSyntax,
    modifiers: ModifeirListSyntax,
    importTok: Token,
    importKind: Token?,
    path: AccessPathSyntax,
  ) {
    self.attributes = attributes
    self.modifiers = modifiers
    self.importTok = importTok
    self.importKind = importKind
    self.path = path
  }

  public func buildDecl(format: Format, leadingTrivia: Trivia) -> DeclSyntax {
    let importDecl = SyntaxFactory.makeImportDecl(
      attributes: attributes, 
      modifiers: modifiers, 
      importTok: importTok, 
      importKind: importKind, 
      path: path
    ).withLeadingTrivia(leadingTrivia)
    return DeclSyntax(importDecl)
  }
}

// Syntax collection
public struct CodeBlockItemList: SyntaxBuildable {
  let elements: [CodeBlockItem]

  public init(elements: [CodeBlockItem]) {
    elements = elements
  }

  public func buildSyntax(format: Format, leadingTrivia: Trivia) -> DeclSyntax {
    let codeBlockItemList = makeCodeBlockItemList(elements)
      .withLeadingTrivia(leadingTrivia)
    return Syntax(codeBlockItemList)
}

Notice how each buildable takes exactly the arguments that are specified, so it’s essentially a wrapper around SyntaxFactory.

Once we have that, I imagine we can improve it as I described in the previous long post.

[ahoppen]

Looks quite good already. I see it really starting to take shape. I’ve posted a few comments inline.

I think it would also be great to have one or two test cases for the buildables. Not only to test that they actually work, but also to give us a feeling for how the API currently behaves (and how we can improve it in the future 😉). Doesn’t have to be anything fancy. I think just an enum case, a struct decl or something like this should be enough, just pick whatever is easiest to construct.

[kimdv]

Okay @ahoppen I have pushed my progress now, but tests don't pass yet! I know, but I think I need some help 😅

I have tried to make what you mentioned in this comment.

I found out that the API with leadingTrivia and format is a little hard (as I see it) to control, like have some custom indent.

If we take the SourceFileTests
We expect:

    ␣import SwiftSyntax
    struct ExampleStruct {
        let syntax: Syntax
    }

But get

␣import ␣SwiftSyntax␣struct ExampleStruct ␣{
  ␣let ␣syntax␣: ␣Syntax}

I have also tried to add some of the convince extensions for ImportDecl, SourceFile, StructDecl.
I have also tried to make some generic Var and Let.

The Python code is still some messy for me.
Etc there is some special handling for two types here.
I found there is an IdentifierToken and Token. They didn't fit in the other cases.
Maybe you can guide me to some place, so it can be more clean and easier to read?

Thanks!

[ahoppen]

Trivia and indentation

I found out that the API with leadingTrivia and format is a little hard (as I see it) to control, like have some custom indent.

Really good indentation will probably be hard to get to work in any case, so for now I’d settle with indentation that compiles, is not too horrible to look at and which e.g. swift-format could improve. But what we have right now is definitely not what we want.

What do you think about adding a newline after every line in a code block and after every member declaration? I.e. add a boolean flag to MemberDeclList and CodeBlock like elements_separated_by_newline and if that flag is set, in the builder you then add a newline between the items like so:

SyntaxFactory.make${node.syntax_kind}(elements.map { 
  $0.build${node.collection_element}(format: format, leadingTrivia: .newlines(1) + format._makeIndent())
})

(side node: you are currently passing leadingTrivia down to every child. I think that’s not correct since the leading trivia parameter is being handled by the withLeadingTrivia here. The leadingTrivia should only add that trivia to the currently constructed node, not to every nested node. format is responsible for doing that)

With that, I think, we would get the following result:

import␣SwiftSyntax␣
struct␣ExampleStruct␣{
let␣syntax␣:␣Syntax
let␣syntax2␣:␣Syntax}

which is already a lot better (and compiles 🎉). Now two things remain:

1. Indent the members inside the struct
2. Add a newline before the closing brace

For 1.) I would suggest to add a property is_indented to the Statements child of CodeBlock here (and similarly for the member decls). If that property is set, you don’t pass down format here but format._indented(), just like it is currently done in the old Struct Syntax buildable, e.g.

if child.is_indented:
  let format = format._indented()

For 2.) I would suggest adding a property requires_leading_newline to the RightBrace child of CodeBlock here (again similarly for member decls). If that property is set, you again use .newline(1) + format._makeIndent() as the child’s leading trivia here. Again, as noted above, passing down leadingTrivia like you do today, isn’t correct.

With 1.) and 2.) in, I think we should get what we want

import␣SwiftSyntax␣
struct␣ExampleStruct␣{
␣␣let␣syntax␣:␣Syntax
␣␣let␣syntax2␣:␣Syntax
}

A background note on my suggestion: I would implement the entire indentation behavior on the level of the Child classes in Python, not the Node classes because the indentation is IMHO the property of a syntax node occurring at a specific location inside a parent, not a property of the syntax node itself. For SyntaxCollections the Child nodes are implicit. Here the elements_separated_by_newline is the equivalent of having a requires_leading_newline on every of its implicit child nodes.

Convenience extensions

I have also tried to add some of the convince extensions for ImportDecl, SourceFile, StructDecl.

I would like to keep any convenience extensions out of this PR for now to (a) make it smaller and easier to review and (b) to see how far we can get with the purely generated code. If this means you need to delete some test cases, that’s totally fine with me. We can always add them again later.

Python code

The Python code is still some messy for me.

Actually, I think it’s fine. I just skimmed over it and was able to understand what’s going on. Perhaps we can move the code that determines what type a SyntaxBuildable child should have to a function to avoid duplication and make it clearer. E.g.

def syntax_buildable_child_type(syntax_kind, is_optional=False):
  if 'Token' in syntax_kind: # Alternatively, add a `is_token` method to `Node`
    buildable_type = 'TokenSyntax'
  elif node.syntax_kind in SYNTAX_BASE_KINDS:
    buildable_type = syntax_kind + 'Buildable'
  else:
    buildable_type = syntax_kind

  if is_optional:
    buildable_type += '?'

  return buildable_type

and then use it in line 69 and line 123.

I found there is an IdentifierToken and Token. They didn't fit in the other cases.

IdentifierTokens are just plain Tokens as far as the SwiftSyntax API is concerned. If you use is_token() on Child the two types are treated exactly the same.

[kimdv]

Trivia and indentation

I found out that the API with leadingTrivia and format is a little hard (as I see it) to control, like have some custom indent.

Really good indentation will probably be hard to get to work in any case, so for now I’d settle with indentation that compiles, is not too horrible to look at and which e.g. swift-format could improve. But what we have right now is definitely not what we want.

What do you think about adding a newline after every line in a code block and after every member declaration? I.e. add a boolean flag to MemberDeclList and CodeBlock like elements_separated_by_newline and if that flag is set, in the builder you then add a newline between the items like so:

SyntaxFactory.make${node.syntax_kind}(elements.map { 
  $0.build${node.collection_element}(format: format, leadingTrivia: .newlines(1) + format._makeIndent())
})

(side node: you are currently passing leadingTrivia down to every child. I think that’s not correct since the leading trivia parameter is being handled by the withLeadingTrivia here. The leadingTrivia should only add that trivia to the currently constructed node, not to every nested node. format is responsible for doing that)

With that, I think, we would get the following result:

import␣SwiftSyntax␣
struct ExampleStruct ␣{
let␣syntax␣:␣Syntax
let␣syntax2␣:␣Syntax}

which is already a lot better (and compiles 🎉). Now two things remain:

1. Indent the members inside the struct
2. Add a newline before the closing brace

For 1.) I would suggest to add a property is_indented to the Statements child of CodeBlock here (and similarly for the member decls). If that property is set, you don’t pass down format here but format._indented(), just like it is currently done in the old Struct Syntax buildable, e.g.

if child.is_indented:
  let format = format._indented()

For 2.) I would suggest adding a property requires_leading_newline to the RightBrace child of CodeBlock here (again similarly for member decls). If that property is set, you again use .newline(1) + format._makeIndent() as the child’s leading trivia here. Again, as noted above, passing down leadingTrivia like you do today, isn’t correct.

With 1.) and 2.) in, I think we should get what we want

import␣SwiftSyntax␣
struct ExampleStruct ␣{
␣␣let␣syntax␣:␣Syntax
␣␣let␣syntax2␣:␣Syntax
}

A background note on my suggestion: I would implement the entire indentation behavior on the level of the Child classes in Python, not the Node classes because the indentation is IMHO the property of a syntax node occurring at a specific location inside a parent, not a property of the syntax node itself. For SyntaxCollections the Child nodes are implicit. Here the elements_separated_by_newline is the equivalent of having a requires_leading_newline on every of its implicit child nodes.

I have added your suggestions and almost something that generates some code (But will not compile yet).
I have some thoughts that I wanted some feedback on before adding it.

Should we add something like requires_leading_space for places like (here)[https://github.com/swiftlang/swift/pull/36726/files#diff-de4680aa2bbb7151cb6595294ce91fb29fec2ecd719ae270ae1cdbd4c37a3c87R48] (and all other related places, where there is a left brace or Identifier etc).
And if requires_leading_space is set we will add a withLeadingTrivia(.spaces(1)).
The reason is, that now we are generating

␣structTestStruct{
}

Or can we make some general rule somewhere for applying all places so we don't need to add the something multiple places?

Python code

The Python code is still some messy for me.

Actually, I think it’s fine. I just skimmed over it and was able to understand what’s going on. Perhaps we can move the code that determines what type a SyntaxBuildable child should have to a function to avoid duplication and make it clearer. E.g.

def syntax_buildable_child_type(syntax_kind, is_optional=False):
  if 'Token' in syntax_kind: # Alternatively, add a `is_token` method to `Node`
    buildable_type = 'TokenSyntax'
  elif node.syntax_kind in SYNTAX_BASE_KINDS:
    buildable_type = syntax_kind + 'Buildable'
  else:
    buildable_type = syntax_kind

  if is_optional:
    buildable_type += '?'

  return buildable_type

and then use it in line 69 and line 123.

I found there is an IdentifierToken and Token. They didn't fit in the other cases.

IdentifierTokens are just plain Tokens as far as the SwiftSyntax API is concerned. If you use is_token() on Child the two types are treated exactly the same.

I have tried to add it to the gyb file?
Maybe I have misunderstood something, but should it be added in Buildables.swift.gyb or as an method on node/child?

[ahoppen]

I haven’t looked at the code yet. Let me know once you think that is ready for review.

Should we add something like requires_leading_space for places like here (and all other related places, where there is a left brace or Identifier etc).
And if requires_leading_space is set we will add a withLeadingTrivia(.spaces(1)).

I don’t think that’s necessary. IIUC when we generate the struct-Keyword with Tokens.struct, we already receive struct␣. For now I think we can put it in the user’s responsibility to create an identifier token with a trailing space so that we would get

struct␣TestStruct␣{
}

Please correct me if I’m wrong.

Later, when we create convenience constructors that take Strings instead of IdentifierTokens, these could add the trailing space.

I have tried to add it to the gyb file?
Maybe I have misunderstood something, but should it be added in Buildables.swift.gyb or as an method on node/child?

Sorry, I don’t quite understand the question. What is “it” referring to?

[kimdv]

This is needed to add correct type here etc.

[ahoppen]

In the original syntax types we are using child.type_name instead of child.syntax_kind for the children type here and node.collection_element_type instead of node.collection_element for the syntax collection’s element types here.

I think if you use that, you don’t need the special cases for List and Token. What do you think about something like the following:

def syntax_buildable_child_type(type_name, syntax_kind, is_optional=False):
  if syntax_kind in SYNTAX_BASE_KINDS:
    buildable_type = syntax_kind + 'Buildable'
  else:
    buildable_type = type_name

  ...

[kimdv]

I have just tried this, and ended up with something that we wanted with
syntax_buildable_child_type(child.type_name, child.syntax_kind, child.is_token(), child.is_optional)
or
syntax_buildable_child_type(node.collection_element_type, node.collection_element, node.is_token())

Look at the example below:

public struct AvailabilityVersionRestriction: SyntaxBuildable {
  let platform: TokenSyntax
  let version: VersionTupleSyntax?

  public init(
    platform: TokenSyntax,
    version: VersionTupleSyntax?
  ) {
    self.platform = platform
    self.version = version
  }

  ...
}

But we want

public struct AvailabilityVersionRestriction: SyntaxBuildable {
  let platform: TokenSyntax
  let version: VersionTuple?

  public init(
    platform: TokenSyntax,
    version: VersionTuple?
  ) {
    self.platform = platform
    self.version = version
  }

  ...
}

Maybe I'm doing something wrong?

[kimdv]

Okay @ahoppen, after some evenings working on it, I have something that works now.
I have commented below.

I haven’t looked at the code yet. Let me know once you think that is ready for review.

Should we add something like requires_leading_space for places like here (and all other related places, where there is a left brace or Identifier etc).
And if requires_leading_space is set we will add a withLeadingTrivia(.spaces(1)).

I don’t think that’s necessary. IIUC when we generate the struct-Keyword with Tokens.struct, we already receive struct␣. For now I think we can put it in the user’s responsibility to create an identifier token with a trailing space so that we would get

struct␣TestStruct␣{
}

Please correct me if I’m wrong.

Later, when we create convenience constructors that take Strings instead of IdentifierTokens, these could add the trailing space.

If you have time, it would be nice if you would look a little on the tests I have added.
I think it will give you an idea on how the API works now and if it matches your vision.
I have added some comments in the code, with some thoughts/ideas

I have tried to add it to the gyb file?
Maybe I have misunderstood something, but should it be added in Buildables.swift.gyb or as an method on node/child?

Sorry, I don’t quite understand the question. What is “it” referring to?

I figured it out, had some problems where to add the code, that is added here.

I first tried to add it add bottom of the gyb file, like this

%  def syntax_buildable_child_type(syntax_kind, is_token=False, is_optional=False):
%    if syntax_kind in SYNTAX_BASE_KINDS:
%      buildable_type = syntax_kind + 'Buildable'
%    elif not is_token:
%      buildable_type = syntax_kind
%
%    else:
%      buildable_type = 'TokenSyntax'
%
%    if is_optional:
%      buildable_type += '?'
%
%    return buildable_type

Again, thanks for your patience 😁

[ahoppen]

I’ve added some comments inline but they are all mostly stylistic now, nothing major.

Once you’ve addressed them, I will give the PR another review, run the test suite and get this merged. I’ve lost track but are there any related. If there are any, could you also push any pending changes to your apple/swift PR for that?

The tests look really good to me and I think we are on the right track to some really nice and exhaustive API if you imagine the following changes (which I believe are minor/trivial, but still would like to keep out of this PR)! 🎉

1. Allow omitting arguments that are nil
2. Allow omitting arguments that take a specific token and default them to that token
3. Allow passing string literals for identifiers
4. Create a result builder API for SyntaxCollections

[ahoppen]

In the original syntax types we are using child.type_name instead of child.syntax_kind for the children type here and node.collection_element_type instead of node.collection_element for the syntax collection’s element types here.

I think if you use that, you don’t need the special cases for List and Token. What do you think about something like the following:

def syntax_buildable_child_type(type_name, syntax_kind, is_optional=False):
  if syntax_kind in SYNTAX_BASE_KINDS:
    buildable_type = syntax_kind + 'Buildable'
  else:
    buildable_type = type_name

  ...

[ahoppen]

Is there a reason why Trivia is an Optional? Wouldn’t it be easier to make the trivia non-optional and passing .zero if we don’t want any? This question basically applies to all Trivia?.

[kimdv]

The reason is that if we pass .zero is replaces the leading Trivia.

We expect
␣import SwiftSyntax
or

␣struct TestStruct {
}

But get

but get ␣importSwiftSyntax

␣struct TestStruct{
}

[ahoppen]

Ah, I see. Makes total sense. Could you add a comment describing the behaviour?

[kimdv]

Would some documentation like this help?

  /// Builds list of `${kind}`s
  /// - Parameter format: The `Format` to use.
  /// - Parameter leadingTrivia: Replaces the the last leading trivia if not nil.

[kimdv]

I have address more less all off them, I have some small questions and added some comments.

I’ve added some comments inline but they are all mostly stylistic now, nothing major.

Once you’ve addressed them, I will give the PR another review, run the test suite and get this merged. I’ve lost track but are there any related. If there are any, could you also push any pending changes to your apple/swift PR for that?

Yes, there is one here: swiftlang/swift#36726

[ahoppen]

Let’s see what CI has to say about this.

swiftlang/swift#36726

@swift-ci Please test

[ahoppen]

🎉 CI passed. I’m proposing that we merge this as is and you address the following outstanding things in follow-up PRs:

• Add comment describing behavior of nil leadingTrivia (Add Buildable gyb #268 (comment))
• Use Token static properties instead of constructing tokens manually in tests (Add Buildable gyb #268 (comment))
• Add syntax node description comments to buildables (Add Buildable gyb #268 (comment))

[kimdv]

Yes!
Let's get it merged and I will fix the 3 things you mentioned after.

I just rebased on main.

[ahoppen]

swiftlang/swift#36726

@swift-ci Please test

[ahoppen]

If it didn’t come through when you put up the PR and during review, I wanted to give you a huge thank you for working on this and having the perseverance to see this patch through to the end (I only now realized that it was started more than a month ago). 🙏🏼

[ktoso]

I love this work so much, thank you @kimdv! 🥳

[kimdv]

You're welcome!
And again thanks for being patient for my, some times stupid, questions 🎉