Fail fast and safe for unparseable files. Use the same DiagnosticEngine for all diagnostics.

After finding some "source" files that stretched the syntax parser to its limits, it looks like the formatter cannot trust any AST that contains any unknown/invalid nodes. It's possible for an unknown node to cause the parser to group tokens in ways that create known but incorrect nodes. Instead of potentially formatting those tokens as the wrong kind of node, which may be destructive, fail early on the file and refuse to format it. When that happens, the formatter raises a diagnostic (which goes to stderr by default).

Additionally, the formatter was using a new diagnostic engine for every file and was explicitly writing certain messages directly to stderr. I've reworked the format and lint operators to use the same diagnostic engine for every file and to use that diagnostic engine instead of stderr. By default, the diagnostic engine writes to stderr so it's a essentially a no-op change but can be helpful if anyone wants to add a diagnostic consumer later.

Author: dylansturg

Package.swift

@@ -71,6 +71,12 @@ let package = Package(
         "SwiftToolsSupport-auto",
       ]
     ),
+    .testTarget(
+      name: "SwiftFormatTests",
+      dependencies: [
+        "SwiftFormat",
+      ]
+    ),
     .testTarget(
       name: "SwiftFormatConfigurationTests",
       dependencies: ["SwiftFormatConfiguration"]

Sources/SwiftFormat/SwiftFormatError.swift

@@ -18,4 +18,7 @@ public enum SwiftFormatError: Error {
 
   /// The requested file was a directory.
   case isDirectory
+
+  /// The file contains invalid or unrecognized Swift syntax and cannot be handled safely.
+  case fileContainsInvalidSyntax
 }

Sources/SwiftFormat/SwiftFormatter.swift

@@ -91,8 +91,11 @@ public final class SwiftFormatter {
   public func format<Output: TextOutputStream>(
     syntax: SourceFileSyntax, assumingFileURL url: URL?, to outputStream: inout Output
   ) throws {
-    let assumedURL = url ?? URL(fileURLWithPath: "source")
+    guard isSyntaxValidForProcessing(Syntax(syntax)) else {
+      throw SwiftFormatError.fileContainsInvalidSyntax
+    }
 
+    let assumedURL = url ?? URL(fileURLWithPath: "source")
     let context = Context(
       configuration: configuration, diagnosticEngine: diagnosticEngine, fileURL: assumedURL,
       sourceFileSyntax: syntax)

Sources/SwiftFormat/SwiftLinter.swift

@@ -75,6 +75,10 @@ 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 {
+    guard isSyntaxValidForProcessing(Syntax(syntax)) else {
+      throw SwiftFormatError.fileContainsInvalidSyntax
+    }
+    
     let context = Context(
       configuration: configuration, diagnosticEngine: diagnosticEngine, fileURL: url,
       sourceFileSyntax: syntax)

Sources/SwiftFormat/SyntaxValidatingVisitor.swift

@@ -0,0 +1,74 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import SwiftSyntax
+
+/// A SyntaxVisitor that searches for nodes that cannot be handled safely.
+fileprivate class SyntaxValidatingVisitor: SyntaxVisitor {
+  /// Tracks whether an invalid node has been encountered.
+  var isValidSyntax = true
+
+  override func visit(_ node: UnknownSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: UnknownDeclSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: UnknownExprSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: UnknownStmtSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: UnknownTypeSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: UnknownPatternSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: NonEmptyTokenListSyntax) -> SyntaxVisitorContinueKind {
+    isValidSyntax = false
+    return .skipChildren
+  }
+
+  override func visit(_ node: AttributeSyntax) -> SyntaxVisitorContinueKind {
+    // The token list is used to collect any unexpected tokens. When it's missing or empty, then
+    // there were no unexpected tokens. Otherwise, the attribute is invalid.
+    guard node.tokenList?.isEmpty ?? true else {
+      isValidSyntax = false
+      return .skipChildren
+    }
+    return .visitChildren
+  }
+}
+
+/// Returns whether the given syntax contains any nodes which are invalid or unrecognized and
+/// cannot be handled safely.
+///
+/// - Parameter syntax: The root of a tree of syntax nodes to check for compatibility.
+func isSyntaxValidForProcessing(_ syntax: Syntax) -> Bool {
+  let visitor = SyntaxValidatingVisitor()
+  visitor.walk(syntax)
+  return visitor.isValidSyntax
+}

Sources/swift-format/Run.swift

@@ -25,40 +25,38 @@ import TSCBasic
 ///   - sourceFile: A file handle from which to read the source code to be linted.
 ///   - assumingFilename: The filename of the source file, used in diagnostic output.
 ///   - debugOptions: The set containing any debug options that were supplied on the command line.
+///   - diagnosticEngine: A diagnostic collector that handles diagnostic messages.
 /// - Returns: Zero if there were no lint errors, otherwise a non-zero number.
 func lintMain(
   configuration: Configuration, sourceFile: FileHandle, assumingFilename: String?,
-  debugOptions: DebugOptions
+  debugOptions: DebugOptions, diagnosticEngine: DiagnosticEngine
 ) -> Int {
-  let diagnosticEngine = makeDiagnosticEngine()
   let linter = SwiftLinter(configuration: configuration, diagnosticEngine: diagnosticEngine)
   linter.debugOptions = debugOptions
   let assumingFileURL = URL(fileURLWithPath: assumingFilename ?? "<stdin>")
 
   guard let source = readSource(from: sourceFile) else {
-    stderrStream.write("Unable to read source for linting from \(assumingFileURL.path).\n")
-    stderrStream.flush()
+    diagnosticEngine.diagnose(
+      Diagnostic.Message(.error, "Unable to read source for linting from \(assumingFileURL.path)."))
     return 1
   }
 
   do {
     try linter.lint(source: source, assumingFileURL: assumingFileURL)
   } catch SwiftFormatError.fileNotReadable {
     let path = assumingFileURL.path
-    stderrStream.write("Unable to lint \(path): file is not readable or does not exist.\n")
-    stderrStream.flush()
+    diagnosticEngine.diagnose(
+      Diagnostic.Message(.error, "Unable to lint \(path): file is not readable or does not exist."))
     return 1
-  } catch {
+  } catch SwiftFormatError.fileContainsInvalidSyntax {
+     let path = assumingFileURL.path
+     diagnosticEngine.diagnose(
+       Diagnostic.Message(
+         .error, "Unable to line \(path): file contains invalid or unrecognized Swift syntax."))
+     return 1
+   } catch {
     let path = assumingFileURL.path
-    // Workaround: we're unable to directly catch unknownTokenKind errors due to access
-    // restrictions. TODO: this can be removed when we update to Swift 5.0.
-    if "\(error)" == "unknownTokenKind(\"pound_error\")" {
-      stderrStream.write("Unable to lint \(path): unknownTokenKind(\"pound_error\")\n")
-      stderrStream.flush()
-      return 1
-    }
-    stderrStream.write("Unable to lint \(path): \(error)\n")
-    stderrStream.flush()
+    diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to lint \(path): \(error)"))
     exit(1)
   }
   return diagnosticEngine.diagnostics.isEmpty ? 0 : 1
@@ -72,18 +70,20 @@ func lintMain(
 ///   - assumingFilename: The filename of the source file, used in diagnostic output.
 ///   - inPlace: Whether or not to overwrite the current file when formatting.
 ///   - debugOptions: The set containing any debug options that were supplied on the command line.
+///   - diagnosticEngine: A diagnostic collector that handles diagnostic messages.
 /// - Returns: Zero if there were no format errors, otherwise a non-zero number.
 func formatMain(
   configuration: Configuration, sourceFile: FileHandle, assumingFilename: String?, inPlace: Bool,
-  debugOptions: DebugOptions
+  debugOptions: DebugOptions, diagnosticEngine: DiagnosticEngine
 ) -> Int {
-  let formatter = SwiftFormatter(configuration: configuration, diagnosticEngine: nil)
+  let formatter = SwiftFormatter(configuration: configuration, diagnosticEngine: diagnosticEngine)
   formatter.debugOptions = debugOptions
   let assumingFileURL = URL(fileURLWithPath: assumingFilename ?? "<stdin>")
 
   guard let source = readSource(from: sourceFile) else {
-    stderrStream.write("Unable to read source for formatting from \(assumingFileURL.path).\n")
-    stderrStream.flush()
+    diagnosticEngine.diagnose(
+      Diagnostic.Message(
+        .error, "Unable to read source for formatting from \(assumingFileURL.path)."))
     return 1
   }
 
@@ -103,26 +103,24 @@ func formatMain(
     }
   } catch SwiftFormatError.fileNotReadable {
     let path = assumingFileURL.path
-    stderrStream.write("Unable to format \(path): file is not readable or does not exist.\n")
-    stderrStream.flush()
+    diagnosticEngine.diagnose(
+      Diagnostic.Message(
+        .error, "Unable to format \(path): file is not readable or does not exist."))
+    return 1
+  } catch SwiftFormatError.fileContainsInvalidSyntax {
+    let path = assumingFileURL.path
+    diagnosticEngine.diagnose(
+      Diagnostic.Message(
+        .error, "Unable to format \(path): file contains invalid or unrecognized Swift syntax."))
     return 1
   } catch {
     let path = assumingFileURL.path
-    stderrStream.write("Unable to format \(path): \(error)\n")
-    stderrStream.flush()
+    diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to format \(path): \(error)"))
     exit(1)
   }
   return 0
 }
 
-/// Makes and returns a new configured diagnostic engine.
-fileprivate func makeDiagnosticEngine() -> DiagnosticEngine {
-  let engine = DiagnosticEngine()
-  let consumer = PrintingDiagnosticConsumer()
-  engine.addConsumer(consumer)
-  return engine
-}
-
 /// Reads from the given file handle until EOF is reached, then returns the contents as a UTF8
 /// encoded string.
 fileprivate func readSource(from fileHandle: FileHandle) -> String? {

Sources/swift-format/main.swift

@@ -14,12 +14,14 @@ import Foundation
 import SwiftFormat
 import SwiftFormatConfiguration
 import SwiftFormatCore
+import SwiftSyntax
 import TSCBasic
 import TSCUtility
 
 fileprivate func main(_ arguments: [String]) -> Int32 {
   let url = URL(fileURLWithPath: arguments.first!)
   let options = processArguments(commandName: url.lastPathComponent, Array(arguments.dropFirst()))
+  let diagnosticEngine = makeDiagnosticEngine()
   switch options.mode {
   case .format:
     if options.paths.isEmpty {
@@ -29,13 +31,17 @@ fileprivate func main(_ arguments: [String]) -> Int32 {
         formatMain(
           configuration: configuration, sourceFile: FileHandle.standardInput,
           assumingFilename: options.assumeFilename, inPlace: false,
-          debugOptions: options.debugOptions))
+          debugOptions: options.debugOptions, diagnosticEngine: diagnosticEngine))
     }
-    return processSources(from: options.paths, configurationPath: options.configurationPath) {
+    return processSources(
+      from: options.paths, configurationPath: options.configurationPath,
+      diagnosticEngine: diagnosticEngine
+    ) {
       (sourceFile, path, configuration) in
       formatMain(
         configuration: configuration, sourceFile: sourceFile, assumingFilename: path,
-        inPlace: options.inPlace, debugOptions: options.debugOptions)
+        inPlace: options.inPlace, debugOptions: options.debugOptions,
+        diagnosticEngine: diagnosticEngine)
     }
   case .lint:
     if options.paths.isEmpty {
@@ -44,13 +50,17 @@ fileprivate func main(_ arguments: [String]) -> Int32 {
       return Int32(
         lintMain(
           configuration: configuration, sourceFile: FileHandle.standardInput,
-          assumingFilename: options.assumeFilename, debugOptions: options.debugOptions))
+          assumingFilename: options.assumeFilename, debugOptions: options.debugOptions,
+          diagnosticEngine: diagnosticEngine))
     }
-    return processSources(from: options.paths, configurationPath: options.configurationPath) {
+    return processSources(
+      from: options.paths, configurationPath: options.configurationPath,
+      diagnosticEngine: diagnosticEngine
+    ) {
       (sourceFile, path, configuration) in
       lintMain(
         configuration: configuration, sourceFile: sourceFile, assumingFilename: path,
-        debugOptions: options.debugOptions)
+        debugOptions: options.debugOptions, diagnosticEngine: diagnosticEngine)
     }
   case .dumpConfiguration:
     dumpDefaultConfiguration()
@@ -66,16 +76,17 @@ fileprivate func main(_ arguments: [String]) -> Int32 {
 /// - Parameters:
 ///   - paths: The file paths for the source files to process with a transformation.
 ///   - configurationPath: The file path to a swift-format configuration file.
+///   - diagnosticEngine: A diagnostic collector that handles diagnostic messages.
 ///   - transform: A closure that performs a transformation on a specific source file.
 fileprivate func processSources(
-  from paths: [String], configurationPath: String?,
+  from paths: [String], configurationPath: String?, diagnosticEngine: DiagnosticEngine,
   transform: (FileHandle, String, Configuration) -> Int
 ) -> Int32 {
   var result = 0
   for path in FileIterator(paths: paths) {
     guard let sourceFile = FileHandle(forReadingAtPath: path) else {
-      stderrStream.write("Unable to create a file handle for source from \(path).\n")
-      stderrStream.flush()
+      diagnosticEngine.diagnose(
+        Diagnostic.Message(.error, "Unable to create a file handle for source from \(path)."))
       return 1
     }
     let configuration = loadConfiguration(forSwiftFile: path, configFilePath: configurationPath)
@@ -84,6 +95,14 @@ fileprivate func processSources(
   return Int32(result)
 }
 
+/// Makes and returns a new configured diagnostic engine.
+fileprivate func makeDiagnosticEngine() -> DiagnosticEngine {
+  let engine = DiagnosticEngine()
+  let consumer = PrintingDiagnosticConsumer()
+  engine.addConsumer(consumer)
+  return engine
+}
+
 /// Load the configuration.
 fileprivate func loadConfiguration(
   forSwiftFile swiftFilePath: String?, configFilePath: String?
@@ -92,15 +111,15 @@ fileprivate func loadConfiguration(
     return decodedConfiguration(fromFile: URL(fileURLWithPath: configFilePath))
   }
 
-  if let swiftFileURL = swiftFilePath.map(URL.init(fileURLWithPath:)), 
-    let configFileURL = Configuration.url(forConfigurationFileApplyingTo: swiftFileURL) {
-      return decodedConfiguration(fromFile: configFileURL)
+  if let swiftFileURL = swiftFilePath.map(URL.init(fileURLWithPath:)),
+    let configFileURL = Configuration.url(forConfigurationFileApplyingTo: swiftFileURL)
+  {
+    return decodedConfiguration(fromFile: configFileURL)
   }
 
   return Configuration()
 }
 
-
 /// Loads and returns a `Configuration` from the given JSON file if it is found and is valid. If the
 /// file does not exist or there was an error decoding it, the program exits with a non-zero exit
 /// code.

Tests/LinuxMain.swift

@@ -4,13 +4,15 @@ import SwiftFormatConfigurationTests
 import SwiftFormatCoreTests
 import SwiftFormatPrettyPrintTests
 import SwiftFormatRulesTests
+import SwiftFormatTests
 import SwiftFormatWhitespaceLinterTests
 
 var tests = [XCTestCaseEntry]()
 tests += SwiftFormatConfigurationTests.__allTests()
 tests += SwiftFormatCoreTests.__allTests()
 tests += SwiftFormatPrettyPrintTests.__allTests()
 tests += SwiftFormatRulesTests.__allTests()
+tests += SwiftFormatTests.__allTests()
 tests += SwiftFormatWhitespaceLinterTests.__allTests()
 
 XCTMain(tests)