[ASTGen] Infrastructure for implementing ASTGen incrementally #69761

rintaro · 2023-11-09T21:38:29Z

ASTGenVisitor has a reference to a legacy C++ Parser configured for ASTGen.
When ASTGenVisitor encounters a AST node that hasn't been migrated, call the C++ Parser::parse(Decl|Stmt|Expr|Type) to parse the position using the legacy parser.
If the legacy parser is configured for ASTGen, it calls ASTGen's swift_ASTGen_build(Decl|Stmt|Expr|Type) for each ASTNode parsing (unless the call is not directly from the ASTGen.)

rdar://117151886

include/swift/Parse/Parser.h

ahoppen · 2023-11-09T23:04:02Z

include/swift/Parse/Parser.h

+  //===--------------------------------------------------------------------===//
+  // ASTGen support.
+
+  ParserResult<TypeRepr> parseTypeReprFromSyntaxTree();


Just reading the function declaration, I’m thinking: Which syntax tree is this parsing from? I think a doc comment could help explain this (or a need a better name).

lib/ASTGen/Sources/ASTGen/Exprs.swift

lib/DriverTool/swift_parse_test_main.cpp

lib/ASTGen/Sources/ASTGen/LegacyParse.swift

lib/Parse/ParseBridging.cpp

rintaro · 2023-11-10T16:51:55Z

lib/ASTGen/Sources/ASTGen/Exprs.swift

+      break
+    case // Known unimplemented kinds.
+        .arrayExpr, .arrowExpr, .asExpr, .assignmentExpr, .awaitExpr,
+        .binaryOperatorExpr, .booleanLiteralExpr, .borrowExpr, .canImportExpr,


Literals are already implemented. I didn't notice that they were in Literals.swift.

When 'wantOutermost' is true, traverse the parent as long as the position is the same as the target position.

rintaro · 2023-11-10T21:17:30Z

@swift-ci Please smoke test

rintaro · 2023-11-10T23:23:13Z

@swift-ci Please smoke test

rintaro · 2023-11-10T23:23:34Z

preset=asan
@swift-ci Please test with preset macOS platform

rintaro · 2023-11-11T00:03:55Z

@swift-ci Please smoke test

rintaro · 2023-11-11T00:04:01Z

preset=asan
@swift-ci Please test with preset macOS platform

DougGregor · 2023-11-13T04:55:26Z

include/swift/Parse/Parser.h

+
+  /// Parse a TypeRepr from the syntax tree. i.e. SF->getExpxortedSourceFile()
+  ParserResult<TypeRepr> parseTypeReprFromSyntaxTree();
+  /// Parse a Stmt from the syntax tree. i.e. SF->getExpxortedSourceFile()


Typo "Expxorted" in all of these.

DougGregor · 2023-11-13T04:57:09Z

lib/ASTGen/Sources/ASTGen/ASTGen.swift

-  }
+  else {
+    // FIXME: Produce an error
+    return nil


It might be worth putting in some stub error sooner rather than later, so this doesn't cause unwanted surprises.

Currently findSyntaxNodeInSourceFile() prints an error message to stdout, which is not great, but at least it prevents "unwanted surprises". But yeah, I think we should add a diagnostic on the diagnostic engine.
Though since this PR is getting big, and to unblock some follow-up patches I have, I'd like to do it in the followups.
Also, I'm thinking to change findSyntaxNodeInSourceFile() itself. #69724 (comment)

DougGregor · 2023-11-13T05:05:55Z

I like this approach of bouncing between the two parsers as needed, so we can opt in to ASTGen production-by-production.

* 'ASTGenVisitor' has a reference to a legacy C++ Parser configured for ASTGen. * If 'ASTGenVisitor' encounters a AST node that hasn't been migrated, call parse(Decl|Stmt|Expr|Type) to parse the position using the legacy parser. * The legacy parser calls ASTGen's 'swift_ASTGen_build(Decl|Stmt|Expr|Type)' for each ASTNode "parsing" (unless the call is not directly from the ASTGen.) rdar://117151886

There's no reason to generate only TypeRepr using ASTGen anymore. Use ParserASTGen feature to test test/ASTGen/types.swift because ASTGen now can generate the whole test file for type checking.

Apparently C++ method bridging doesn't work in Linux?

rintaro · 2023-11-13T05:44:02Z

@swift-ci Please smoke test

rintaro · 2023-11-13T05:50:19Z

preset=asan
@swift-ci Please test with preset macOS platform

rintaro · 2023-11-13T07:00:57Z

test/ASTGen/types.swift


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


It is really unfortunate that this test hits ASAN failure too. We're actively looking into this issue.
Anyway I confirmed these tests passes locally.

Should we mark it UNSUPPORTED: asan for now so we at least don't lose the test coverage?

bnbarham · 2023-11-13T20:43:38Z

lib/DriverTool/swift_parse_test_main.cpp

@@ -113,6 +134,67 @@ struct SwiftParserExecutor {
  }
 };

