Merge pull request #1354 from artemcm/LibSwiftScanPathsEscape

[Dependency Scanning] Unify path-escaping handling using TSC's spm_shellEscaped

Author: artemcm

Sources/SwiftDriver/Execution/ArgsResolver.swift

@@ -57,17 +57,16 @@ public final class ArgsResolver {
     }
   }
 
-  public func resolveArgumentList(for job: Job, useResponseFiles: ResponseFileHandling = .heuristic,
-                                  quotePaths: Bool = false) throws -> [String] {
-    let (arguments, _) = try resolveArgumentList(for: job, useResponseFiles: useResponseFiles,
-                                                 quotePaths: quotePaths)
+  public func resolveArgumentList(for job: Job, useResponseFiles: ResponseFileHandling = .heuristic)
+  throws -> [String] {
+    let (arguments, _) = try resolveArgumentList(for: job, useResponseFiles: useResponseFiles)
     return arguments
   }
 
-  public func resolveArgumentList(for job: Job, useResponseFiles: ResponseFileHandling = .heuristic,
-                                  quotePaths: Bool = false) throws -> ([String], usingResponseFile: Bool) {
-    let tool = try resolve(.path(job.tool), quotePaths: quotePaths)
-    var arguments = [tool] + (try job.commandLine.map { try resolve($0, quotePaths: quotePaths) })
+  public func resolveArgumentList(for job: Job, useResponseFiles: ResponseFileHandling = .heuristic)
+  throws -> ([String], usingResponseFile: Bool) {
+    let tool = try resolve(.path(job.tool))
+    var arguments = [tool] + (try job.commandLine.map { try resolve($0) })
     let usingResponseFile = try createResponseFileIfNeeded(for: job, resolvedArguments: &arguments,
                                                            useResponseFiles: useResponseFiles)
     return (arguments, usingResponseFile)
@@ -77,46 +76,45 @@ public final class ArgsResolver {
   public func resolveArgumentList(for job: Job, forceResponseFiles: Bool,
                                   quotePaths: Bool = false) throws -> [String] {
     let useResponseFiles: ResponseFileHandling = forceResponseFiles ? .forced : .heuristic
-    return try resolveArgumentList(for: job, useResponseFiles: useResponseFiles, quotePaths: quotePaths)
+    return try resolveArgumentList(for: job, useResponseFiles: useResponseFiles)
   }
 
   @available(*, deprecated, message: "use resolveArgumentList(for:,useResponseFiles:,quotePaths:)")
   public func resolveArgumentList(for job: Job, forceResponseFiles: Bool,
                                   quotePaths: Bool = false) throws -> ([String], usingResponseFile: Bool) {
     let useResponseFiles: ResponseFileHandling = forceResponseFiles ? .forced : .heuristic
-    return try resolveArgumentList(for: job, useResponseFiles: useResponseFiles, quotePaths: quotePaths)
+    return try resolveArgumentList(for: job, useResponseFiles: useResponseFiles)
   }
 
   /// Resolve the given argument.
-  public func resolve(_ arg: Job.ArgTemplate,
-                      quotePaths: Bool = false) throws -> String {
+  public func resolve(_ arg: Job.ArgTemplate) throws -> String {
     switch arg {
     case .flag(let flag):
       return flag
 
     case .path(let path):
       return try lock.withLock {
-        return try unsafeResolve(path: path, quotePaths: quotePaths)
+        return try unsafeResolve(path: path)
       }
 
     case .responseFilePath(let path):
-      return "@\(try resolve(.path(path), quotePaths: quotePaths))"
+      return "@\(try resolve(.path(path)))"
 
     case let .joinedOptionAndPath(option, path):
-      return option + (try resolve(.path(path), quotePaths: quotePaths))
+      return option + (try resolve(.path(path)))
 
     case let .squashedArgumentList(option: option, args: args):
       return try option + args.map {
-        try resolve($0, quotePaths: quotePaths)
+        try resolve($0)
       }.joined(separator: " ")
     }
   }
 
   /// Needs to be done inside of `lock`. Marked unsafe to make that more obvious.
-  private func unsafeResolve(path: VirtualPath, quotePaths: Bool) throws -> String {
+  private func unsafeResolve(path: VirtualPath) throws -> String {
     // If there was a path mapping, use it.
     if let actualPath = pathMapping[path] {
-      return quotePaths ? quoteArgument(actualPath) : actualPath
+      return actualPath
     }
 
     // Return the path from the temporary directory if this is a temporary file.
@@ -140,21 +138,21 @@ public final class ArgsResolver {
 
       let result = actualPath.name
       pathMapping[path] = result
-      return quotePaths ? "'\(result)'" : result
+      return result
     }
 
     // Otherwise, return the path.
     let result = path.name
     pathMapping[path] = result
-    return quotePaths ? quoteArgument(result) : result
+    return result
   }
 
   private func createFileList(path: VirtualPath, contents: [VirtualPath]) throws {
     // FIXME: Need a way to support this for distributed build systems...
     if let absPath = path.absolutePath {
       try fileSystem.writeFileContents(absPath) { out in
         for path in contents {
-          try! out <<< unsafeResolve(path: path, quotePaths: false) <<< "\n"
+          try! out <<< unsafeResolve(path: path) <<< "\n"
         }
       }
     }
@@ -184,7 +182,7 @@ public final class ArgsResolver {
   }
 
   private func quoteAndEscape(path: VirtualPath) -> String {
-    let inputNode = Node.scalar(Node.Scalar(try! unsafeResolve(path: path, quotePaths: false),
+    let inputNode = Node.scalar(Node.Scalar(try! unsafeResolve(path: path),
                                             Tag(.str), .doubleQuoted))
     // Width parameter of -1 sets preferred line-width to unlimited so that no extraneous
     // line-breaks will be inserted during serialization.

Sources/SwiftDriver/ExplicitModuleBuilds/ModuleDependencyScanning.swift

@@ -415,12 +415,11 @@ public extension Driver {
 
   static func itemizedJobCommand(of job: Job, useResponseFiles: ResponseFileHandling,
                                  using resolver: ArgsResolver) throws -> [String] {
-    // FIXME: this is to walkaround rdar://108769167
-    let quotePaths = job.kind != .scanDependencies
-    let (args, _) = try resolver.resolveArgumentList(for: job,
-                                                     useResponseFiles: useResponseFiles,
-                                                     quotePaths: quotePaths)
-    return args
+    // Because the command-line passed to libSwiftScan does not go through the shell
+    // we must ensure that we generate a shell-escaped string for all arguments/flags that may
+    // potentially need it.
+    return try resolver.resolveArgumentList(for: job,
+                                            useResponseFiles: useResponseFiles).0.map { $0.spm_shellEscaped() }
   }
 
   static func getRootPath(of toolchain: Toolchain, env: [String: String])

Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift

@@ -1558,6 +1558,28 @@ final class ExplicitModuleBuildTests: XCTestCase {
     }
   }
 
+  func testDependencyScanCommandLineEscape() throws {
+#if os(Windows)
+  let quoteCharacter: Character = "\""
+#else
+  let quoteCharacter: Character = "'"
+#endif
+    let swiftInputWithSpace = "/tmp/input example/test.swift"
+    let swiftInputWithoutSpace = "/tmp/baz.swift"
+    let clangInputWithSpace = "/tmp/input example/bar.o"
+    var driver = try Driver(args: ["swiftc", "-explicit-module-build",
+                                   "-module-name", "testDependencyScanning",
+                                   swiftInputWithSpace, swiftInputWithoutSpace,
+                                   "-Xcc", clangInputWithSpace])
+    let scanJob = try driver.dependencyScanningJob()
+    let scanJobCommand = try Driver.itemizedJobCommand(of: scanJob,
+                                                       useResponseFiles: .disabled,
+                                                       using: ArgsResolver(fileSystem: InMemoryFileSystem()))
+    XCTAssertTrue(scanJobCommand.contains(String(quoteCharacter) + swiftInputWithSpace + String(quoteCharacter)))
+    XCTAssertTrue(scanJobCommand.contains(String(quoteCharacter) + clangInputWithSpace + String(quoteCharacter)))
+    XCTAssertTrue(scanJobCommand.contains(swiftInputWithoutSpace))
+  }
+
   func testExplicitSwiftModuleMap() throws {
     let jsonExample : String = """
     [