ASTGen: An assortment of strides and improvements #67111

AnthonyLatsis · 2023-07-04T12:03:40Z

This pull request focuses on declaration translation, extrapolating preparatory work to the rest of the module for consistency and testing.

AnthonyLatsis · 2023-07-04T12:23:01Z

test/ASTGen/verify-parse.swift

+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend %s -dump-parse -disable-availability-checking -enable-experimental-feature ParserASTGen > %t/astgen.ast
+// RUN: %target-swift-frontend %s -dump-parse -disable-availability-checking > %t/cpp-parser.ast
+// RUN: %diff -u %t/astgen.ast %t/cpp-parser.ast


This is of course ersatz diffing — we ought to implement an AST matcher.

We should, but it's a big task and the approach you're using here can provide real value along the way.

slavapestov · 2023-07-04T13:22:35Z

Nice!

AnthonyLatsis · 2023-07-04T16:13:01Z

@swift-ci please smoke test

AnthonyLatsis · 2023-07-05T14:13:30Z

@swift-ci please smoke test macOS

bnbarham

Thanks @AnthonyLatsis. Big fan of your operator test choices :)

lib/ASTGen/Sources/ASTGen/Misc.swift

lib/ASTGen/Sources/ASTGen/Generics.swift

include/swift/AST/CASTBridging.h

bnbarham · 2023-07-05T05:29:15Z

test/ASTGen/verify-parse.swift


 // REQUIRES: executable_test

 // -enable-experimental-feature requires an asserts build
 // REQUIRES: asserts

