Merge pull request #81350 from hamishknight/cmd-prefix

[xcodegen] A couple of command-line handling tweaks

Author: hamishknight

utils/swift-xcodegen/Sources/SwiftXcodeGen/BuildArgs/ClangBuildArgsProvider.swift

@@ -50,25 +50,32 @@ struct ClangBuildArgsProvider {
       commandsToAdd[relFilePath] = (output, command.command.args)
     }
     for (path, (output, commandArgs)) in commandsToAdd {
-      // Only include arguments that have known flags.
-      args.insert(commandArgs.filter({ $0.flag != nil }), for: path)
+      let commandArgs = commandArgs.filter { arg in
+        // Only include arguments that have known flags.
+        // Ignore `-fdiagnostics-color`, we don't want ANSI escape sequences
+        // in Xcode build logs.
+        guard let flag = arg.flag, flag != .fDiagnosticsColor else {
+          return false
+        }
+        return true
+      }
+      args.insert(commandArgs, for: path)
       outputs[path] = output
     }
   }
 
   /// Retrieve the arguments at a given path, including those in the parent.
   func getArgs(for path: RelativePath) -> BuildArgs {
-    // Sort the arguments to get a deterministic ordering.
     // FIXME: We ought to get the command from the arg tree.
-    .init(for: .clang, args: args.getArgs(for: path).sorted())
+    .init(for: .clang, args: args.getArgs(for: path))
   }
 
   /// Retrieve the arguments at a given path, excluding those already covered
   /// by a parent.
   func getUniqueArgs(
     for path: RelativePath, parent: RelativePath, infer: Bool = false
   ) -> BuildArgs {
-    var fileArgs: Set<Command.Argument> = []
+    var fileArgs: [Command.Argument] = []
     if hasBuildArgs(for: path) {
       fileArgs = args.getUniqueArgs(for: path, parent: parent)
     } else if infer {
@@ -78,14 +85,8 @@ struct ClangBuildArgsProvider {
         fileArgs = args.getUniqueArgs(for: component, parent: parent)
       }
     }
-    // Sort the arguments to get a deterministic ordering.
     // FIXME: We ought to get the command from the arg tree.
-    return .init(for: .clang, args: fileArgs.sorted())
-  }
-
-  /// Whether the given path has any unique args not covered by `parent`.
-  func hasUniqueArgs(for path: RelativePath, parent: RelativePath) -> Bool {
-    args.hasUniqueArgs(for: path, parent: parent)
+    return .init(for: .clang, args: fileArgs)
   }
 
   /// Whether the given file has build arguments.

utils/swift-xcodegen/Sources/SwiftXcodeGen/BuildArgs/CommandArgTree.swift

@@ -11,49 +11,43 @@
 //===----------------------------------------------------------------------===//
 
 /// A tree of compile command arguments, indexed by path such that those unique
