Don't skip argument validation steps if an earlier step failed

Author: owenv

Sources/SwiftDriver/Driver/Driver.swift

@@ -342,12 +342,13 @@ public struct Driver {
     self.numThreads = Self.determineNumThreads(&parsedOptions, compilerMode: compilerMode, diagnosticsEngine: diagnosticEngine)
     self.numParallelJobs = Self.determineNumParallelJobs(&parsedOptions, diagnosticsEngine: diagnosticEngine, env: env)
 
-    try Self.validateWarningControlArgs(&parsedOptions)
-    try Self.validateProfilingArgs(&parsedOptions,
-                                   fileSystem: fileSystem,
-                                   workingDirectory: workingDirectory)
-    try Self.validateCompilationConditionArgs(&parsedOptions)
-    try Self.validateFrameworkSearchPathArgs(&parsedOptions)
+    Self.validateWarningControlArgs(&parsedOptions, diagnosticEngine: diagnosticEngine)
+    Self.validateProfilingArgs(&parsedOptions,
+                               fileSystem: fileSystem,
+                               workingDirectory: workingDirectory,
+                               diagnosticEngine: diagnosticEngine)
+    Self.validateCompilationConditionArgs(&parsedOptions, diagnosticEngine: diagnosticEngine)
+    Self.validateFrameworkSearchPathArgs(&parsedOptions, diagnosticEngine: diagnosticEngine)
     Self.validateCoverageArgs(&parsedOptions, diagnosticsEngine: diagnosticEngine)
     try toolchain.validateArgs(&parsedOptions,
                                targetTriple: self.frontendTargetInfo.target.triple,
@@ -1578,50 +1579,52 @@ extension Diagnostic.Message {
 
 // MARK: Miscellaneous Argument Validation
 extension Driver {
-  static func validateWarningControlArgs(_ parsedOptions: inout ParsedOptions) throws {
+  static func validateWarningControlArgs(_ parsedOptions: inout ParsedOptions,
+                                         diagnosticEngine: DiagnosticsEngine) {
     if parsedOptions.hasArgument(.suppressWarnings) &&
         parsedOptions.hasFlag(positive: .warningsAsErrors, negative: .noWarningsAsErrors, default: false) {
-      throw Error.conflictingOptions(.warningsAsErrors, .suppressWarnings)
+      diagnosticEngine.emit(Error.conflictingOptions(.warningsAsErrors, .suppressWarnings))
     }
   }
 
   static func validateProfilingArgs(_ parsedOptions: inout ParsedOptions,
                                     fileSystem: FileSystem,
-                                    workingDirectory: AbsolutePath?) throws {
+                                    workingDirectory: AbsolutePath?,
+                                    diagnosticEngine: DiagnosticsEngine) {
     if parsedOptions.hasArgument(.profileGenerate) &&
         parsedOptions.hasArgument(.profileUse) {
-      throw Error.conflictingOptions(.profileGenerate, .profileUse)
+      diagnosticEngine.emit(Error.conflictingOptions(.profileGenerate, .profileUse))
     }
 
     if let profileArgs = parsedOptions.getLastArgument(.profileUse)?.asMultiple,
        let workingDirectory = workingDirectory ?? fileSystem.currentWorkingDirectory {
       for profilingData in profileArgs {
-        guard fileSystem.exists(AbsolutePath(profilingData,
-                                             relativeTo: workingDirectory)) else {
-          throw Error.missingProfilingData(profilingData)
+        if !fileSystem.exists(AbsolutePath(profilingData,
+                                           relativeTo: workingDirectory)) {
+          diagnosticEngine.emit(Error.missingProfilingData(profilingData))
         }
       }
     }
   }
 
-  static func validateCompilationConditionArgs(_ parsedOptions: inout ParsedOptions) throws {
+  static func validateCompilationConditionArgs(_ parsedOptions: inout ParsedOptions,
+                                               diagnosticEngine: DiagnosticsEngine) {
     for arg in parsedOptions.arguments(for: .D).map(\.argument.asSingle) {
-      guard !arg.contains("=") else {
-        throw Error.cannotAssignToConditionalCompilationFlag(arg)
-      }
-      guard !arg.hasPrefix("-D") else {
-        throw Error.conditionalCompilationFlagHasRedundantPrefix(arg)
-      }
-      guard arg.sd_isSwiftIdentifier else {
-        throw Error.conditionalCompilationFlagIsNotValidIdentifier(arg)
+      if arg.contains("=") {
+        diagnosticEngine.emit(Error.cannotAssignToConditionalCompilationFlag(arg))
+      } else if arg.hasPrefix("-D") {
+        diagnosticEngine.emit(Error.conditionalCompilationFlagHasRedundantPrefix(arg))
+      } else if !arg.sd_isSwiftIdentifier {
+        diagnosticEngine.emit(Error.conditionalCompilationFlagIsNotValidIdentifier(arg))
       }
     }
   }
 
-  static func validateFrameworkSearchPathArgs(_ parsedOptions: inout ParsedOptions) throws {
+  static func validateFrameworkSearchPathArgs(_ parsedOptions: inout ParsedOptions,
+                                              diagnosticEngine: DiagnosticsEngine) {
     for arg in parsedOptions.arguments(for: .F, .Fsystem).map(\.argument.asSingle) {
       if arg.hasSuffix(".framework") || arg.hasSuffix(".framework/") {
-        throw Error.frameworkSearchPathIncludesExtension(arg)
+        diagnosticEngine.emit(Error.frameworkSearchPathIncludesExtension(arg))
       }
     }
   }

Sources/swift-driver/main.swift

@@ -47,6 +47,10 @@ do {
   var driver = try Driver(args: arguments,
                           diagnosticsEngine: diagnosticsEngine,
                           executor: executor)
+  // FIXME: The following check should be at the end of Driver.init, but current
+  // usage of the DiagnosticVerifier in tests makes this difficult.
+  guard !driver.diagnosticEngine.hasErrors else { throw Diagnostics.fatalError }
+
   let jobs = try driver.planBuild()
   try driver.run(jobs: jobs)
 

Tests/SwiftDriverTests/Helpers/AssertDiagnostics.swift

@@ -13,6 +13,7 @@
 import XCTest
 import SwiftDriver
 import TSCBasic
+import TSCUtility
 
 func assertDriverDiagnostics(
   args: [String],

Tests/SwiftDriverTests/SwiftDriverTests.swift

@@ -1665,61 +1665,67 @@ final class SwiftDriverTests: XCTestCase {
   }
 
   func testProfileArgValidation() throws {
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-profile-generate", "-profile-use=profile.profdata"])) {
-      XCTAssertEqual($0 as? Driver.Error, .conflictingOptions(.profileGenerate, .profileUse))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-profile-generate", "-profile-use=profile.profdata"]) {
+      $1.expect(.error(Driver.Error.conflictingOptions(.profileGenerate, .profileUse)))
+      $1.expect(.error(Driver.Error.missingProfilingData("profile.profdata")))
     }
 
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-profile-use=profile.profdata"])) {
-      XCTAssertEqual($0 as? Driver.Error, .missingProfilingData("profile.profdata"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-profile-use=profile.profdata"]) {
+      $1.expect(.error(Driver.Error.missingProfilingData("profile.profdata")))
     }
 
     try withTemporaryDirectory { path in
       try localFileSystem.writeFileContents(path.appending(component: "profile.profdata"), bytes: .init())
-      XCTAssertNoThrow(try Driver(args: ["swiftc", "-working-directory", path.pathString, "foo.swift", "-profile-use=profile.profdata"]))
+      try assertNoDriverDiagnostics(args: "swiftc", "-working-directory", path.pathString, "foo.swift", "-profile-use=profile.profdata")
     }
 
     try withTemporaryDirectory { path in
       try localFileSystem.writeFileContents(path.appending(component: "profile.profdata"), bytes: .init())
-      XCTAssertThrowsError(try Driver(args: ["swiftc", "-working-directory", path.pathString, "foo.swift",
-                                             "-profile-use=profile.profdata,profile2.profdata"])) {
-        guard case Driver.Error.missingProfilingData = $0 else {
-          XCTFail()
-          return
-        }
+      try assertDriverDiagnostics(args: ["swiftc", "-working-directory", path.pathString, "foo.swift",
+                                         "-profile-use=profile.profdata,profile2.profdata"]) {
+        $1.expect(.error(Driver.Error.missingProfilingData(path.appending(component: "profile2.profdata").pathString)))
       }
     }
   }
 
   func testConditionalCompilationArgValidation() throws {
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-DFOO=BAR"])) {
-      XCTAssertEqual($0 as? Driver.Error, .cannotAssignToConditionalCompilationFlag("FOO=BAR"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-DFOO=BAR"]) {
+      $1.expect(.error(Driver.Error.cannotAssignToConditionalCompilationFlag("FOO=BAR")))
     }
 
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-D-DFOO"])) {
-      XCTAssertEqual($0 as? Driver.Error, .conditionalCompilationFlagHasRedundantPrefix("-DFOO"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-D-DFOO"]) {
+      $1.expect(.error(Driver.Error.conditionalCompilationFlagHasRedundantPrefix("-DFOO")))
     }
 
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-Dnot-an-identifier"])) {
-      XCTAssertEqual($0 as? Driver.Error, .conditionalCompilationFlagIsNotValidIdentifier("not-an-identifier"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-Dnot-an-identifier"]) {
+      $1.expect(.error(Driver.Error.conditionalCompilationFlagIsNotValidIdentifier("not-an-identifier")))
     }
 
-    XCTAssertNoThrow(try Driver(args: ["swiftc", "foo.swift", "-DFOO"]))
+    try assertNoDriverDiagnostics(args: "swiftc", "foo.swift", "-DFOO")
   }
 
   func testFrameworkSearchPathArgValidation() throws {
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-F/some/dir/xyz.framework"])) {
-      XCTAssertEqual($0 as? Driver.Error, .frameworkSearchPathIncludesExtension("/some/dir/xyz.framework"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-F/some/dir/xyz.framework"]) {
+      $1.expect(.error(Driver.Error.frameworkSearchPathIncludesExtension("/some/dir/xyz.framework")))
     }
 
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-F/some/dir/xyz.framework/"])) {
-      XCTAssertEqual($0 as? Driver.Error, .frameworkSearchPathIncludesExtension("/some/dir/xyz.framework/"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-F/some/dir/xyz.framework/"]) {
+      $1.expect(.error(Driver.Error.frameworkSearchPathIncludesExtension("/some/dir/xyz.framework/")))
     }
 
-    XCTAssertThrowsError(try Driver(args: ["swiftc", "foo.swift", "-Fsystem", "/some/dir/xyz.framework"])) {
-      XCTAssertEqual($0 as? Driver.Error, .frameworkSearchPathIncludesExtension("/some/dir/xyz.framework"))
+    try assertDriverDiagnostics(args: ["swiftc", "foo.swift", "-Fsystem", "/some/dir/xyz.framework"]) {
+      $1.expect(.error(Driver.Error.frameworkSearchPathIncludesExtension("/some/dir/xyz.framework")))
     }
 
-    XCTAssertNoThrow(try Driver(args: ["swiftc", "foo.swift", "-Fsystem", "/some/dir/"]))
+   try assertNoDriverDiagnostics(args: "swiftc", "foo.swift", "-Fsystem", "/some/dir/")
+  }
+
+  func testMultipleValidationFailures() throws {
+    try assertDiagnostics { engine, verifier in
+      verifier.expect(.error(Driver.Error.conditionalCompilationFlagIsNotValidIdentifier("not-an-identifier")))
+      verifier.expect(.error(Driver.Error.frameworkSearchPathIncludesExtension("/some/dir/xyz.framework")))
+      _ = try Driver(args: ["swiftc", "foo.swift", "-Dnot-an-identifier", "-F/some/dir/xyz.framework"], diagnosticsEngine: engine)
+    }
   }
 
   // Test cases ported from Driver/macabi-environment.swift
@@ -2368,8 +2374,8 @@ final class SwiftDriverTests: XCTestCase {
     }
 
     do {
-      XCTAssertThrowsError(try Driver(args: ["swift", "-no-warnings-as-errors", "-warnings-as-errors", "-suppress-warnings", "foo.swift"])) {
-        XCTAssertEqual($0 as? Driver.Error, Driver.Error.conflictingOptions(.warningsAsErrors, .suppressWarnings))
+      try assertDriverDiagnostics(args: ["swift", "-no-warnings-as-errors", "-warnings-as-errors", "-suppress-warnings", "foo.swift"]) {
+        $1.expect(.error(Driver.Error.conflictingOptions(.warningsAsErrors, .suppressWarnings)))
       }
     }