+static void appendToVector(void *declPtr, void *vecPtr) {


Is this used? Only reference I see is in ParseDecl.cpp

Ah, it's a leftover. I'm removing it in a followup

hamishknight

Cool!

hamishknight · 2023-11-14T13:19:28Z

lib/ASTGen/Sources/ASTGen/ASTGen.swift

+) -> UnsafeMutableRawPointer? {
+  let sourceFile = sourceFilePtr.assumingMemoryBound(to: ExportedSourceFile.self)
+
+  // Find the type syntax node.


Nit: Not necessarily a type syntax any more, right?

The field `IsForASTGen` only exists when `SWIFT_BUILD_SWIFT_SYNTAX` is defined, but the usage was unprotected. The whole function can be inside the protected block because it only seems to be invoked from inside blocks already protected by `SWIFT_BUILD_SWIFT_SYNTAX`. Fix for swiftlang#69761

…69858) The field `IsForASTGen` only exists when `SWIFT_BUILD_SWIFT_SYNTAX` is defined, but the usage was unprotected. The whole function can be inside the protected block because it only seems to be invoked from inside blocks already protected by `SWIFT_BUILD_SWIFT_SYNTAX`. Fix for #69761

rintaro force-pushed the astgen-incremental-develop branch 3 times, most recently from c15feed to b423363 Compare November 9, 2023 23:18

ahoppen reviewed Nov 9, 2023

View reviewed changes

rintaro commented Nov 10, 2023

View reviewed changes

[ASTGen] Generalize findSyntaxNodeInSourceFile(wantOutermost:true)

2c8d7f2

When 'wantOutermost' is true, traverse the parent as long as the position is the same as the target position.

rintaro force-pushed the astgen-incremental-develop branch from 0323409 to f333245 Compare November 10, 2023 21:17

rintaro force-pushed the astgen-incremental-develop branch from f333245 to 5f7c020 Compare November 10, 2023 23:22

rintaro force-pushed the astgen-incremental-develop branch from 5f7c020 to 253fbb9 Compare November 11, 2023 00:03

DougGregor reviewed Nov 13, 2023

View reviewed changes

rintaro added 4 commits November 12, 2023 21:43

[ASTGen] Remove ASTGenTypes experimental feature

f3f4b19

There's no reason to generate only TypeRepr using ASTGen anymore. Use ParserASTGen feature to test test/ASTGen/types.swift because ASTGen now can generate the whole test file for type checking.

[ASTGen] Use C briding instead of C++ method

7c195ef

Apparently C++ method bridging doesn't work in Linux?

[ASTGen] Disable a test for the same ASAN failure as other ASTGen test

d51058c

rintaro force-pushed the astgen-incremental-develop branch from fe4ba34 to d51058c Compare November 13, 2023 05:43

rintaro marked this pull request as ready for review November 13, 2023 05:51

rintaro requested review from zoecarver, CodaFi, bnbarham, hborla, slavapestov and xedin as code owners November 13, 2023 05:51

rintaro requested a review from tshortli as a code owner November 13, 2023 05:51

rintaro requested review from DougGregor, ahoppen and hamishknight and removed request for xedin, slavapestov, CodaFi, hborla and tshortli November 13, 2023 05:51

rintaro commented Nov 13, 2023

View reviewed changes

ahoppen approved these changes Nov 13, 2023

View reviewed changes

bnbarham reviewed Nov 13, 2023

View reviewed changes

rintaro merged commit b399173 into swiftlang:main Nov 13, 2023

rintaro mentioned this pull request Nov 14, 2023

[ASTGen] Generalize findSyntaxNodeInSourceFile(wantOutermost:true) #69724

Closed

hamishknight reviewed Nov 14, 2023

View reviewed changes

drodriguez mentioned this pull request Nov 14, 2023

[ASTGen] Protect parseASTFromSyntaxTree for SWIFT_BUILD_SWIFT_SYNTAX #69858

Merged

rintaro mentioned this pull request Nov 15, 2023

[Parse] Unguard Parser.IsForASTGen for SWIFT_BUILD_SWIFT_SYNTAX #69877

Merged

[ASTGen] Infrastructure for implementing ASTGen incrementally #69761

[ASTGen] Infrastructure for implementing ASTGen incrementally #69761

Uh oh!

Conversation

rintaro commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rintaro Nov 10, 2023 • 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!

rintaro commented Nov 10, 2023

Uh oh!

rintaro commented Nov 10, 2023

Uh oh!

rintaro commented Nov 10, 2023

Uh oh!

rintaro commented Nov 11, 2023

Uh oh!

rintaro commented Nov 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Nov 13, 2023 • 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!

DougGregor commented Nov 13, 2023

Uh oh!

rintaro commented Nov 13, 2023

Uh oh!

rintaro commented Nov 13, 2023

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!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro commented Nov 9, 2023 •

edited

Loading

rintaro Nov 10, 2023 •

edited

Loading

rintaro Nov 13, 2023 •

edited

Loading