-
Notifications
You must be signed in to change notification settings - Fork 2k
unified: Switch over to using swift-syntax for parsing
#22233
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
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
7a573c4
yeast: Order AST dump fields by schema-declared order
tausbn 8206f6f
swift-syntax-rs: Fold local and stdlib operators
tausbn 2e74c1d
yeast: Desugar an externally-built AST, and validate by field name
tausbn bbf52ce
tree-sitter-extractor: Split direct and desugaring extractors
tausbn 79954ce
unified: Add the swift-syntax parser and unresolved operator sequence…
tausbn ad2ce93
unified: Port top-level, literal, and name rules to swift-syntax
tausbn 8328fba
unified: Port operator rules to swift-syntax
tausbn 437e482
unified: Port variable-binding rules to swift-syntax
tausbn 191fc54
unified: Port type-expression rules to swift-syntax
tausbn 52cdf59
unified: Port function, call, and member-access rules to swift-syntax
tausbn 6ad1fac
unified: Port closure rules to swift-syntax
tausbn 2de1549
unified: Port control-flow and pattern rules to swift-syntax
tausbn 2fcbc9b
unified: Port loop rules to swift-syntax
tausbn 4c764b5
unified: Port collection rules to swift-syntax
tausbn 6275977
unified: Port optional and error-handling rules to swift-syntax
tausbn 37654b4
unified: Port import rules to swift-syntax
tausbn 8e6651a
unified: Port type-container declarations to swift-syntax
tausbn ec0c49a
unified: Port property accessor rules to swift-syntax
tausbn 90ea1b5
unified: Port enum-case rules to swift-syntax
tausbn c4fbd74
unified: Port constructor and related declaration rules to swift-syntax
tausbn 5c5fd5e
unified: Switch the Swift front-end to swift-syntax
tausbn 20a2537
unified: Regenerate the raw-AST corpus section for swift-syntax
tausbn 046c88a
unified: Regenerate the enhanced getter/setter property corpus case
tausbn c50bcba
swift-syntax-rs: Degrade gracefully without a Swift toolchain
tausbn 17cbb4e
unified: Harden the external Swift parser integration
tausbn 7721ce2
unified: Add swift_node_types.yml to the extractor's compile_data
tausbn e3a0822
unified: Package the swift-syntax parser in the extractor pack
tausbn ba26e1d
unified: Add corpus tests for nested types
tausbn a3d9492
Merge branch 'main' into tausbn/swift-syntax-rs-sequenced
tausbn a2ff35a
unified: Fix Bazel formatting errors
tausbn ceffe40
unified: Remove references to Swift input schema generation
tausbn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| //! Swift front-end parser: shells out to the separate `swift-syntax-parse` | ||
| //! binary (which links swift-syntax) to obtain a JSON syntax tree, then adapts | ||
| //! that JSON into a `yeast::Ast` via the pure-Rust [`swift_adapter`] module. | ||
| //! | ||
| //! Running the parser in a separate process keeps the Swift toolchain out of | ||
| //! the extractor's own build: the extractor never links Swift, so working on | ||
| //! other (e.g. tree-sitter based) languages needs no Swift toolchain. Each call | ||
| //! spawns the parser afresh; a longer-lived parser process could be swapped in | ||
| //! behind this same seam later without touching the extraction pipeline. | ||
|
|
||
| use std::io::Write; | ||
| use std::process::{Command, Stdio}; | ||
|
|
||
| use codeql_extractor::extractor::ParsedTree; | ||
|
|
||
| use super::swift_adapter; | ||
|
|
||
| /// Environment variable naming the `swift-syntax-parse` executable. When unset, | ||
| /// `swift-syntax-parse` is looked up on `PATH`. | ||
| const PARSE_BIN_ENV: &str = "CODEQL_EXTRACTOR_UNIFIED_SWIFT_SYNTAX_PARSE"; | ||
|
|
||
| /// Parse Swift `source` into a [`ParsedTree`] (a raw `yeast::Ast` plus | ||
| /// side-channel `extra` tokens), ready to be desugared via `run_from_ast`. | ||
| pub fn parse(source: &[u8]) -> Result<ParsedTree, String> { | ||
| let source = | ||
| std::str::from_utf8(source).map_err(|e| format!("Swift source is not valid UTF-8: {e}"))?; | ||
| let json = run_parser(source)?; | ||
| let mut adapted = swift_adapter::json_to_ast(&json)?; | ||
| adapted.ast.set_source(source.as_bytes().to_vec()); | ||
| Ok(ParsedTree { | ||
| ast: adapted.ast, | ||
| extras: adapted.extras, | ||
| }) | ||
| } | ||
|
|
||
| /// The `swift-syntax-parse` executable to invoke. | ||
| fn parse_bin() -> String { | ||
| std::env::var(PARSE_BIN_ENV).unwrap_or_else(|_| "swift-syntax-parse".to_string()) | ||
|
tausbn marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /// Run the external parser, feeding `source` on stdin and returning its JSON | ||
| /// stdout. | ||
| fn run_parser(source: &str) -> Result<String, String> { | ||
| let bin = parse_bin(); | ||
| let mut child = Command::new(&bin) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .map_err(|e| format!("failed to spawn Swift parser `{bin}`: {e}"))?; | ||
|
|
||
| // The parser reads all of stdin before writing any stdout, so writing the | ||
| // whole source and then closing stdin (by dropping it) cannot deadlock. | ||
| child | ||
| .stdin | ||
| .take() | ||
| .expect("child stdin was piped") | ||
| .write_all(source.as_bytes()) | ||
| .map_err(|e| format!("failed to write source to Swift parser `{bin}`: {e}"))?; | ||
|
|
||
| let output = child | ||
| .wait_with_output() | ||
| .map_err(|e| format!("failed to run Swift parser `{bin}`: {e}"))?; | ||
| if !output.status.success() { | ||
| return Err(format!( | ||
| "Swift parser `{bin}` failed ({}): {}", | ||
| output.status, | ||
| String::from_utf8_lossy(&output.stderr).trim() | ||
| )); | ||
| } | ||
| String::from_utf8(output.stdout) | ||
| .map_err(|e| format!("Swift parser produced non-UTF-8 output: {e}")) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to suggest that we will only ever have a single unified extractor that then links to (or calls) a plethora of parsers. Is that a worthwhile complexity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also given our internal discussion regarding linking, I wonder whether this makes sense at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed like the easiest solution in the short term, but I don't think it implies that we want all parsers to follow this approach. In particular, for the ones based on
tree-sitter, I think it would be nicer to just invoke that parser directly from Rust.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would linking it in directly be more difficult?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will be more difficult, necessarily, but my worry was that this would put us in a situation where building the unified extractor -- even if you wanted to work on something completely different (e.g. if we convert the Python analysis to use commonAST) -- would result in building the Swift parser (which can be rather slow).
So, to me the cleanest way to enforce the separation seemed to be to just have it completely external, as a separate binary.
Though, having said that I now realise that -- with the current setup -- building the unified extractor still invokes the Swift extractor if the necessary toolchain is present, so I didn't really succeed in this goal.
I'm honestly not sure what the best solution is here. For the short term, it doesn't really matter, since we're only targeting a single language. Once we go to support multiple languages, we may want several build targets, one for each supported language and one for all of them at the same time...
I have no strong feelings about the present solution -- we can easily switch it out for some other approach. However, that is perhaps best left as work in a follow-up PR, as this one is already quite hefty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again you seem to be suggesting that we will have one glorious unified parser that combines the extraction for a multitude of languages. If we step away from that, then these problems all go away, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption was that this was the architecture we were aiming for, cf. the fact that
swift.rslives in alanguages/swiftdirectory.However, you are right that we could also recast this as a Swift-only extractor (at least, that's how I read your message). In that case, linking directly is probably better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so we have/had different expectations here. My assumption was that in the end we were still building a separate extractor per language that we would support, based on the underlying shared extractor and yeast code that we have in place. No matter how things are currently laid out.
With one glorious unified extractor, I have issues seeing how this would work if a user would only want to do an extraction for one of the supported languages. This is probably something we need to have a deeper think about.