Skip to content

[Parser] Decouple the parser from AST creation (part 1) #25193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 8, 2019
Merged

[Parser] Decouple the parser from AST creation (part 1) #25193

merged 2 commits into from
Jul 8, 2019

Conversation

jansvoboda11
Copy link
Contributor

@jansvoboda11 jansvoboda11 commented Jun 1, 2019

Instead of creating the AST directly in the parser (and libSyntax or SwiftSyntax via SyntaxParsingContext), make Parser to explicitly create a tree of ParsedSyntaxNodes. Their OpaqueSyntaxNodes can be either libSyntax or SwiftSyntax. If AST is needed, it can be generated from the libSyntax tree.
This PR contains the infrastructure to achieve the goal in an incremental way and also refactored literal parsing.

This PR is a part of a Google Summer of Code 2019 project.

@rintaro rintaro self-assigned this Jun 1, 2019
Instead of creating the AST directly in the parser (and libSyntax or
SwiftSyntax via SyntaxParsingContext), make Parser to explicitly create
a tree of ParsedSyntaxNodes. Their OpaqueSyntaxNodes can be either
libSyntax or SwiftSyntax. If AST is needed, it can be generated from the
libSyntax tree.
@rintaro
Copy link
Member

rintaro commented Jul 2, 2019

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Jul 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - 44d7769

@akyrtzi
Copy link
Contributor

akyrtzi commented Jul 8, 2019

@swift-ci Please test

@swiftlang swiftlang deleted a comment from swift-ci Jul 8, 2019
@swiftlang swiftlang deleted a comment from swift-ci Jul 8, 2019
@akyrtzi akyrtzi merged commit d0ddece into swiftlang:master Jul 8, 2019
@beccadax
Copy link
Contributor

beccadax commented Jul 9, 2019

@jansvoboda11 Unfortunately, I had to revert this change because a few of our CI bots had trouble building after it was merged.

From what I can see, the problem is that Parser.h now includes ParsedSyntaxNodes.h; this is a generated header and it looks like it doesn't always get generated before Parser.h is needed. You can test this locally by removing the build/[configuration]/swift-macosx-x86_64/ directory between build-script runs. To fix this, you might be able to avoid including ParsedSyntaxNodes.h in Parser.h; if you can't, you'll need to tweak the CMake configuration to generate ParsedSyntaxNodes.h earlier. Once you've finished this work, @akyrtzi or another committer can use "clean smoke test" instead of just "smoke test" to check that it worked.

I also noticed a leaks test run with failures that may stem from this PR. I don't think there's a way to ask the CI bot to run a leaks test, but you can use --enable-lsan with the usual options like -t to run one locally.

@jansvoboda11
Copy link
Contributor Author

Thanks for the heads-up @brentdax, I'll look into it ASAP.

@davezarzycki
Copy link
Contributor

In my experience with unified builds, the project CMake files are already brittle in this regard. I think the following needs to be added in a few more CMakeLists.txt files but I haven't looked into it thoroughly:

# Add generated libSyntax headers to global dependencies.
list(APPEND LLVM_COMMON_DEPENDS swift-syntax-generated-headers)
list(APPEND LLVM_COMMON_DEPENDS swift-parse-syntax-generated-headers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants