Merge pull request swiftlang#144 from dylansturg/safer_unknown_nodes

Reject invalid files & use the same DiagnosticEngine for all diagnostics

Author: allevato

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)