-/// to a particular file can be queried, with common arguments associated
-/// with a common parent.
+/// to a particular file can be queried, with common-prefixed arguments
+/// associated with a common parent.
 struct CommandArgTree {
-  private var storage: [RelativePath: Set<Command.Argument>]
+  private var storage: [RelativePath: [Command.Argument]]
 
   init() {
     self.storage = [:]
   }
 
   mutating func insert(_ args: [Command.Argument], for path: RelativePath) {
-    let args = Set(args)
     for component in path.stackedComponents {
       // If we haven't added any arguments, add them. If we're adding arguments
       // for the file itself, this is the only way we'll add arguments,
-      // otherwise we can form an intersection with the other arguments.
+      // otherwise we can form a common prefix with the other arguments.
       let inserted = storage.insertValue(args, for: component)
       guard !inserted && component != path else { continue }
 
-      // We use subscript(_:default:) to mutate in-place without CoW.
-      storage[component, default: []].formIntersection(args)
+      // We use withValue(for:default:) to mutate in-place without CoW.
+      storage.withValue(for: component, default: []) { existingArgs in
+        let slice = existingArgs.commonPrefix(with: args)
+        existingArgs.removeSubrange(slice.endIndex...)
+      }
     }
   }
 
   /// Retrieve the arguments at a given path, including those in the parent.
-  func getArgs(for path: RelativePath) -> Set<Command.Argument> {
+  func getArgs(for path: RelativePath) -> [Command.Argument] {
     storage[path] ?? []
   }
 
   /// Retrieve the arguments at a given path, excluding those already covered
   /// by a given parent.
   func getUniqueArgs(
     for path: RelativePath, parent: RelativePath
-  ) -> Set<Command.Argument> {
-    getArgs(for: path).subtracting(getArgs(for: parent))
-  }
-
-  /// Whether the given path has any unique args not covered by `parent`.
-  func hasUniqueArgs(for path: RelativePath, parent: RelativePath) -> Bool {
-    let args = getArgs(for: path)
-    guard !args.isEmpty else { return false }
-    // Assuming `parent` is an ancestor of path, the arguments for parent is
-    // guaranteed to be a subset of the arguments for `path`. As such, we
-    // only have to compare sizes here.
-    return args.count != getArgs(for: parent).count
+  ) -> [Command.Argument] {
+    let childArgs = getArgs(for: path)
+    let parentArgs = getArgs(for: parent)
+    return Array(childArgs[parentArgs.count...])
   }
 }

utils/swift-xcodegen/Sources/SwiftXcodeGen/BuildArgs/SwiftTargets.swift

@@ -17,7 +17,6 @@ struct SwiftTargets {
   private var dependenciesByTargetName: [String: Set<String>] = [:]
   private var targetsByName: [String: SwiftTarget] = [:]
   private var targetsByOutput: [String: SwiftTarget] = [:]
-  private var addedFiles: Set<RelativePath> = []
 
   // Track some state for debugging
   private var debugLogUnknownFlags: Set<String> = []
@@ -204,22 +203,10 @@ struct SwiftTargets {
     var buildRule: SwiftTarget.BuildRule?
     var emitModuleRule: SwiftTarget.EmitModuleRule?
     if forBuild && !repoSources.isEmpty {
-      // Bail if we've already recorded a target with one of these inputs.
-      // TODO: Attempt to merge?
-      // TODO: Should we be doing this later?
-      for input in repoSources {
-        guard addedFiles.insert(input).inserted else {
-          log.debug("""
-            ! Skipping '\(name)' with output '\(primaryOutput)'; \
-            contains input '\(input)' already added
-            """)
-          return
-        }
-      }
       // We've already ensured that `repoSources` is non-empty.
-      let parent = repoSources.commonAncestor!
       buildRule = .init(
-        parentPath: parent, sources: sources, buildArgs: buildArgs
+        parentPath: repoSources.commonAncestor!, sources: sources,
+        buildArgs: buildArgs
       )
     }
     if forModule {

utils/swift-xcodegen/Sources/SwiftXcodeGen/Command/Command.swift

@@ -240,47 +240,3 @@ extension Command.Flag: CustomStringConvertible {
 extension Command.Option: CustomStringConvertible {
   var description: String { printed }
 }
-
-// MARK: Comparable
-// We sort the resulting command-line arguments to ensure deterministic
-// ordering.
-
-extension Command.Flag: Comparable {
-  static func < (lhs: Self, rhs: Self) -> Bool {
-    guard lhs.dash == rhs.dash else {
-      return lhs.dash < rhs.dash
-    }
-    return lhs.name.rawValue < rhs.name.rawValue
-  }
-}
-
-extension Command.Option: Comparable {
-  static func < (lhs: Self, rhs: Self) -> Bool {
-    guard lhs.flag == rhs.flag else {
-      return lhs.flag < rhs.flag
-    }
-    guard lhs.spacing == rhs.spacing else {
-      return lhs.spacing < rhs.spacing
-    }
-    return lhs.value < rhs.value
-  }
-}
-
-extension Command.Argument: Comparable {
-  static func < (lhs: Self, rhs: Self) -> Bool {
-    switch (lhs, rhs) {
-    // Sort flags < options < values
-    case (.flag, .option):  true
-    case (.flag, .value):   true
-    case (.option, .value): true
-
-    case (.option, .flag):  false
-    case (.value, .flag):   false
-    case (.value, .option): false
-
-    case (.flag(let lhs), .flag(let rhs)):     lhs < rhs
-    case (.option(let lhs), .option(let rhs)): lhs < rhs
-    case (.value(let lhs), .value(let rhs)):   lhs < rhs
-    }
-  }
-}

utils/swift-xcodegen/Sources/SwiftXcodeGen/Command/CommandParser.swift

@@ -13,6 +13,7 @@
 struct CommandParser {
   private var input: ByteScanner
   private var knownCommand: KnownCommand?
+  private var stopAtShellOperator = false
 
   private init(_ input: UnsafeBufferPointer<UInt8>) {
     self.input = ByteScanner(input)
@@ -26,6 +27,8 @@ struct CommandParser {
     }
   }
 
+  /// Parse an arbitrary shell command, returning the first single invocation
+  /// of a known command.
   static func parseKnownCommandOnly(_ input: String) throws -> Command? {
     var input = input
     return try input.withUTF8 { bytes in
@@ -35,6 +38,9 @@ struct CommandParser {
       ) else {
         return nil
       }
+      // We're parsing an arbitrary shell command so stop if we hit a shell
+      // operator like '&&'
+      parser.stopAtShellOperator = true
       return Command(executable: executable, args: try parser.consumeArguments())
     }
   }
@@ -62,7 +68,7 @@ struct CommandParser {
   ) throws -> AnyPath? {
     var executable: AnyPath
     repeat {
-      guard let executableUTF8 = try input.consumeElement() else {
+      guard let executableUTF8 = try consumeElement() else {
         return nil
       }
       executable = AnyPath(String(utf8: executableUTF8))
@@ -119,17 +125,27 @@ fileprivate extension ByteScanner.Consumer {
   }
 }
 
-fileprivate extension ByteScanner {
-  mutating func consumeElement() throws -> Bytes? {
+extension CommandParser {
+  mutating func consumeElement() throws -> ByteScanner.Bytes? {
     // Eat any leading whitespace.
-    skip(while: \.isSpaceOrTab)
+    input.skip(while: \.isSpaceOrTab)
 
     // If we're now at the end of the input, nothing can be parsed.
-    guard hasInput else { return nil }
-
-    // Consume the element, stopping at the first space.
-    return try consume(using: { consumer in
-      switch consumer.peek {
+    guard input.hasInput else { return nil }
+
+    // Consume the element, stopping at the first space or shell operator.
+    let start = input.cursor
+    let elt = try input.consume(using: { consumer in
+      guard let char = consumer.peek else { return false }
+      if stopAtShellOperator {
+        switch char {
+        case "<", ">", "(", ")", "|", "&", ";":
+          return false
+        default:
+          break
+        }
+      }
+      switch char {
       case \.isSpaceOrTab:
         return false
       case "\"":
@@ -139,6 +155,9 @@ fileprivate extension ByteScanner {
         return consumer.consumeUnescaped()
       }
     })
+    // Note that we may have an empty element while still moving the cursor
+    // for e.g '-I ""', which is an option with an empty value.
+    return start != input.cursor ? elt : nil
   }
 }
 
@@ -167,7 +186,7 @@ extension CommandParser {
       return makeOption(spacing: .unspaced, String(utf8: option.remaining))
     }
     if spacing.contains(.spaced), !option.hasInput,
-       let value = try input.consumeElement() {
+       let value = try consumeElement() {
       return makeOption(spacing: .spaced, String(utf8: value))
     }
     return option.empty ? .flag(flag) : nil
@@ -188,7 +207,7 @@ extension CommandParser {
   }
 
   mutating func consumeArgument() throws -> Command.Argument? {
-    guard let element = try input.consumeElement() else { return nil }
+    guard let element = try consumeElement() else { return nil }
     return try element.withUnsafeBytes { bytes in
       var option = ByteScanner(bytes)
       var numDashes = 0

utils/swift-xcodegen/Sources/SwiftXcodeGen/Command/KnownCommand.swift

@@ -128,6 +128,7 @@ extension Command.Flag {
   static let isystem = dash("isystem")
   static let isysroot = dash("isysroot")
   static let f = dash("f")
+  static let fDiagnosticsColor = dash("fdiagnostics-color")
   static let U = dash("U")
   static let W = dash("W")
   static let std = dash("std")
@@ -211,6 +212,8 @@ extension KnownCommand {
     // FIXME: We ought to see if we can get away with preserving unknown flags.
     .init(.f, option: .unspaced),
 
+    .init(.fDiagnosticsColor),
+
     // FIXME: Really we ought to map to Xcode's SDK
     .init(.isystem, option: .unspaced, .spaced),
     .init(.isysroot, option: .unspaced, .spaced),

utils/swift-xcodegen/Sources/SwiftXcodeGen/Utils.swift

@@ -34,6 +34,18 @@ extension Sequence {
   }
 }
 
+extension Collection where Element: Equatable {
+  func commonPrefix(with other: some Collection<Element>) -> SubSequence {
+    var (i, j) = (self.startIndex, other.startIndex)
+    while i < self.endIndex, j < other.endIndex {
+      guard self[i] == other[j] else { break }
+      self.formIndex(after: &i)
+      other.formIndex(after: &j)
+    }
+    return self[..<i]
+  }
+}
+
 extension String {
   init(utf8 buffer: UnsafeRawBufferPointer) {
     guard !buffer.isEmpty else {

utils/swift-xcodegen/Tests/SwiftXcodeGenTest/CompileCommandsTests.swift

@@ -54,6 +54,31 @@ class CompileCommandsTests: XCTestCase {
       knownCommandOnly: true
     )
 
+    for op in ["&&", "||", ">", "<", ">>", ";", "(", ")"] {
+      assertParse(
+        "x y x/y/clang -DX -I \(op) ignored",
+        executable: "x/y/clang",
+        args: [.option(.D, spacing: .unspaced, value: "X"), .flag(.I)],
+        knownCommandOnly: true
+      )
+      assertParse(
+        "x y x/y/clang -DX -I x\(op) ignored",
+        executable: "x/y/clang",
+        args: [
+          .option(.D, spacing: .unspaced, value: "X"),
+          .option(.I, spacing: .spaced, value: "x")
+        ],
+        knownCommandOnly: true
+      )
+    }
+
+    assertParse(
+      #"x/y/clang \< x\< "<""#,
+      executable: "x/y/clang",
+      args: [.value("<"), .value("x<"), .value("<")],
+      knownCommandOnly: true
+    )
+
     assertParse(
       "clang -DX -I", 
       args: [.option(.D, spacing: .unspaced, value: "X"), .flag(.I)]