Initial implementation of declarative and type-safe wrapper #139

akkyie · 2019-08-07T21:07:14Z

Implemented declarative and type-safe wrappers around SwiftSyntax, using @_functionBuilder.

(@akyrtzi thank you for encouraging me to upstream it!)

Example Usage

let format = Format(indentWidth: 2)

let sourceFile = SourceFile {
  Import("SwiftSyntax")

  Struct("ExampleStruct") {
    Let("syntax", of: "Syntax")
  }
}

let syntax = sourceFile.buildSyntax(format: format, leadingTrivia: .zero)

var text = ""
syntax.write(to: &text)
print(text)

emits:

import SwiftSyntax
struct ExampleStruct {
  let syntax: Syntax
}

Currently implemented

Integer and string literals (w/o interpolation) (3781538)
let and var with optional initializer (edad18d)
struct with member definition (w/o attributions, access control, generics etc.) (e8f2d02)
import (w/o importing individual declarations) (1687304)
Source file (4495871)
Changing indent width (using tabs is not yet)

Considarations

Since they are built on top of @_functionBuilder, calling them "SyntaxBuilders" sounds natural to me. But we already have SyntaxBuilders; can this conflict be a problem? I'm welcome to change my code if we have a good way to call them.
I think they can provide some syntactic safety, not semantic safety. But how do we validate the input, or should not? Should we accept something like Import("") or Let("===", of: "@@@")?
I consider strict formatting to be also out of scope, and the output of my "SyntaxBuilders" can have an opinionated format. We can provide some basic options such as changing indent width, and further formatting will be done by piping the output to swift-format or other formatters which supports SwiftSyntax.
I implemented them by hand because I think the structure and parameters of each builder don't have to exactly match the syntax tree of SwiftSyntax for better developer ergonomics. Do you have any idea to gyb-generate any part of (or, possibly, entire) the implementation?

And I'm a complete newbie here. Let me know if I have to change anything!

nkcsgexi

@akkyie Welcome to the project and this change looks awesome! Some nit-picky comments are inline.

nkcsgexi · 2019-08-09T18:40:48Z

Package.swift

+}
+
+package.products.append(.library(name: "SwiftSyntaxBuilder", targets: ["SwiftSyntaxBuilder"]))


Could you put this alone with SwiftSyntax? We'd like it to be a dynamic library when building from build script and a static library otherwise.

Should we have this functionality as part of the SwiftSyntax library instead of a separate SwiftSyntaxBuilder library? It will simplify the clients since they won't have to import two separate modules.

Since Swift doesn't yet permit dead-stripping anything that's declared public in external modules, adding more code to a module forces its binary size increase on everyone, whether the new APIs are used or not.

So in a case like this, where we have something that's adding value as a layer on top of the underlying library, but isn't critical functionality in and of itself, I think it makes sense to keep it a separate module that folks opt in to.

To clarify, if a library is statically linked with an executable, won't SwiftPM do dead-strip on it and remove public APIs that are not used in the executable? \cc @aciidb0mb3r

Unless the situation has changed since I last looked into it, anything declared as open or public is lowered by IRGen as @llvm.used, preventing it from ever being dead-stripped by the linker (SR-1021). That makes sense in a dylib/framework situation where you don't know what the eventual consumer is using, but ends up also applying to statically linked executables.

Ah interesting, I wasn't aware of that.

I was just not sure I should do so; did in bd31ddb.

Let's leave it as a separate library while it's still in flux, then we can revisit if we want it to become SwiftSyntax API later.

nkcsgexi · 2019-08-09T18:41:54Z

Sources/SwiftSyntaxBuilder/Declarations/Buildables/Variable.swift

