Skip to content

Use string based SourceLocationConverter init because it's faster. #200

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 1 commit into from
Jun 10, 2020
Merged

Use string based SourceLocationConverter init because it's faster. #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 13 additions & 3 deletions Sources/SwiftFormat/SwiftFormatter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public final class SwiftFormatter {
throw SwiftFormatError.isDirectory
}
let sourceFile = try SyntaxParser.parse(url)
try format(syntax: sourceFile, assumingFileURL: url, to: &outputStream)
let source = try String(contentsOf: url, encoding: .utf8)
try format(syntax: sourceFile, assumingFileURL: url, source: source, to: &outputStream)
}

/// Formats the given Swift source code and writes the result to an output stream.
Expand All @@ -75,11 +76,13 @@ public final class SwiftFormatter {
source: String, assumingFileURL url: URL?, to outputStream: inout Output
) throws {
let sourceFile = try SyntaxParser.parse(source: source)
try format(syntax: sourceFile, assumingFileURL: url, to: &outputStream)
try format(syntax: sourceFile, assumingFileURL: url, source: source, to: &outputStream)
}

/// Formats the given Swift syntax tree and writes the result to an output stream.
///
/// - Note: The formatter may be faster using the source text, if it's available.
///
/// - Parameters:
/// - syntax: The Swift syntax tree to be converted to source code and formatted.
/// - url: A file URL denoting the filename/path that should be assumed for this syntax tree,
Expand All @@ -90,6 +93,13 @@ public final class SwiftFormatter {
/// - Throws: If an unrecoverable error occurs when formatting the code.
public func format<Output: TextOutputStream>(
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the APIs that take a SourceFileSyntax as input; the goal here is to let someone construct a syntax tree in memory using SwiftSyntax APIs (e.g., a code generator) and then hand that off to the swift-format API to format it, without having to convert it to a string with description first and then reparse it again.

Can we have our cake and eat it too? If we make the new source argument of Context optional, then we could create the SourceLocationConverter using source if it's present, or fall back to the syntax tree if it's not. Then, we'd only be pessimizing performance of the case where we didn't have source text to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works - I hadn't considered a use case where we needed to support having just the syntax representation.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we wouldn't change any of the public signatures—we wouldn't pass source at all into the version of the function that takes a SourceFileSyntax, because then we'd have to have a way to handle what would happen to the source locations if syntax.description and source didn't match.

I guess to keep the signatures all the same along with the cascading function calls, we'd need a private helper function that takes both syntax and source and have all the public ones delegate to that with the appropriate combinations, instead of having them delegate to a single bottom-most public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's reasonable, updated to use a private method that accepts both.

syntax: SourceFileSyntax, assumingFileURL url: URL?, to outputStream: inout Output
) throws {
try format(syntax: syntax, assumingFileURL: url, source: nil, to: &outputStream)
}

private func format<Output: TextOutputStream>(
syntax: SourceFileSyntax, assumingFileURL url: URL?, source: String?,
to outputStream: inout Output
) throws {
if let position = firstInvalidSyntaxPosition(in: Syntax(syntax)) {
throw SwiftFormatError.fileContainsInvalidSyntax(position: position)
Expand All @@ -98,7 +108,7 @@ public final class SwiftFormatter {
let assumedURL = url ?? URL(fileURLWithPath: "source")
let context = Context(
configuration: configuration, diagnosticEngine: diagnosticEngine, fileURL: assumedURL,
sourceFileSyntax: syntax)
sourceFileSyntax: syntax, source: source)
let pipeline = FormatPipeline(context: context)
let transformedSyntax = pipeline.visit(Syntax(syntax))

Expand Down
15 changes: 11 additions & 4 deletions Sources/SwiftFormat/SwiftLinter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ public final class SwiftLinter {
throw SwiftFormatError.isDirectory
}
let sourceFile = try SyntaxParser.parse(url)
try lint(syntax: sourceFile, assumingFileURL: url)
let source = try String(contentsOf: url, encoding: .utf8)
try lint(syntax: sourceFile, assumingFileURL: url, source: source)
}

/// Lints the given Swift source code.
Expand All @@ -65,23 +66,29 @@ public final class SwiftLinter {
/// - Throws: If an unrecoverable error occurs when formatting the code.
public func lint(source: String, assumingFileURL url: URL) throws {
let sourceFile = try SyntaxParser.parse(source: source)
try lint(syntax: sourceFile, assumingFileURL: url)
try lint(syntax: sourceFile, assumingFileURL: url, source: source)
}

/// Lints the given Swift syntax tree.
///
/// - Note: The linter may be faster using the source text, if it's available.
///
/// - Parameters:
/// - syntax: The Swift syntax tree to be converted to be linted.
/// - 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)
}

private func lint(syntax: SourceFileSyntax, assumingFileURL url: URL, source: String?) throws {
if let position = firstInvalidSyntaxPosition(in: Syntax(syntax)) {
throw SwiftFormatError.fileContainsInvalidSyntax(position: position)
}

let context = Context(
configuration: configuration, diagnosticEngine: diagnosticEngine, fileURL: url,
sourceFileSyntax: syntax)
sourceFileSyntax: syntax, source: source)
let pipeline = LintPipeline(context: context)
pipeline.walk(Syntax(syntax))

Expand Down
8 changes: 5 additions & 3 deletions Sources/SwiftFormatCore/Context.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,16 @@ public class Context {
configuration: Configuration,
diagnosticEngine: DiagnosticEngine?,
fileURL: URL,
sourceFileSyntax: SourceFileSyntax
sourceFileSyntax: SourceFileSyntax,
source: String? = nil
) {
self.configuration = configuration
self.diagnosticEngine = diagnosticEngine
self.fileURL = fileURL
self.importsXCTest = .notDetermined
self.sourceLocationConverter = SourceLocationConverter(
file: fileURL.path, tree: sourceFileSyntax)
self.sourceLocationConverter =
source.map { SourceLocationConverter(file: fileURL.path, source: $0) }
?? SourceLocationConverter(file: fileURL.path, tree: sourceFileSyntax)
self.ruleMask = RuleMask(
syntaxNode: Syntax(sourceFileSyntax),
sourceLocationConverter: sourceLocationConverter
Expand Down