From 133ce4dd545230cb8c2a2d3dfcb6a25bd0d30e57 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Thu, 14 Dec 2023 20:17:53 -0500 Subject: [PATCH 1/2] Migrate the parallel mode to Swift concurrency. This replaces `Dispatch.concurrentPerform` with a task group. One benefit of this approach is that we don't need to know the number of iterations that we want ahead of time. We can start tasks during the initial iteration over the `FileIterator` instead of eagerly expanding it first; this should potentially improve behavior for invocations that cover very large directories recursively, especially when they're on slower storage like a network drive. --- .../Frontend/ConfigurationLoader.swift | 45 ++++++++++++++----- Sources/swift-format/Frontend/Frontend.swift | 44 +++++++++--------- Sources/swift-format/Subcommands/Format.swift | 8 ++-- Sources/swift-format/Subcommands/Lint.swift | 10 ++--- .../Subcommands/PerformanceMeasurement.swift | 6 +-- Sources/swift-format/SwiftFormatCommand.swift | 2 +- 6 files changed, 72 insertions(+), 43 deletions(-) diff --git a/Sources/swift-format/Frontend/ConfigurationLoader.swift b/Sources/swift-format/Frontend/ConfigurationLoader.swift index 56154b090..31ca65356 100644 --- a/Sources/swift-format/Frontend/ConfigurationLoader.swift +++ b/Sources/swift-format/Frontend/ConfigurationLoader.swift @@ -15,9 +15,18 @@ import SwiftFormat /// Loads formatter configurations, caching them in memory so that multiple operations in the same /// directory do not repeatedly hit the file system. -struct ConfigurationLoader { +actor ConfigurationLoader { + /// Keeps track of the state of configurations in the cache. + private enum CacheEntry { + /// The configuration has been fully loaded. + case ready(Configuration) + + /// The configuration is in the process of being loaded. + case loading(Task) + } + /// The cache of previously loaded configurations. - private var cache = [String: Configuration]() + private var cache = [String: CacheEntry]() /// Returns the configuration found by searching in the directory (and ancestor directories) /// containing the given `.swift` source file. @@ -25,25 +34,41 @@ struct ConfigurationLoader { /// If no configuration file was found during the search, this method returns nil. /// /// - Throws: If a configuration file was found but an error occurred loading it. - mutating func configuration(forSwiftFileAt url: URL) throws -> Configuration? { + func configuration(forSwiftFileAt url: URL) async throws -> Configuration? { guard let configurationFileURL = Configuration.url(forConfigurationFileApplyingTo: url) else { return nil } - return try configuration(at: configurationFileURL) + return try await configuration(at: configurationFileURL) } /// Returns the configuration associated with the configuration file at the given URL. /// /// - Throws: If an error occurred loading the configuration. - mutating func configuration(at url: URL) throws -> Configuration { + func configuration(at url: URL) async throws -> Configuration { let cacheKey = url.absoluteURL.standardized.path - if let cachedConfiguration = cache[cacheKey] { - return cachedConfiguration + + if let cached = cache[cacheKey] { + switch cached { + case .ready(let configuration): + return configuration + case .loading(let task): + return try await task.value + } } - let configuration = try Configuration(contentsOf: url) - cache[cacheKey] = configuration - return configuration + let task = Task { + try Configuration(contentsOf: url) + } + cache[cacheKey] = .loading(task) + + do { + let configuration = try await task.value + cache[cacheKey] = .ready(configuration) + return configuration + } catch { + cache[cacheKey] = nil + throw error + } } } diff --git a/Sources/swift-format/Frontend/Frontend.swift b/Sources/swift-format/Frontend/Frontend.swift index 2d5274160..4610cc441 100644 --- a/Sources/swift-format/Frontend/Frontend.swift +++ b/Sources/swift-format/Frontend/Frontend.swift @@ -86,11 +86,11 @@ class Frontend { } /// Runs the linter or formatter over the inputs. - final func run() { + final func run() async { if lintFormatOptions.paths.isEmpty { - processStandardInput() + await processStandardInput() } else { - processURLs( + await processURLs( lintFormatOptions.paths.map(URL.init(fileURLWithPath:)), parallel: lintFormatOptions.parallel) } @@ -107,8 +107,8 @@ class Frontend { } /// Processes source content from standard input. - private func processStandardInput() { - guard let configuration = configuration( + private func processStandardInput() async { + guard let configuration = await configuration( fromPathOrString: lintFormatOptions.configuration, orInferredFromSwiftFileAt: nil) else { @@ -124,29 +124,33 @@ class Frontend { } /// Processes source content from a list of files and/or directories provided as file URLs. - private func processURLs(_ urls: [URL], parallel: Bool) { + private func processURLs(_ urls: [URL], parallel: Bool) async { precondition( !urls.isEmpty, "processURLs(_:) should only be called when 'urls' is non-empty.") if parallel { - let filesToProcess = - FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks) - .compactMap(openAndPrepareFile) - DispatchQueue.concurrentPerform(iterations: filesToProcess.count) { index in - processFile(filesToProcess[index]) + await withTaskGroup(of: Void.self) { group in + for url in FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks) { + group.addTask { + if let fileToProcess = await self.openAndPrepareFile(at: url) { + self.processFile(fileToProcess) + } + } + } } } else { - FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks) - .lazy - .compactMap(openAndPrepareFile) - .forEach(processFile) + for url in FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks) { + if let fileToProcess = await openAndPrepareFile(at: url) { + processFile(fileToProcess) + } + } } } /// Read and prepare the file at the given path for processing, optionally synchronizing /// diagnostic output. - private func openAndPrepareFile(at url: URL) -> FileToProcess? { + private func openAndPrepareFile(at url: URL) async -> FileToProcess? { guard let sourceFile = try? FileHandle(forReadingFrom: url) else { diagnosticsEngine.emitError( "Unable to open \(url.relativePath): file is not readable or does not exist") @@ -154,7 +158,7 @@ class Frontend { } guard - let configuration = configuration( + let configuration = await configuration( fromPathOrString: lintFormatOptions.configuration, orInferredFromSwiftFileAt: url) else { @@ -185,13 +189,13 @@ class Frontend { private func configuration( fromPathOrString pathOrString: String?, orInferredFromSwiftFileAt swiftFileURL: URL? - ) -> Configuration? { + ) async -> Configuration? { if let pathOrString = pathOrString { // If an explicit configuration file path was given, try to load it and fail if it cannot be // loaded. (Do not try to fall back to a path inferred from the source file path.) let configurationFileURL = URL(fileURLWithPath: pathOrString) do { - let configuration = try configurationLoader.configuration(at: configurationFileURL) + let configuration = try await configurationLoader.configuration(at: configurationFileURL) self.checkForUnrecognizedRules(in: configuration) return configuration } catch { @@ -213,7 +217,7 @@ class Frontend { // then try to load the configuration by inferring it based on the source file path. if let swiftFileURL = swiftFileURL { do { - if let configuration = try configurationLoader.configuration(forSwiftFileAt: swiftFileURL) { + if let configuration = try await configurationLoader.configuration(forSwiftFileAt: swiftFileURL) { self.checkForUnrecognizedRules(in: configuration) return configuration } diff --git a/Sources/swift-format/Subcommands/Format.swift b/Sources/swift-format/Subcommands/Format.swift index 548e30e18..bbb95229a 100644 --- a/Sources/swift-format/Subcommands/Format.swift +++ b/Sources/swift-format/Subcommands/Format.swift @@ -14,7 +14,7 @@ import ArgumentParser extension SwiftFormatCommand { /// Formats one or more files containing Swift code. - struct Format: ParsableCommand { + struct Format: AsyncParsableCommand { static var configuration = CommandConfiguration( abstract: "Format Swift source code", discussion: "When no files are specified, it expects the source from standard input.") @@ -39,10 +39,10 @@ extension SwiftFormatCommand { } } - func run() throws { - try performanceMeasurementOptions.printingInstructionCountIfRequested() { + func run() async throws { + try await performanceMeasurementOptions.printingInstructionCountIfRequested() { let frontend = FormatFrontend(lintFormatOptions: formatOptions, inPlace: inPlace) - frontend.run() + await frontend.run() if frontend.diagnosticsEngine.hasErrors { throw ExitCode.failure } } } diff --git a/Sources/swift-format/Subcommands/Lint.swift b/Sources/swift-format/Subcommands/Lint.swift index 43b3872a9..488048ee0 100644 --- a/Sources/swift-format/Subcommands/Lint.swift +++ b/Sources/swift-format/Subcommands/Lint.swift @@ -14,7 +14,7 @@ import ArgumentParser extension SwiftFormatCommand { /// Emits style diagnostics for one or more files containing Swift code. - struct Lint: ParsableCommand { + struct Lint: AsyncParsableCommand { static var configuration = CommandConfiguration( abstract: "Diagnose style issues in Swift source code", discussion: "When no files are specified, it expects the source from standard input.") @@ -31,11 +31,11 @@ extension SwiftFormatCommand { @OptionGroup(visibility: .hidden) var performanceMeasurementOptions: PerformanceMeasurementsOptions - func run() throws { - try performanceMeasurementOptions.printingInstructionCountIfRequested { + func run() async throws { + try await performanceMeasurementOptions.printingInstructionCountIfRequested { let frontend = LintFrontend(lintFormatOptions: lintOptions) - frontend.run() - + await frontend.run() + if frontend.diagnosticsEngine.hasErrors || strict && frontend.diagnosticsEngine.hasWarnings { throw ExitCode.failure } diff --git a/Sources/swift-format/Subcommands/PerformanceMeasurement.swift b/Sources/swift-format/Subcommands/PerformanceMeasurement.swift index a230aa052..af1c89321 100644 --- a/Sources/swift-format/Subcommands/PerformanceMeasurement.swift +++ b/Sources/swift-format/Subcommands/PerformanceMeasurement.swift @@ -19,15 +19,15 @@ struct PerformanceMeasurementsOptions: ParsableArguments { /// If `measureInstructions` is set, execute `body` and print the number of instructions /// executed by it. Otherwise, just execute `body` - func printingInstructionCountIfRequested(_ body: () throws -> T) rethrows -> T { + func printingInstructionCountIfRequested(_ body: () async throws -> T) async rethrows -> T { if !measureInstructions { - return try body() + return try await body() } else { let startInstructions = getInstructionsExecuted() defer { print("Instructions executed: \(getInstructionsExecuted() - startInstructions)") } - return try body() + return try await body() } } } diff --git a/Sources/swift-format/SwiftFormatCommand.swift b/Sources/swift-format/SwiftFormatCommand.swift index 5b814a159..0ee461a2f 100644 --- a/Sources/swift-format/SwiftFormatCommand.swift +++ b/Sources/swift-format/SwiftFormatCommand.swift @@ -15,7 +15,7 @@ import ArgumentParser /// Collects the command line options that were passed to `swift-format` and dispatches to the /// appropriate subcommand. @main -struct SwiftFormatCommand: ParsableCommand { +struct SwiftFormatCommand: AsyncParsableCommand { static var configuration = CommandConfiguration( commandName: "swift-format", abstract: "Format or lint Swift source code", From 1bed37561c1fe5ee6b92042ec9031cdb88893c16 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Sat, 16 Dec 2023 13:11:51 -0500 Subject: [PATCH 2/2] Use an `AsyncStream` to emit diagnostics. Instead of having `StderrDiagnosticPrinter` internally manage its own synchronization (which currently uses a Dispatch queue), turn `DiagnosticsEngine` into an actor that processes diagnostics from an `AsyncStream`. When rules emit findings, they are enqueued in the stream, and a single task iterates over them to ensure that individual diagnostic handlers don't need to synchronize their own activity. --- .../Frontend/FormatFrontend.swift | 4 +- Sources/swift-format/Frontend/Frontend.swift | 5 +- Sources/swift-format/Subcommands/Format.swift | 4 +- Sources/swift-format/Subcommands/Lint.swift | 6 +- .../Utilities/DiagnosticsEngine.swift | 79 +++++++++++++++---- .../Utilities/StderrDiagnosticPrinter.swift | 25 +++--- 6 files changed, 83 insertions(+), 40 deletions(-) diff --git a/Sources/swift-format/Frontend/FormatFrontend.swift b/Sources/swift-format/Frontend/FormatFrontend.swift index 019eb40c0..354af756b 100644 --- a/Sources/swift-format/Frontend/FormatFrontend.swift +++ b/Sources/swift-format/Frontend/FormatFrontend.swift @@ -20,9 +20,9 @@ class FormatFrontend: Frontend { /// Whether or not to format the Swift file in-place. private let inPlace: Bool - init(lintFormatOptions: LintFormatOptions, inPlace: Bool) { + init(lintFormatOptions: LintFormatOptions, inPlace: Bool) async { self.inPlace = inPlace - super.init(lintFormatOptions: lintFormatOptions) + await super.init(lintFormatOptions: lintFormatOptions) } override func processFile(_ fileToProcess: FileToProcess) { diff --git a/Sources/swift-format/Frontend/Frontend.swift b/Sources/swift-format/Frontend/Frontend.swift index 4610cc441..4d270ca42 100644 --- a/Sources/swift-format/Frontend/Frontend.swift +++ b/Sources/swift-format/Frontend/Frontend.swift @@ -76,13 +76,13 @@ class Frontend { /// Creates a new frontend with the given options. /// /// - Parameter lintFormatOptions: Options that apply during formatting or linting. - init(lintFormatOptions: LintFormatOptions) { + init(lintFormatOptions: LintFormatOptions) async { self.lintFormatOptions = lintFormatOptions self.diagnosticPrinter = StderrDiagnosticPrinter( colorMode: lintFormatOptions.colorDiagnostics.map { $0 ? .on : .off } ?? .auto) self.diagnosticsEngine = - DiagnosticsEngine(diagnosticsHandlers: [diagnosticPrinter.printDiagnostic]) + await DiagnosticsEngine(diagnosticsHandlers: [diagnosticPrinter.printDiagnostic]) } /// Runs the linter or formatter over the inputs. @@ -94,6 +94,7 @@ class Frontend { lintFormatOptions.paths.map(URL.init(fileURLWithPath:)), parallel: lintFormatOptions.parallel) } + await diagnosticsEngine.flush() } /// Called by the frontend to process a single file. diff --git a/Sources/swift-format/Subcommands/Format.swift b/Sources/swift-format/Subcommands/Format.swift index bbb95229a..fbdd9a186 100644 --- a/Sources/swift-format/Subcommands/Format.swift +++ b/Sources/swift-format/Subcommands/Format.swift @@ -41,9 +41,9 @@ extension SwiftFormatCommand { func run() async throws { try await performanceMeasurementOptions.printingInstructionCountIfRequested() { - let frontend = FormatFrontend(lintFormatOptions: formatOptions, inPlace: inPlace) + let frontend = await FormatFrontend(lintFormatOptions: formatOptions, inPlace: inPlace) await frontend.run() - if frontend.diagnosticsEngine.hasErrors { throw ExitCode.failure } + if await frontend.diagnosticsEngine.hasErrors { throw ExitCode.failure } } } } diff --git a/Sources/swift-format/Subcommands/Lint.swift b/Sources/swift-format/Subcommands/Lint.swift index 488048ee0..df0f9d75e 100644 --- a/Sources/swift-format/Subcommands/Lint.swift +++ b/Sources/swift-format/Subcommands/Lint.swift @@ -33,10 +33,12 @@ extension SwiftFormatCommand { func run() async throws { try await performanceMeasurementOptions.printingInstructionCountIfRequested { - let frontend = LintFrontend(lintFormatOptions: lintOptions) + let frontend = await LintFrontend(lintFormatOptions: lintOptions) await frontend.run() - if frontend.diagnosticsEngine.hasErrors || strict && frontend.diagnosticsEngine.hasWarnings { + let hasErrors = await frontend.diagnosticsEngine.hasErrors + let hasWarnings = await frontend.diagnosticsEngine.hasWarnings + if hasErrors || strict && hasWarnings { throw ExitCode.failure } } diff --git a/Sources/swift-format/Utilities/DiagnosticsEngine.swift b/Sources/swift-format/Utilities/DiagnosticsEngine.swift index a6cd6978f..41f4218b3 100644 --- a/Sources/swift-format/Utilities/DiagnosticsEngine.swift +++ b/Sources/swift-format/Utilities/DiagnosticsEngine.swift @@ -16,7 +16,16 @@ import SwiftDiagnostics /// Unifies the handling of findings from the linter, parsing errors from the syntax parser, and /// generic errors from the frontend so that they are emitted in a uniform fashion. -final class DiagnosticsEngine { +/// +/// To best support parallel mode, the diagnostic engine asynchronously enqueues diagnostics and the +/// `emit*` and `consume*` methods return immediately, before the handlers have been called. +/// However, the engine does guarantee the following: +/// +/// * Diagnostics are processed in the order that those methods are called. +/// * Calls to the registered handlers will not overlap, so they do not need to internally +/// synchronize themselves (for example, when printing diagnostics across multiple tasks to +/// standard error). +actor DiagnosticsEngine { /// The handler functions that will be called to process diagnostics that are emitted. private let handlers: [(Diagnostic) -> Void] @@ -26,29 +35,65 @@ final class DiagnosticsEngine { /// A Boolean value indicating whether any warnings were emitted by the diagnostics engine. private(set) var hasWarnings: Bool + /// The continuation used to enqueue emitted diagnostics into the async stream. + private nonisolated let continuation: AsyncStream.Continuation + + /// The background task that iterates over the diagnostics as they are emitted and hands them off + /// to the handlers. + private var streamTask: Task! + /// Creates a new diagnostics engine with the given diagnostic handlers. /// /// - Parameter diagnosticsHandlers: An array of functions, each of which takes a `Diagnostic` as /// its sole argument and returns `Void`. The functions are called whenever a diagnostic is /// received by the engine. - init(diagnosticsHandlers: [(Diagnostic) -> Void]) { + init(diagnosticsHandlers: [(Diagnostic) -> Void]) async { self.handlers = diagnosticsHandlers self.hasErrors = false self.hasWarnings = false + + var continuation: AsyncStream.Continuation! + let diagnosticStream: AsyncStream = .init { continuation = $0 } + self.continuation = continuation + self.streamTask = Task { + await self.streamDiagnostics(from: diagnosticStream) + } + } + + /// Processes diagnostics from the given stream as they arrive, sending each one to the registered + /// handlers. + private func streamDiagnostics(from stream: AsyncStream) async { + for await diagnostic in stream { + switch diagnostic.severity { + case .error: self.hasErrors = true + case .warning: self.hasWarnings = true + default: break + } + + for handler in handlers { + handler(diagnostic) + } + } + + // TODO: It would be useful to model handlers as a protocol instead so that we can add a + // `flush()` method to them. This could support handlers that need to finalize their results; + // for example, a JSON diagnostic printer that needs to know when the stream has ended so that + // it can terminate its root object. } /// Emits the diagnostic by passing it to the registered handlers, and tracks whether it was an /// error or warning diagnostic. - private func emit(_ diagnostic: Diagnostic) { - switch diagnostic.severity { - case .error: self.hasErrors = true - case .warning: self.hasWarnings = true - default: break - } + private nonisolated func emit(_ diagnostic: Diagnostic) { + continuation.yield(diagnostic) + } - for handler in handlers { - handler(diagnostic) - } + /// Waits until the remaining diagnostics in the stream have been processed. + /// + /// This method must be called before program shutdown to ensure that all remaining diagnostics + /// are handled. + nonisolated func flush() async { + continuation.finish() + await self.streamTask.value } /// Emits a generic error message. @@ -57,7 +102,7 @@ final class DiagnosticsEngine { /// - message: The message associated with the error. /// - location: The location in the source code associated with the error, or nil if there is no /// location associated with the error. - func emitError(_ message: String, location: SourceLocation? = nil) { + nonisolated func emitError(_ message: String, location: SourceLocation? = nil) { emit( Diagnostic( severity: .error, @@ -71,7 +116,7 @@ final class DiagnosticsEngine { /// - message: The message associated with the error. /// - location: The location in the source code associated with the error, or nil if there is no /// location associated with the error. - func emitWarning(_ message: String, location: SourceLocation? = nil) { + nonisolated func emitWarning(_ message: String, location: SourceLocation? = nil) { emit( Diagnostic( severity: .warning, @@ -82,7 +127,7 @@ final class DiagnosticsEngine { /// Emits a finding from the linter and any of its associated notes as diagnostics. /// /// - Parameter finding: The finding that should be emitted. - func consumeFinding(_ finding: Finding) { + nonisolated func consumeFinding(_ finding: Finding) { emit(diagnosticMessage(for: finding)) for note in finding.notes { @@ -97,7 +142,7 @@ final class DiagnosticsEngine { /// Emits a diagnostic from the syntax parser and any of its associated notes. /// /// - Parameter diagnostic: The syntax parser diagnostic that should be emitted. - func consumeParserDiagnostic( + nonisolated func consumeParserDiagnostic( _ diagnostic: SwiftDiagnostics.Diagnostic, _ location: SourceLocation ) { @@ -106,7 +151,7 @@ final class DiagnosticsEngine { /// Converts a diagnostic message from the syntax parser into a diagnostic message that can be /// used by the `TSCBasic` diagnostics engine and returns it. - private func diagnosticMessage( + private nonisolated func diagnosticMessage( for message: SwiftDiagnostics.DiagnosticMessage, at location: SourceLocation ) -> Diagnostic { @@ -126,7 +171,7 @@ final class DiagnosticsEngine { /// Converts a lint finding into a diagnostic message that can be used by the `TSCBasic` /// diagnostics engine and returns it. - private func diagnosticMessage(for finding: Finding) -> Diagnostic { + private nonisolated func diagnosticMessage(for finding: Finding) -> Diagnostic { let severity: Diagnostic.Severity switch finding.severity { case .error: severity = .error diff --git a/Sources/swift-format/Utilities/StderrDiagnosticPrinter.swift b/Sources/swift-format/Utilities/StderrDiagnosticPrinter.swift index f7730f00c..920d53fa8 100644 --- a/Sources/swift-format/Utilities/StderrDiagnosticPrinter.swift +++ b/Sources/swift-format/Utilities/StderrDiagnosticPrinter.swift @@ -38,9 +38,6 @@ final class StderrDiagnosticPrinter { case reset = "0" } - /// The queue used to synchronize printing uninterrupted diagnostic messages. - private let printQueue = DispatchQueue(label: "com.apple.swift-format.StderrDiagnosticPrinter") - /// Indicates whether colors should be used when printing diagnostics. private let useColors: Bool @@ -58,22 +55,20 @@ final class StderrDiagnosticPrinter { /// Prints a diagnostic to standard error. func printDiagnostic(_ diagnostic: Diagnostic) { - printQueue.sync { - let stderr = FileHandleTextOutputStream(FileHandle.standardError) + let stderr = FileHandleTextOutputStream(FileHandle.standardError) - stderr.write("\(ansiSGR(.boldWhite))\(description(of: diagnostic.location)): ") + stderr.write("\(ansiSGR(.boldWhite))\(description(of: diagnostic.location)): ") - switch diagnostic.severity { - case .error: stderr.write("\(ansiSGR(.boldRed))error: ") - case .warning: stderr.write("\(ansiSGR(.boldMagenta))warning: ") - case .note: stderr.write("\(ansiSGR(.boldGray))note: ") - } + switch diagnostic.severity { + case .error: stderr.write("\(ansiSGR(.boldRed))error: ") + case .warning: stderr.write("\(ansiSGR(.boldMagenta))warning: ") + case .note: stderr.write("\(ansiSGR(.boldGray))note: ") + } - if let category = diagnostic.category { - stderr.write("\(ansiSGR(.boldYellow))[\(category)] ") - } - stderr.write("\(ansiSGR(.boldWhite))\(diagnostic.message)\(ansiSGR(.reset))\n") + if let category = diagnostic.category { + stderr.write("\(ansiSGR(.boldYellow))[\(category)] ") } + stderr.write("\(ansiSGR(.boldWhite))\(diagnostic.message)\(ansiSGR(.reset))\n") } /// Returns a string representation of the given diagnostic location, or a fallback string if the