-func test1(x: Int, fn: (Int) -> Int) -> Int {
-  let xx = fn(42)
+// NB: Ridiculous formatting to test that we do not include leading trivia in locations.


Could use inline comments instead , I'm not sure which is more readable though to be honest.

include/swift/AST/CASTBridging.h

lib/AST/Decl.cpp

DougGregor

This is really quite awesome! A few minor comments here and there, but I love that you've increased the coverage of ASTGen so far.

It would be fun to follow this up with the infrastructure to redirect the C++ parser over to ASTGen for specific declaration kinds (e.g., import decls, operators, other small stuff), like I did in #66033, which lets you run a whole bunch of code through a narrow slice of ASTGen.

lib/AST/ASTDumper.cpp

lib/ASTGen/Sources/ASTGen/Decls.swift

DougGregor · 2023-07-05T21:33:43Z

lib/ASTGen/Sources/ASTGen/Decls.swift

+    case .keyword(.none): self = .none
+    case .keyword(.left): self = .left
+    case .keyword(.right): self = .right
+    default: fatalError("Unknown precedence group associativity value")


Should this produce a diagnostic rather than failing? The incoming syntax tree could be ill-formed.

Do we not expect a SwiftParserDiagnostics pass to catch this sort of thing? Why do we want to allow for translation of ill-formed syntax trees?

ASTGen should handle any swift-syntax tree, whether from the parser or not. There's a few cases that currently have ! that really should not as well. @ahoppen as I know he has strong opinions on this :)

I see, so what we want is to simply emit a generic error about the syntax tree being invalid, I suppose? We don’t seem to currently have a way to bail out without constructing a node either.

ASTGen should handle any swift-syntax tree, whether from the parser or not. There's a few cases that currently have ! that really should not as well.

Does SwiftSyntax have a way to verify nodes? We could have an interceptor or verifier pre-pass rather than having to recover from every corner where the model is too permissive in the middle of translation.

Edit: I just thought about this a little bit more and it’s hard to model the groupAttributes with an optional child for each of the attribute kinds because we don’t enforce an order of the group attributes. I.e. we need to be able to represent both

precedencegroup T { assignment: true higherThan: AdditionPrecedence }

and

precedencegroup T { higherThan: AdditionPrecedence assignment: true }

And that basically requires a SyntaxCollection, unfortunately.

Ah, right. How ugly would it be to not actually enforce their order in the layout? Say, the initializer always inserts the attributes per the parameter order, but one could pass nils in the initializer and then insert the attributes in the appropriate order via the setters if necessary. The getters and setters would then work with a specific subrange of the layout of size 4 which is nilled out by default.

I’m not entirely sure what you mean. Do you have a short example of how you think the API should look like?

An optional child for each attribute kind as originally suggested, with accessors similar to these:

public var higherThanRelation: PrecedenceGroupRelationSyntax? { get { for index in 11...14 { guard let childData = data.child(at: index) else { // No data for this attribute (attributes are stored contiguously, i.e., no 'nil' data in between attributes). break } if let value = PrecedenceGroupRelationSyntax(Syntax(childData)) { return value } } return nil } set(value) { for index in 11...14 { guard let rawChild = data.rawChild(at: index) else { // No data for this attribute (attributes are stored contiguously, i.e., no 'nil' data in between attributes). // Stick it in at this index. self = PrecedenceGroupDeclSyntax(data.replacingChild(at: index, with: value?.data, arena: SyntaxArena())) return } if rawChild.kind == .precedenceGroupRelation { if let value { // Simply replace the data at this index. self = PrecedenceGroupDeclSyntax(data.replacingChild(at: index, with: value.data, arena: SyntaxArena())) return } else { self = PrecedenceGroupDeclSyntax(data.replacingChild(at: index, with: nil, andShiftingItTo: 14, arena: SyntaxArena())) } } } } } }

Another possibility is to statically feature the 4 optional child attributes in PrecedenceGroupAttributeListSyntax, as they would have been in PredenceGroupDeclSyntax, perhaps retaining the conformance to SyntaxCollection. This way no changes to PredenceGroupDeclSyntax are necessary.

An optional child for each attribute kind as originally suggested, with accessors similar to these.

The issue I see with this design is that a client of SwiftSyntax might create a PrecedenceGroupDeclSyntax that has two higherThan relations in the groupAttributes and the higherThanRelation getter would only return the first.

Similarly, if you set a higherThan relation using this setter, you can’t control where it is being inserted if no higherThan relation exists yet. So

let pg = PrecedenceGroupDeclSyntax() pg.higherThanRelation = // … pg.assignment = // ...

would give you a different node than

let pg = PrecedenceGroupDeclSyntax() pg.assignment = // ... pg.higherThanRelation = // …

and that’s quite surprising IMO.

DougGregor · 2023-07-05T21:36:07Z

lib/ASTGen/Sources/ASTGen/Decls.swift

+  func visit(_ node: PrecedenceGroupDeclSyntax) -> ASTNode {
+    let (name, nameLoc) = self.bridgedIdentifierAndSourceLoc(for: node.identifier)
+
+    struct PrecedenceGroupBody {


For precedence groups and operators, I wonder if we should instead build on top of the SwiftOperators library, which has a mapping from the syntax tree into a more semantic representation (Operator and PrecedenceGroup).

Apparently the inits from SwiftOperators assume the nodes are valid, so we would have to do most of the same work in ASTGen to prevalidate the nodes and diagnose issues before calling these inits, like switching on the fixity of an operator or iterating over the attributes of a precedence group. Do you believe there is something to gain from building on top of SwiftOperators regardless?

lib/ASTGen/Sources/ASTGen/Decls.swift

lib/ASTGen/Sources/ASTGen/Exprs.swift

lib/ASTGen/Sources/ASTGen/Literals.swift

ahoppen · 2023-07-07T18:03:13Z

Just a quick comment: I’d like to review the PR but won’t get to it until Monday/Tuesday. I’d appreciate if you could leave it open until then.

AnthonyLatsis · 2023-07-07T18:31:12Z

I’d like to review the PR but won’t get to it until Monday/Tuesday. I’d appreciate if you could leave it open until then.

No problem! I would hate to land this without a review from you.

ahoppen

Here’s my first round of review. You’re getting a bit of my complaints with the current ASTGen design but I think that I managed to keep my comments somewhat focused so that I don’t require you to do a massive refactoring first.

lib/AST/CASTBridging.cpp

lib/ASTGen/Sources/ASTGen/Misc.swift

lib/ASTGen/Sources/ASTGen/Decls.swift

lib/ASTGen/Sources/ASTGen/Exprs.swift

lib/ASTGen/Sources/ASTGen/ParameterClause.swift

ahoppen · 2023-07-10T14:06:16Z

lib/ASTGen/Sources/ASTGen/Types.swift

+        Name: name,
+        NameLoc: nameLoc,
+        SecondName: secondName,
+        SecondNameLoc: secondNameLoc,
+        UnderscoreLoc: nil, /*N.B. Only important for SIL*/
+        ColonLoc: self.bridgedSourceLoc(for: element.colon),
+        Type: type,
+        TrailingCommaLoc: self.bridgedSourceLoc(for: element.trailingComma)


Just me complaining, no action necessary for you in this PR: I know that you didn’t introduce BridgedTupleTypeElement but the members of it should really be lowercase so I don’t need to look at uppercase parameter labels in Swift.

AnthonyLatsis · 2023-09-06T04:11:01Z

@swift-ci please smoke test macOS

AnthonyLatsis · 2023-09-12T17:38:36Z

@swift-ci please smoke test

ahoppen · 2023-09-14T15:13:18Z

@swift-ci Please test Windows

AnthonyLatsis · 2023-09-15T03:32:10Z

@ahoppen We have a problem with swift_name on Windows. I thought we were using the just-built clang to compile the Swift compiler, but apparently not.

AnthonyLatsis · 2023-09-15T06:12:22Z

@swift-ci please test Windows

AnthonyLatsis · 2023-09-15T10:13:40Z

I take it the conditional def works because we skip the early swift-syntax on Windows. Is this an OK workaround?

bnbarham · 2023-09-15T17:45:59Z

I take it the conditional def works because we skip the early swift-syntax on Windows. Is this an OK workaround?

That's not going to be true very soon, but regardless the conditional def seems fine to me - it isn't needed when compiling C++, only when importing into Swift (in which case, it will be defined). Unless I'm misunderstanding something crucial here?

ahoppen · 2023-09-16T00:34:13Z

@swift-ci Please test

AnthonyLatsis · 2023-09-16T02:37:44Z

it isn't needed when compiling C++, only when importing into Swift (in which case, it will be defined). Unless I'm misunderstanding something crucial here?

Nah, actually, thank you, I had a brain fart. I just realized the importer invokes clang to parse headers (so of course the attribute will be defined when importing into Swift). For some reason I had this silly impression that it would be reading a normalized MSVC AST on Windows by default, even though I knew the importer reads clang ASTs 🙃

AnthonyLatsis · 2023-09-18T17:55:11Z

@ahoppen @DougGregor @bnbarham @hamishknight Thank you so much for weighing in!

bnbarham · 2023-09-18T18:10:55Z

Woo, it's in! Thanks for all the work here @AnthonyLatsis!

ahoppen · 2023-09-18T19:36:25Z

Thank you for doing all this work! 🙏🏽

AnthonyLatsis requested review from zoecarver, CodaFi, hborla, slavapestov, xedin, ahoppen, bnbarham, DougGregor and rintaro as code owners July 4, 2023 12:03

AnthonyLatsis commented Jul 4, 2023

View reviewed changes

AnthonyLatsis force-pushed the astgen-decls branch from 5f8c4e2 to fbc1f14 Compare July 4, 2023 21:37

This comment was marked as duplicate.

Sign in to view

AnthonyLatsis force-pushed the astgen-decls branch from fbc1f14 to a491701 Compare July 4, 2023 22:54

This comment was marked as duplicate.

Sign in to view

AnthonyLatsis force-pushed the astgen-decls branch from a491701 to 332b8a6 Compare July 5, 2023 14:13

bnbarham reviewed Jul 5, 2023

View reviewed changes

DougGregor reviewed Jul 5, 2023

View reviewed changes

ahoppen reviewed Jul 10, 2023

View reviewed changes

xedin removed their request for review July 10, 2023 17:06

AnthonyLatsis force-pushed the astgen-decls branch 2 times, most recently from b0741a2 to 942f06c Compare September 5, 2023 16:09

AnthonyLatsis requested review from DougGregor, ahoppen and bnbarham September 6, 2023 04:34

AnthonyLatsis added 8 commits September 12, 2023 20:37

ASTGen: Translate enum case declarations

fa1c9fd

ASTGen: Translate enum element payloads

76e2cdc

ASTGen: Translate extension declarations

de155c9

ASTGen: Translate operator declarations

96f13f5

ASTGen: Translate deinitializer declarations

58ce5d1

ASTGen: Translate initializer declarations

5e46243

ASTGen: Translate import declarations

a8db87a

ASTGen: Translate precedence group declarations

a1ba72c

AnthonyLatsis force-pushed the astgen-decls branch from 953a476 to 448712a Compare September 12, 2023 17:38

jakedves mentioned this pull request Sep 13, 2023

Add SWIFT_NAME attributes to all functions in CASTBridging.h #68492

Closed

AnthonyLatsis requested a review from rintaro September 14, 2023 01:19

This was referenced Sep 14, 2023

Make ASTGenVisitor not conform to SyntaxTransformVisitor #68350

Closed

Add a custom @_spi kind to expose BumpPtrAllocator from SwiftSyntax to ASTGen. #68351

Closed

[NFC] ASTGen: Add Swift names to bridging functions for decls

ae83632

AnthonyLatsis force-pushed the astgen-decls branch from 448712a to ae83632 Compare September 15, 2023 06:11

ahoppen merged commit 44a4d69 into swiftlang:main Sep 16, 2023

AnthonyLatsis deleted the astgen-decls branch September 16, 2023 21:27

bnbarham mentioned this pull request Oct 2, 2023

Determine if ASTGen is responsible for checking that optional tokens exist in the SwiftSyntax tree #68355

Open

ahoppen mentioned this pull request Nov 15, 2023

[ASTGen] Several improvements to generalize node handling #69894

Merged

ASTGen: An assortment of strides and improvements #67111

ASTGen: An assortment of strides and improvements #67111

Conversation

AnthonyLatsis commented Jul 4, 2023

AnthonyLatsis Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented Jul 4, 2023

AnthonyLatsis commented Jul 4, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

AnthonyLatsis commented Jul 5, 2023

bnbarham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLatsis Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLatsis Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLatsis Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen commented Jul 7, 2023

AnthonyLatsis commented Jul 7, 2023

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLatsis commented Sep 6, 2023

AnthonyLatsis commented Sep 12, 2023

ahoppen commented Sep 14, 2023

AnthonyLatsis commented Sep 15, 2023

AnthonyLatsis commented Sep 15, 2023

AnthonyLatsis commented Sep 15, 2023

bnbarham commented Sep 15, 2023

ahoppen commented Sep 16, 2023

AnthonyLatsis commented Sep 16, 2023 • edited Loading

AnthonyLatsis commented Sep 18, 2023

bnbarham commented Sep 18, 2023

ahoppen commented Sep 18, 2023

AnthonyLatsis Jul 4, 2023 •

edited

Loading

AnthonyLatsis Jul 5, 2023 •

edited

Loading

AnthonyLatsis Jul 7, 2023 •

edited

Loading

AnthonyLatsis Sep 6, 2023 •

edited

Loading

AnthonyLatsis commented Sep 16, 2023 •

edited

Loading