+public typealias Let = Variable<VariableLetMutability>
+public typealias Var = Variable<VariableVarMutability>
+
+public struct Variable<Mutability: VariableMutability>: DeclBuildable {


Using generic types here for mutability seems to me an overkill. Can we put let or var keyword as part of the initializer?

Do you mean it should look like Variable(let: "something")? I think it's more natural if we can just write Let("something") and Var("something"), and afaik we need separate types (or a generic type) for this way.

nkcsgexi · 2019-08-09T18:42:55Z

Sources/SwiftSyntaxBuilder/Declarations/DeclListBuilder.swift

+import SwiftSyntax
+
+@_functionBuilder
+public struct DeclListBuilder {


This could be an internal type; so is the buildBlock function.

nkcsgexi · 2019-08-09T18:45:23Z

Sources/SwiftSyntaxBuilder/Expression/Buildables/StringLiteral.swift

+
+import SwiftSyntax
+
+public struct StringLiteral: ExprBuildable {


Could you merge all ExprBuildables into a single file? something like ExprBuildables.swift? along with the protocols.

nkcsgexi · 2019-08-09T18:45:59Z

Sources/SwiftSyntaxBuilder/Declarations/Buildables/Struct.swift

+
+import SwiftSyntax
+
+public struct Struct: DeclBuildable {


Could you merge all DeclBuildables into a single file? something like DeclBuildables.swift? along with the protocols.

nkcsgexi · 2019-08-09T18:47:58Z

Sources/SwiftSyntaxBuilder/Syntax/SyntaxListBuilder.swift

+import SwiftSyntax
+
+@_functionBuilder
+public struct SyntaxListBuilder {


This type and the buildBlock function could both be internal.

Merged into files in f2ef721, and removed public modifiers in fe3a245.

akkyie · 2019-08-14T03:22:44Z

Sorry for addition during review, but I fixed implementation to handle leading trivia (7b0e51d) along with tests for it (232a4aa), which is needed to support Comment(akkyie#2).

harlanhaskins · 2019-08-29T20:37:39Z

Sources/SwiftSyntaxBuilder/DeclBuildables.swift

+  let initializer: ExprBuildable?
+
+  public init(_ name: String, of type: String, value: ExprBuildable? = nil) {
+    self.name = name


I think type being String here is not great, but it's a good first step. I'd like to see more declarative wrappers around building types alongside declarations and expressions, and those APIs can eventually subsume this one.

Agreed. In fact, I made a Type type to express a type when it was just my toy, but couldn't figure it out how a straightforward API should look like.

harlanhaskins

This is good to land, we'll want to talk about the high-level API of how we want this to look, from a broad perspective, before committing to it. Thanks for making this, @akkyie!

akyrtzi · 2019-08-29T22:47:00Z

@swift-ci Please test

swift-ci · 2019-08-29T23:41:59Z

Build failed
Swift Test OS X Platform
Git Sha - 232a4aa

akyrtzi · 2019-08-29T23:43:51Z

CI shows this error:

/Users/buildnode/jenkins/workspace/swift-syntax-PR-macOS/branch-master/swift-syntax/Tests/SwiftSyntaxBuilderTest/FormatTests.swift:4:18: error: module 'SwiftSyntaxBuilder' was not compiled for testing
@testable import SwiftSyntaxBuilder
                 ^

akyrtzi · 2019-08-30T22:40:01Z

I'd suggest to try removing @testable and only test the public APIs.

akkyie · 2019-09-01T17:34:39Z

I agree that we should test public APIs. I added @testable not to test an internal API, but to use Format.indented which I don't see any reason to be used outside the implementation and testing.

I'm wondering if I can fix it by adding -Xswiftc -enable-testing to swift test in the build script:
https://github.com/apple/swift-syntax/blob/bc8a30794279c63b637a8918eb3c5eef75f714c5/build-script.py#L165-L172

Could you tell me if there is any reason not to do so?

benlangmuir · 2019-09-03T17:25:45Z

I don't like -enable-testing, because it means you are pessimizing the release build by treating all the internal symbols as public (increased binary size, reduced ability to optimize).

You can avoid that affecting the final toolchain by testing with -enable-testing and then doing the final release build for installation without it, but then you can't share the build between testing and install (unlike the rest of building swift), and you're not testing exactly the same configuration you ship. You can avoid -enable-testing entirely by using underscored public methods for the small number of symbols you actually need to expose. That's what I did in sourcekit-lsp.

akkyie · 2019-09-04T15:48:46Z

@benlangmuir Thank you for your explanations! That convinced me. Made some API public and underscored, and testing with the release build has succeeded on my machine.

@nkcsgexi I made a revert 9b5a8ad as tests could not be built without making function builders public.

SourceFileTests.swift:10:33: error: 'buildBlock' is inaccessible due to 'internal' protection level
    let sourceFile = SourceFile {
                                ^
<unknown>:0: note: 'buildBlock' declared here

akyrtzi · 2019-09-04T16:49:36Z

@swift-ci Please test

akyrtzi · 2019-09-04T17:03:22Z

There is a bit of "churn" with all these commits in the PR, do you mind squashing them to one commit and doing a force push?

SDGGiesbrecht · 2019-09-04T19:02:35Z

@akyrtzi, are you aware of the “Squash and merge” button? (Click the down arrow beside “Merge”.)

It has the same end effect for the commit history on master, but since it doesn’t alter the pull request, the request’s own commit timeline still corresponds to the conversation and reviews if anyone needs to look back at them in the future.

It also avoids the issues force pushing can cause across clones, even when the branch in question was never used.

akyrtzi · 2019-09-04T19:52:40Z

Does it preserve the original author? My concern is that if I do the "squash and merge" it will squash and make me the author of the commit instead of @akkyie

SDGGiesbrecht · 2019-09-04T20:33:44Z

I hadn’t thought much about that until now. From what I can tell, it results in “X authored and Y committed”, a subset of GitHub’s co‐author functionality. But I don’t know how transferable that is to other repository inspection tools.

akkyie · 2019-09-05T01:09:38Z

@akyrtzi Squashed them into 3dc22c9 :)

akyrtzi · 2019-09-05T02:28:41Z

@swift-ci Please test

akkyie · 2019-09-05T04:25:52Z

Thank you everyone so much, for all of your reviews and advice! 🙌

…ized_exprs Group the closing paren of exprs into the enclosed open/close breaks.

akyrtzi requested review from nkcsgexi and harlanhaskins August 7, 2019 21:23

nkcsgexi requested changes Aug 9, 2019

View reviewed changes

harlanhaskins reviewed Aug 29, 2019

View reviewed changes

harlanhaskins approved these changes Aug 29, 2019

View reviewed changes

nkcsgexi approved these changes Aug 29, 2019

View reviewed changes

swiftlang deleted a comment from swift-ci Sep 4, 2019

Add SwiftSyntaxBuilder

3dc22c9

akkyie force-pushed the swift-syntax-builder branch from 9b5a8ad to 3dc22c9 Compare September 5, 2019 01:04

swiftlang deleted a comment from swift-ci Sep 5, 2019

akyrtzi merged commit 7fd8a8b into swiftlang:master Sep 5, 2019

akkyie deleted the swift-syntax-builder branch October 22, 2019 15:57

adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024

Merge pull request swiftlang#139 from dylansturg/unbalanced_parenthes…

b6c05ed

…ized_exprs Group the closing paren of exprs into the enclosed open/close breaks.

		}

		package.products.append(.library(name: "SwiftSyntaxBuilder", targets: ["SwiftSyntaxBuilder"]))


		import SwiftSyntax

		public struct StringLiteral: ExprBuildable {

Initial implementation of declarative and type-safe wrapper #139

Initial implementation of declarative and type-safe wrapper #139

Conversation

akkyie commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example Usage

Currently implemented

Considarations

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akkyie Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akkyie Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akkyie commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harlanhaskins left a comment

Choose a reason for hiding this comment

Uh oh!

akyrtzi commented Aug 29, 2019

Uh oh!

swift-ci commented Aug 29, 2019

Uh oh!

akyrtzi commented Aug 29, 2019

Uh oh!

akyrtzi commented Aug 30, 2019

Uh oh!

akkyie commented Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benlangmuir commented Sep 3, 2019

Uh oh!

akkyie commented Sep 4, 2019

Uh oh!

akyrtzi commented Sep 4, 2019

Uh oh!

akyrtzi commented Sep 4, 2019

Uh oh!

SDGGiesbrecht commented Sep 4, 2019

Uh oh!

akyrtzi commented Sep 4, 2019

Uh oh!

SDGGiesbrecht commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

akkyie commented Aug 7, 2019 •

edited

Loading

akkyie Aug 10, 2019 •

edited

Loading

akkyie Aug 10, 2019 •

edited

Loading

akkyie commented Aug 14, 2019 •

edited

Loading

akkyie commented Sep 1, 2019 •

edited

Loading

SDGGiesbrecht commented Sep 4, 2019 •

edited

Loading

akkyie commented Sep 5, 2019 •

edited

Loading

akkyie commented Sep 5, 2019 •

edited

Loading