Adopt the Swift Parser #395
Conversation
There was a problem hiding this comment.
Ah, I wish we had synced up! I have a branch locally that was doing the same update but I was waiting for swiftlang/swift-syntax#619 to be merged so I could finish it.
I like your changes to TokenStreamCreator better than mine though, so let's keep this one. Just a couple questions about handling some cases that SwiftParser does differently.
| @@ -54,8 +54,7 @@ public final class SwiftFormatter { | |||
| /// - Throws: If an unrecoverable error occurs when formatting the code. | |||
| public func format<Output: TextOutputStream>( | |||
| contentsOf url: URL, | |||
| to outputStream: inout Output, | |||
| parsingDiagnosticHandler: ((Diagnostic) -> Void)? = nil | |||
There was a problem hiding this comment.
I have mixed feelings about dropping this entirely. For the overload that takes a SourceFileSyntax, it makes sense to assume that the user has run the diagnostic-generating visitor over the tree first if they're interested in those. But the overloads that take source text or a URL are a nice convenience and I don't think we want them to silently drop diagnostics.
One idea I had was to change the callback to take the syntax tree instead and the caller could get the diagnostics that way, but by that point it's more natural for the user to just parse the file themselves and run the visitor. What I was doing in my branch was to update the callback to take the Diagnostic and SourceLocation and then run the visitor here to send that information back.
WDYT?
There was a problem hiding this comment.
I agree, this is definitely a convenient spot to run the diagnostic post-pass. I'll try to restore this.
| final class AsExprTests: PrettyPrintTestCase { | ||
| func testWithoutPunctuation() { | ||
| func testWithoutPunctuation() throws { | ||
| throw XCTSkip("As expression grouping does not account for new sequence expression structure.") |
There was a problem hiding this comment.
Let's wait for swiftlang/swift-syntax#619 to be merged so that we can handle all of these properly without major formatting regressions.
There was a problem hiding this comment.
To be clear, this has already regressed regardless of parsing backend as of swiftlang/swift@ed34dfc
There was a problem hiding this comment.
Yes, you're right. We could fix that by lifting swift-format's operating folding implementation into SwiftFormatCore instead of SwiftFormatPrettyPrint but I don't think it's worth the churn since swiftlang/swift-syntax#619 is on the horizon.
Ok, once you've made the change you mentioned in the other comment thread, we can land this and then do follow-ups.
There was a problem hiding this comment.
Very much agreed. Excited to be able to have a common operator folding library available for y'all. Thanks Tony.
The idea being that the token stream cares about seeing the entire structure of the document, missing nodes and all, because it can try to format around the missing parts. Additionally, the structure of the missing nodes provides a place to hang opening and closing breaks in the formatting stream for now. A lot of the tests were relying on the C++ parser skipping large regions of syntax with unknown syntax nodes - which the stream would render back verbatim. The printing parts have been updated so they do not print missing tokens. Note that if the formatter wishes to heal invalid syntax, it can do so by simply printing missing keyword-like tokens into the stream, and replacing missing identifier-like tokens with code completion placeholders.
The new parser will attach the unexpected text here as trailing trivia. Update this rule to account for that.
The new parser also fixes a regression in this test and diagnoses
() -> (\u{feff})
without replacing it
The new parser no longer instantiates UnknownSyntax nodes, so this test isn't actually ever going to catch anything.
These tests were failing against with the SwiftSyntaxParser on mainline too because they need to adapt to the new Unresolved* components of the sequence expressions produced by both parsers.
The new Swift Parser does not emit diagnostics on its own. Schedule the diagnostic pass in all of the places SwiftFormat provides a diagnostic hook.
CodaFi
force-pushed
the
parsely-sage-rosemary-and-thyme
branch
from
fc52e09 to
ac8064f
Compare
|
Thank you! The new parser makes swift-format development immeasurably more approachable. 🎉 |
Adopt the new Swift parser in swift-format
This would otherwise be a fairly straightfoward patchset, but there's some existing tests that are failing on mainline that I've gone ahead and inserted explicit skips for. Tests that are expecting "unknown nodes" have also been skipped because the new parser never produces them. Finally, this patchset actually resolves a regression in mainline by properly attaching unexpected trivia text to the correct side of the type AST for
(\u{feff}), so we recover a missing diagnostic inReturnVoidInsteadOfEmptyTupleTests.