Skip to content

Adopt the Swift Parser #395

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.

Sign up for GitHub

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 6 commits into from
Sep 13, 2022
Merged

Adopt the Swift Parser #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe193a9
Adopt the Swift Parser
CodaFi Sep 10, 2022
b0adacc
Migrate the TokenStreamCreator to the "all" View Mode
CodaFi Sep 10, 2022
1e43154
Update ReturnVoidInsteadOfEmptyTuple Rule
CodaFi Sep 10, 2022
5214b42
Remove Invalid Syntax Tests
CodaFi Sep 10, 2022
a409bd6
Explicitly XFAIL Currently Failing Tests
CodaFi Sep 10, 2022
ac8064f
Migrate Diagnostics to Explicitly-Scheduled Pass
CodaFi Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ let package = Package(
"SwiftFormatRules",
"SwiftFormatWhitespaceLinter",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.target(
Expand Down Expand Up @@ -89,7 +89,7 @@ let package = Package(
"SwiftFormatCore",
"SwiftFormatRules",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.executableTarget(
Expand All @@ -100,6 +100,7 @@ let package = Package(
"SwiftFormatCore",
.product(name: "ArgumentParser", package: "swift-argument-parser"),
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
.product(name: "TSCBasic", package: "swift-tools-support-core"),
]
),
Expand All @@ -109,7 +110,7 @@ let package = Package(
dependencies: [
"SwiftFormat",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.testTarget(
Expand All @@ -122,7 +123,7 @@ let package = Package(
"SwiftFormatConfiguration",
"SwiftFormatCore",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.testTarget(
Expand All @@ -131,7 +132,7 @@ let package = Package(
"SwiftFormatTestSupport",
"SwiftFormatWhitespaceLinter",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.testTarget(
Expand All @@ -143,7 +144,7 @@ let package = Package(
"SwiftFormatRules",
"SwiftFormatTestSupport",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.testTarget(
Expand All @@ -155,7 +156,7 @@ let package = Package(
"SwiftFormatRules",
"SwiftFormatTestSupport",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
.testTarget(
Expand All @@ -166,7 +167,7 @@ let package = Package(
"SwiftFormatTestSupport",
"SwiftFormatWhitespaceLinter",
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxParser", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
]
),
]
Expand All @@ -182,7 +183,7 @@ if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil {
branch: "main"
),
.package(
url: "https://github.com/apple/swift-syntax",
url: "https://github.com/apple/swift-syntax.git",
branch: "main"
),
.package(
Expand Down
28 changes: 22 additions & 6 deletions Sources/SwiftFormat/SwiftFormatter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import SwiftFormatCore
import SwiftFormatPrettyPrint
import SwiftFormatRules
import SwiftSyntax
import SwiftSyntaxParser
import SwiftParser
import SwiftDiagnostics

/// Formats Swift source code or syntax trees according to the Swift style guidelines.
public final class SwiftFormatter {
Expand Down Expand Up @@ -55,7 +56,7 @@ public final class SwiftFormatter {
public func format<Output: TextOutputStream>(
contentsOf url: URL,
to outputStream: inout Output,
parsingDiagnosticHandler: ((Diagnostic) -> Void)? = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is definitely a convenient spot to run the diagnostic post-pass. I'll try to restore this.

parsingDiagnosticHandler: ((Diagnostic, SourceLocation) -> Void)? = nil
) throws {
guard FileManager.default.isReadableFile(atPath: url.path) else {
throw SwiftFormatError.fileNotReadable
Expand All @@ -64,8 +65,16 @@ public final class SwiftFormatter {
if FileManager.default.fileExists(atPath: url.path, isDirectory: &isDir), isDir.boolValue {
throw SwiftFormatError.isDirectory
}
let sourceFile = try SyntaxParser.parse(url, diagnosticHandler: parsingDiagnosticHandler)
let source = try String(contentsOf: url, encoding: .utf8)
let sourceFile = try Parser.parse(source: source)
if let parsingDiagnosticHandler = parsingDiagnosticHandler {
let expectedConverter = SourceLocationConverter(file: url.path, tree: sourceFile)
let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: sourceFile)
for diagnostic in diagnostics {
let location = diagnostic.location(converter: expectedConverter)
parsingDiagnosticHandler(diagnostic, location)
}
}
try format(syntax: sourceFile, assumingFileURL: url, source: source, to: &outputStream)
}

Expand All @@ -85,10 +94,17 @@ public final class SwiftFormatter {
source: String,
assumingFileURL url: URL?,
to outputStream: inout Output,
parsingDiagnosticHandler: ((Diagnostic) -> Void)? = nil
parsingDiagnosticHandler: ((Diagnostic, SourceLocation) -> Void)? = nil
) throws {
let sourceFile =
try SyntaxParser.parse(source: source, diagnosticHandler: parsingDiagnosticHandler)
let sourceFile = try Parser.parse(source: source)
if let parsingDiagnosticHandler = parsingDiagnosticHandler {
let expectedConverter = SourceLocationConverter(file: url?.path ?? "<unknown>", tree: sourceFile)
let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: sourceFile)
for diagnostic in diagnostics {
let location = diagnostic.location(converter: expectedConverter)
parsingDiagnosticHandler(diagnostic, location)
}
}
try format(syntax: sourceFile, assumingFileURL: url, source: source, to: &outputStream)
}

Expand Down
37 changes: 27 additions & 10 deletions Sources/SwiftFormat/SwiftLinter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import SwiftFormatPrettyPrint
import SwiftFormatRules
import SwiftFormatWhitespaceLinter
import SwiftSyntax
import SwiftSyntaxParser
import SwiftParser
import SwiftDiagnostics

/// Diagnoses and reports problems in Swift source code or syntax trees according to the Swift style
/// guidelines.
Expand Down Expand Up @@ -53,7 +54,7 @@ public final class SwiftLinter {
/// - Throws: If an unrecoverable error occurs when formatting the code.
public func lint(
contentsOf url: URL,
parsingDiagnosticHandler: ((Diagnostic) -> Void)? = nil
parsingDiagnosticHandler: ((Diagnostic, SourceLocation) -> Void)? = nil
) throws {
guard FileManager.default.isReadableFile(atPath: url.path) else {
throw SwiftFormatError.fileNotReadable
Expand All @@ -62,9 +63,10 @@ public final class SwiftLinter {
if FileManager.default.fileExists(atPath: url.path, isDirectory: &isDir), isDir.boolValue {
throw SwiftFormatError.isDirectory
}
let sourceFile = try SyntaxParser.parse(url, diagnosticHandler: parsingDiagnosticHandler)
let source = try String(contentsOf: url, encoding: .utf8)
try lint(syntax: sourceFile, assumingFileURL: url, source: source)
let sourceFile = try Parser.parse(source: source)
try lint(syntax: sourceFile, assumingFileURL: url,
source: source, parsingDiagnosticHandler: parsingDiagnosticHandler)
}

/// Lints the given Swift source code.
Expand All @@ -78,11 +80,11 @@ public final class SwiftLinter {
public func lint(
source: String,
assumingFileURL url: URL,
parsingDiagnosticHandler: ((Diagnostic) -> Void)? = nil
parsingDiagnosticHandler: ((Diagnostic, SourceLocation) -> Void)? = nil
) throws {
let sourceFile =
try SyntaxParser.parse(source: source, diagnosticHandler: parsingDiagnosticHandler)
try lint(syntax: sourceFile, assumingFileURL: url, source: source)
let sourceFile = try Parser.parse(source: source)
try lint(syntax: sourceFile, assumingFileURL: url,
source: source, parsingDiagnosticHandler: parsingDiagnosticHandler)
}

/// Lints the given Swift syntax tree.
Expand All @@ -94,14 +96,29 @@ public final class SwiftLinter {
/// - url: A file URL denoting the filename/path that should be assumed for this syntax tree.
/// - Throws: If an unrecoverable error occurs when formatting the code.
public func lint(syntax: SourceFileSyntax, assumingFileURL url: URL) throws {
try lint(syntax: syntax, assumingFileURL: url, source: nil)
try lint(syntax: syntax, assumingFileURL: url,
source: nil, parsingDiagnosticHandler: nil)
}

private func lint(syntax: SourceFileSyntax, assumingFileURL url: URL, source: String?) throws {
private func lint(
syntax: SourceFileSyntax,
assumingFileURL url: URL,
source: String?,
parsingDiagnosticHandler: ((Diagnostic, SourceLocation) -> Void)?
) throws {
if let position = _firstInvalidSyntaxPosition(in: Syntax(syntax)) {
throw SwiftFormatError.fileContainsInvalidSyntax(position: position)
}

if let parsingDiagnosticHandler = parsingDiagnosticHandler {
let expectedConverter = SourceLocationConverter(file: url.path, tree: syntax)
let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: syntax)
for diagnostic in diagnostics {
let location = diagnostic.location(converter: expectedConverter)
parsingDiagnosticHandler(diagnostic, location)
}
}

let context = Context(
configuration: configuration, findingConsumer: findingConsumer, fileURL: url,
sourceFileSyntax: syntax, source: source, ruleNameCache: ruleNameCache)
Expand Down
42 changes: 26 additions & 16 deletions Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
self.config = configuration
self.operatorContext = operatorContext
self.maxlinelength = config.lineLength
super.init(viewMode: .sourceAccurate)
super.init(viewMode: .all)
}

func makeStream(from node: Syntax) -> [Token] {
Expand Down Expand Up @@ -328,7 +328,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
// Due to visitation order, the matching .open break is added in ParameterClauseSyntax.
after(node.signature.lastToken, tokens: .close)
}

arrangeParameterClause(node.signature.input, forcesBreakBeforeRightParen: node.body != nil)

// Prioritize keeping "<modifiers> init<punctuation>" together.
Expand Down Expand Up @@ -525,7 +525,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {

// Breaks are only allowed after `else` when there's a comment; otherwise there shouldn't be
// any newlines between `else` and the open brace or a following `if`.
if let tokenAfterElse = elseKeyword.nextToken, tokenAfterElse.leadingTrivia.hasLineComment {
if let tokenAfterElse = elseKeyword.nextToken(viewMode: .all), tokenAfterElse.leadingTrivia.hasLineComment {
after(node.elseKeyword, tokens: .break(.same, size: 1))
} else if let elseBody = node.elseBody, elseBody.is(IfStmtSyntax.self) {
after(node.elseKeyword, tokens: .space)
Expand Down Expand Up @@ -793,7 +793,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
// to exist at the EOL with the left paren or on its own line. The contents are always
// indented on the following lines, since parens always create a scope. An open/close break
// pair isn't used here to avoid forcing the closing paren down onto a new line.
if node.leftParen.nextToken?.leadingTrivia.hasLineComment ?? false {
if node.leftParen.nextToken(viewMode: .all)?.leadingTrivia.hasLineComment ?? false {
after(node.leftParen, tokens: .break(.continue, size: 0))
}
} else if elementCount > 1 {
Expand Down Expand Up @@ -951,8 +951,8 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
// When this function call is wrapped by a try-expr or await-expr, the group applied when
// visiting that wrapping expression is sufficient. Adding another group here in that case
// can result in unnecessarily breaking after the try/await keyword.
if !(base.firstToken?.previousToken?.parent?.is(TryExprSyntax.self) ?? false
|| base.firstToken?.previousToken?.parent?.is(AwaitExprSyntax.self) ?? false) {
if !(base.firstToken?.previousToken(viewMode: .all)?.parent?.is(TryExprSyntax.self) ?? false
|| base.firstToken?.previousToken(viewMode: .all)?.parent?.is(AwaitExprSyntax.self) ?? false) {
before(base.firstToken, tokens: .open)
after(calledMemberAccessExpr.name.lastToken, tokens: .close)
}
Expand Down Expand Up @@ -1304,7 +1304,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
if let lastElemTok = node.elements.lastToken {
after(lastElemTok, tokens: .break(breakKindClose, newlines: .soft), .close)
} else {
before(tokenToOpenWith.nextToken, tokens: .break(breakKindClose, newlines: .soft), .close)
before(tokenToOpenWith.nextToken(viewMode: .all), tokens: .break(breakKindClose, newlines: .soft), .close)
}

if isNestedInPostfixIfConfig(node: Syntax(node)) {
Expand Down Expand Up @@ -1915,7 +1915,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
var closesNeeded: Int = 0
var closeAfterToken: TokenSyntax? = nil

if let typeAnnotation = node.typeAnnotation {
if let typeAnnotation = node.typeAnnotation, !typeAnnotation.type.is(MissingTypeSyntax.self) {
after(
typeAnnotation.colon,
tokens: .break(.open(kind: .continuation), newlines: .elective(ignoresDiscretionary: true)))
Expand Down Expand Up @@ -2190,6 +2190,11 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
}

override func visit(_ node: GenericWhereClauseSyntax) -> SyntaxVisitorContinueKind {
guard node.whereKeyword != node.lastToken else {
verbatimToken(Syntax(node))
return .skipChildren
}

after(node.whereKeyword, tokens: .break(.open))
after(node.lastToken, tokens: .break(.close, size: 0))

Expand Down Expand Up @@ -2378,6 +2383,11 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {

// MARK: - Nodes representing unknown or malformed syntax

override func visit(_ node: UnexpectedNodesSyntax) -> SyntaxVisitorContinueKind {
verbatimToken(Syntax(node))
return .skipChildren
}

override func visit(_ node: UnknownDeclSyntax) -> SyntaxVisitorContinueKind {
verbatimToken(Syntax(node))
return .skipChildren
Expand Down Expand Up @@ -2429,7 +2439,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
appendMultilineStringSegments(at: pendingSegmentIndex)
} else if !ignoredTokens.contains(token) {
// Otherwise, it's just a regular token, so add the text as-is.
appendToken(.syntax(token.text))
appendToken(.syntax(token.presence == .present ? token.text : ""))
}

appendTrailingTrivia(token)
Expand Down Expand Up @@ -2883,7 +2893,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
}

private func extractLeadingTrivia(_ token: TokenSyntax) {
var isStartOfFile = token.previousToken == nil
var isStartOfFile = token.previousToken(viewMode: .all) == nil
let trivia = token.leadingTrivia

// If we're at the end of the file, determine at which index to stop checking trivia pieces to
Expand Down Expand Up @@ -3260,12 +3270,12 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
/// alongside the last token of the given node. Any tokens between `node.lastToken` and the
/// returned node's `lastToken` are delimiter tokens that shouldn't be preceded by a break.
private func outermostEnclosingNode(from node: Syntax) -> Syntax? {
guard let afterToken = node.lastToken?.nextToken, closingDelimiterTokens.contains(afterToken)
guard let afterToken = node.lastToken?.nextToken(viewMode: .all), closingDelimiterTokens.contains(afterToken)
else {
return nil
}
var parenthesizedExpr = afterToken.parent
while let nextToken = parenthesizedExpr?.lastToken?.nextToken,
while let nextToken = parenthesizedExpr?.lastToken?.nextToken(viewMode: .all),
closingDelimiterTokens.contains(nextToken),
let nextExpr = nextToken.parent
{
Expand Down Expand Up @@ -3355,9 +3365,9 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
// followed by a dot (for example, in an implicit member reference)---removing the spaces in
// those situations would cause the parser to greedily treat the combined sequence of
// operator characters as a single operator.
if case .postfixOperator? = token.previousToken?.tokenKind { return true }
if case .postfixOperator? = token.previousToken(viewMode: .all)?.tokenKind { return true }

switch token.nextToken?.tokenKind {
switch token.nextToken(viewMode: .all)?.tokenKind {
case .prefixOperator?, .prefixPeriod?: return true
default: return false
}
Expand Down Expand Up @@ -3389,7 +3399,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
// The leading trivia of the next token, after the ignored node, may contain content that
// belongs with the ignored node. The trivia extraction that is performed for `lastToken` later
// excludes that content so it needs to be extracted and added to the token stream here.
if let next = node.lastToken?.nextToken, let trivia = next.leadingTrivia.first {
if let next = node.lastToken?.nextToken(viewMode: .all), let trivia = next.leadingTrivia.first {
switch trivia {
case .lineComment, .blockComment:
trivia.write(to: &nodeText)
Expand Down Expand Up @@ -3581,7 +3591,7 @@ class CommentMovingRewriter: SyntaxRewriter {
override func visit(_ node: SequenceExprSyntax) -> ExprSyntax {
for element in node.elements {
if let binaryOperatorExpr = element.as(BinaryOperatorExprSyntax.self),
let followingToken = binaryOperatorExpr.operatorToken.nextToken,
let followingToken = binaryOperatorExpr.operatorToken.nextToken(viewMode: .all),
followingToken.leadingTrivia.hasLineComment
{
// Rewrite the trivia so that the comment is in the operator token's leading trivia.
Expand Down
Loading