[Syntax] Swift libSyntax API

[harlanhaskins]

This patch is an initial implementation of the Swift libSyntax API. It
aims to provide all features of the C++ API but exposed to Swift.

It currently resides in tools/ and will likely exist in a
molten state for a while.

Tasks remaining:

• Get the tests passing
• Write more tests
• Figure out where this should live (It lives in tools/)
• Review API for Swiftiness.

[nkcsgexi]

woot!

[felix91gr]

I get the reference of your commit message 👀
Also, what is libSyntax for? I couldn’t find such a library for C++

[harlanhaskins]

@felix91gr libSyntax is a library in the Swift compiler that provides a full source-preserving Syntax tree that can be easily transformed and re-printed as a file.

You can find more info about the C++ side of libSyntax here: https://github.com/apple/swift/tree/master/lib/Syntax

[felix91gr]

That’s awesome! Thank you for explaining 😊

[tiferrei]

So, would I be able to use this to get an AST from a file, almost like Clang's libTooling?

[harlanhaskins]

@tiferrei You'll be able to call Syntax.parse(file) and get a SourceFileSyntax that you can walk, transform, and re-print to a file if you want.

[nkcsgexi]

Looks great! some minor comments/question in line.

[nkcsgexi]

Do we need to assert the validity of index here?

[harlanhaskins]

Technically it'll fail in the stdlib but it wouldn't hurt to add a more descriptive error message here 😄

[nkcsgexi]

Could we add some doc-comment that this is the access path from the root to a specific node?

[harlanhaskins]

👍

[nkcsgexi]

Thinking more about this, in our design, should the meaning of the identifier be transparent to the end users? Do you think clients will misuse them?

[harlanhaskins]

This should be an implementation detail and not exposed to clients.

[nkcsgexi]

That's good. Thanks for explaining!

[nkcsgexi]

What does this if statement do?

[harlanhaskins]

(this should go in a comment above this if statement)

If the token doesn't have an exact text specified, then the case will have a single associated value of type String. This is for identifier, spacedBinaryOperator, unspacedBinaryOperator, prefixOperator, postfixOperator, integerLiteral, floatingLiteral, and stringLiteral

[nkcsgexi]

Can we gyb over TriviaPiece to shorten these above two switch statements?

[harlanhaskins]

I'm wary of gybing Trivia just because it's a static set of values and we don't anticipate it changing.

[nkcsgexi]

hmm, i think gybing reduces logic duplication and avoids some errors introduced by copy-n-paste.

[harlanhaskins]

That's fair! I can gyb this, no problem.

[benlangmuir]

You could use a method that takes the key+count if you think it's worth factoring this. I don't think gyb'ing is the right answer for this. Gyb adds a large cognitive burden to any code that uses it, so it has to have a big benefit to be worth it IMO.

[nkcsgexi]

For simple logic like this, I don't think gybing will harm readability and it makes sure we cover all cases.

[benlangmuir]

There is no default in this switch, so the language already ensures we cover all cases.

[nkcsgexi]

Conceptually, I agree that gyb can be hard to read than actual code; but based on the context here, also since we are using gyb already for other enums in this patch, i still prefer we generate the switch to be consistent. Readability won't suffer because of its simple logic.

[nkcsgexi]

Can we generate these factory methods too?

[benlangmuir]

Similarly, I disagree with gyb'ing this. It's much easier to read as plain source code and the benefit is very small.

[nkcsgexi]

Using gyb here doesn't harm readability either. The logic is straightforward.

[moiseev]

What's the outcome of the validation? Should it throw or return error if validation fails?

[harlanhaskins]

This is just debug validation to ensure that the things generated from SyntaxFactory/SyntaxBuilders in the tests hold the right structure. This validation doesn't happen in Release builds because the only way to construct one of these is through the Factory and Builders, and they're meant to guarantee you a valid structure.

[moiseev]

This sounds like an Equatable conformance. Doesn't it?

[harlanhaskins]

Ah! Yeah, I had originally structured this with protocols, and didn't want to enforce Equatable conformance because of the Self requirement, but since this is a class hierarchy I can make this a conformance.

[moiseev]

Would this work?

extension Syntax : Equatable {
  public static func == (lhs: Self, rhs: Self) -> Bool {
    return lhs.data === rhs.data
  }
}

[benlangmuir]

Overall looking great, thanks Harlan!

[benlangmuir]

Is this trying to say that .missing does not imply "implicit", but instead might be a required syntax element that was omitted?

[harlanhaskins]

Yep, I can clarify the comment a bit.

[benlangmuir]

I'm curious why not a random access collection?

[felix91gr]

Not a random access collection would be something like a Stack or a Queue, where you can only work on a specific region of the Data Structure. Contrasting with an Array or a List in that regard.

[benlangmuir]

D'oh, edited my comment: I was trying to ask why this is not a random access collection, since it seems like the underlying abstraction would support it. That said, I'm really just curious, no need to change it.

[harlanhaskins]

The underlying abstraction totally does support it. It's currently bound to Collection but I'll conform it to RandomAccessCollection instead.

[benlangmuir]

Can we end up in a situation where the parent goes away without the syntax data being "invalid" in some send, or could this be unowned?

[harlanhaskins]

parent is optional -- nodes that are the root node have no parent. We can't have an unowned optional ☹️

[benlangmuir]

Not sure how often you'd have more than one backtick in a row in source code, but I guess the Int is free in this representation 😉

[moiseev]

See TextOutputStreamable protocol.

[harlanhaskins]

That's absolutely what I was missing here. Thanks!

[moiseev]

Oh, you don't want to do this ;-) (I looked inside AnyIterator and I saw things in there). Writing a special Iterator struct for SyntaxChildren is just a few more lines of code, but avoids an unnecessary allocation.

[moiseev]

Based on the methods in the class itself, it can also conform (I guess) to MutableCollection and RangeReplaceableCollection and potentially acquire a more familiar API for things like removingLast etc...

[harlanhaskins]

It can't conform to MutableCollection because the nodes themselves are immutable.

[harlanhaskins]

@moiseev This shouldn't live in stdlib/internal/ -- where should it live? Also I need to figure out what CMake flags this needs. Currently it's using the same flags as SwiftExperimental.

[moiseev]

/cc @zisko for the CMake question above.
As for the location, let's leave it where it is and move if the need arises (or we come up with a better location for it).

[zisko]

At the place it is now, the CMake LGTM.

[harlanhaskins]

@swift-ci please smoke test

[shahmishal]

@swift-ci please python lint

[harlanhaskins]

@swift-ci please python lint

[shahmishal]

@swift-ci please python lint

[harlanhaskins]

@swift-ci please smoke test

[fbartho]

Typo: noe -> node

[harlanhaskins]

Thanks!

[harlanhaskins]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 4a8b360

[harlanhaskins]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 4a8b360

[harlanhaskins]

@swift-ci please test

[harlanhaskins]

Not sure what happened with that last failure.

[harlanhaskins]

@nkcsgexi @benlangmuir @moiseev Good to merge?

[nkcsgexi]

I've no more comments! LGTM!

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - fd45799

[benlangmuir]

👍

[harlanhaskins]

That "Build failed" comment...doesn't make sense? It actually passed. 🤔

[harlanhaskins]

⛴