Add PositionalArgumentsValidator (apple#101)

* Add `PositionalArgumentsValidator`

* Update tests for PositionalArgumentsValidator

* Use argument property names in PositionalArgumentsValidator.Error

Sources/ArgumentParser/Parsable Types/ParsableArgumentsValidation.swift

@@ -13,13 +13,99 @@ fileprivate protocol ParsableArgumentsValidator {
   static func validate(_ type: ParsableArguments.Type) throws
 }
 
+struct ParsableArgumentsValidationError: Error, CustomStringConvertible {
+  let parsableArgumentsType: ParsableArguments.Type
+  let underlayingErrors: [Error]
+  var description: String {
+    """
+    Validation failed for `\(parsableArgumentsType)`:
+    \(underlayingErrors.map({"- \($0)"}).joined(separator: "\n"))
+    
+    """
+  }
+}
+
 extension ParsableArguments {
   static func _validate() throws {
     let validators: [ParsableArgumentsValidator.Type] = [
+      PositionalArgumentsValidator.self,
       ParsableArgumentsCodingKeyValidator.self
     ]
-    for validator in validators {
-      try validator.validate(self)
+    let errors: [Error] = validators.compactMap { validator in
+      do {
+        try validator.validate(self)
+        return nil
+      } catch {
+        return error
+      }
+    }
+    if errors.count > 0 {
+      throw ParsableArgumentsValidationError(parsableArgumentsType: self, underlayingErrors: errors)
+    }
+  }
+}
+
+fileprivate extension ArgumentSet {
+  var firstPositionalArgument: ArgumentDefinition? {
+    switch content {
+    case .arguments(let arguments):
+      return arguments.first(where: { $0.isPositional })
+    case .sets(let sets):
+      return sets.first(where: { $0.firstPositionalArgument != nil })?.firstPositionalArgument
+    }
+  }
+
+  var firstRepeatedPositionalArgument: ArgumentDefinition? {
+    switch content {
+    case .arguments(let arguments):
+      return arguments.first(where: { $0.isRepeatingPositional })
+    case .sets(let sets):
+      return sets.first(where: { $0.firstRepeatedPositionalArgument != nil })?.firstRepeatedPositionalArgument
+    }
+  }
+}
+
+/// For positional arguments to be valid, there must be at most one
+/// positional array argument, and it must be the last positional argument
+/// in the argument list. Any other configuration leads to ambiguity in
+/// parsing the arguments.
+struct PositionalArgumentsValidator: ParsableArgumentsValidator {
+
+  struct Error: Swift.Error, CustomStringConvertible {
+    let repeatedPositionalArgument: String
+    let positionalArgumentFollowingRepeated: String
+    var description: String {
+      "Can't have a positional argument `\(positionalArgumentFollowingRepeated)` following an array of positional arguments `\(repeatedPositionalArgument)`."
+    }
+  }
+
+  static func validate(_ type: ParsableArguments.Type) throws {
+    let sets: [ArgumentSet] = Mirror(reflecting: type.init())
+      .children
+      .compactMap { child in
+        guard
+          var codingKey = child.label,
+          let parsed = child.value as? ArgumentSetProvider
+          else { return nil }
+
+        // Property wrappers have underscore-prefixed names
+        codingKey = String(codingKey.first == "_" ? codingKey.dropFirst(1) : codingKey.dropFirst(0))
+
+        let key = InputKey(rawValue: codingKey)
+        return parsed.argumentSet(for: key)
+    }
+
+    guard let repeatedPositional = sets.firstIndex(where: { $0.firstRepeatedPositionalArgument != nil })
+      else { return }
+    let positionalFollowingRepeated = sets[repeatedPositional...]
+      .dropFirst()
+      .first(where: { $0.firstPositionalArgument != nil })
+
+    if let positionalFollowingRepeated = positionalFollowingRepeated {
+      let firstRepeatedPositionalArgument: ArgumentDefinition = sets[repeatedPositional].firstRepeatedPositionalArgument!
+      let positionalFollowingRepeatedArgument: ArgumentDefinition = positionalFollowingRepeated.firstPositionalArgument!
+      throw Error(repeatedPositionalArgument: firstRepeatedPositionalArgument.help.keys.first!.rawValue,
+                  positionalArgumentFollowingRepeated: positionalFollowingRepeatedArgument.help.keys.first!.rawValue)
     }
   }
 }
@@ -59,13 +145,12 @@ struct ParsableArgumentsCodingKeyValidator: ParsableArgumentsValidator {
   /// This error indicates that an option, a flag, or an argument of
   /// a `ParsableArguments` is defined without a corresponding `CodingKey`.
   struct Error: Swift.Error, CustomStringConvertible {
-    let parsableArgumentsType: ParsableArguments.Type
     let missingCodingKeys: [String]
     var description: String {
       if missingCodingKeys.count > 1 {
-        return "Arguments \(missingCodingKeys.map({ "`\($0)`" }).joined(separator: ",")) of `\(parsableArgumentsType)` are defined without corresponding `CodingKey`s."
+        return "Arguments \(missingCodingKeys.map({ "`\($0)`" }).joined(separator: ",")) are defined without corresponding `CodingKey`s."
       } else {
-        return "Argument `\(missingCodingKeys[0])` of `\(parsableArgumentsType)` is defined without a corresponding `CodingKey`."
+        return "Argument `\(missingCodingKeys[0])` is defined without a corresponding `CodingKey`."
       }
     }
   }
@@ -91,7 +176,7 @@ struct ParsableArgumentsCodingKeyValidator: ParsableArgumentsValidator {
     } catch let result as Validator.ValidationResult {
       switch result {
       case .missingCodingKeys(let keys):
-        throw Error(parsableArgumentsType: type, missingCodingKeys: keys)
+        throw Error(missingCodingKeys: keys)
       case .success:
         break
       }

Sources/ArgumentParser/Parsing/ArgumentSet.swift

@@ -41,19 +41,11 @@ struct ArgumentSet {
   init(arguments: [ArgumentDefinition], kind: Kind) {
     self.content = .arguments(arguments)
     self.kind = kind
-    // TODO: Move this check into a separate validation pass for completed
-    // argument sets.
-    precondition(
-      self.hasValidArguments,
-      "Can't have a positional argument following an array of positional arguments.")
   }
 
   init(sets: [ArgumentSet], kind: Kind) {
     self.content = .sets(sets)
     self.kind = kind
-    precondition(
-      self.hasValidArguments,
-      "Can't have a positional argument following an array of positional arguments.")
   }
 }
 
@@ -98,64 +90,6 @@ extension ArgumentSet {
   }
 }
 
-extension ArgumentSet {
-  var hasPositional: Bool {
-    switch content {
-    case .arguments(let arguments):
-      return arguments.contains(where: { $0.isPositional })
-    case .sets(let sets):
-      return sets.contains(where: { $0.hasPositional })
-    }
-  }
-
-  var hasRepeatingPositional: Bool {
-    switch content {
-    case .arguments(let arguments):
-      return arguments.contains(where: { $0.isRepeatingPositional })
-    case .sets(let sets):
-      return sets.contains(where: { $0.hasRepeatingPositional })
-    }
-  }
-
-  /// A Boolean value indicating whether this set has valid positional
-  /// arguments.
-  ///
-  /// For positional arguments to be valid, there must be at most one
-  /// positional array argument, and it must be the last positional argument
-  /// in the argument list. Any other configuration leads to ambiguity in
-  /// parsing the arguments.
-  var hasValidArguments: Bool {
-    // Exclusive and alternative argument sets must each individually
-    // satisfy this requirement.
-    guard kind == .additive else {
-      switch content {
-      case .arguments: return true
-      case .sets(let sets): return sets.allSatisfy({ $0.hasValidArguments })
-      }
-    }
-
-    switch content {
-    case .arguments(let arguments):
-      guard let repeatedPositional = arguments.firstIndex(where: { $0.isRepeatingPositional })
-        else { return true }
-
-      let hasPositionalFollowingRepeated = arguments[repeatedPositional...]
-        .dropFirst()
-        .contains(where: { $0.isPositional })
-      return !hasPositionalFollowingRepeated
-
-    case .sets(let sets):
-      guard let repeatedPositional = sets.firstIndex(where: { $0.hasRepeatingPositional })
-        else { return true }
-      let hasPositionalFollowingRepeated = sets[repeatedPositional...]
-        .dropFirst()
-        .contains(where: { $0.hasPositional })
-
-      return !hasPositionalFollowingRepeated
-    }
-  }
-}
-
 // MARK: Flag
 
 extension ArgumentSet {

Tests/ArgumentParserUnitTests/CMakeLists.txt

@@ -1,5 +1,5 @@
 add_library(UnitTests
-  CodingKeyValidationTests.swift
+  ParsableArgumentsValidationTests.swift
   ErrorMessageTests.swift
   HelpGenerationTests.swift
   NameSpecificationTests.swift

Tests/ArgumentParserUnitTests/CodingKeyValidationTests.swift → Tests/ArgumentParserUnitTests/ParsableArgumentsValidationTests.swift

@@ -13,7 +13,7 @@ import XCTest
 import ArgumentParserTestHelpers
 @testable import ArgumentParser
 
-final class CodingKeyValidationTests: XCTestCase {
+final class ParsableArgumentsValidationTests: XCTestCase {
   private struct A: ParsableCommand {
     @Option(help: "The number of times to repeat 'phrase'.")
     var count: Int?
@@ -88,7 +88,6 @@ final class CodingKeyValidationTests: XCTestCase {
     XCTAssertThrowsError(try ParsableArgumentsCodingKeyValidator.validate(C.self)) { (error) in
       if let error = error as? ParsableArgumentsCodingKeyValidator.Error {
         XCTAssert(error.missingCodingKeys == ["count"])
-        XCTAssert(error.parsableArgumentsType.init() is C)
       } else {
         XCTFail()
       }
@@ -97,7 +96,6 @@ final class CodingKeyValidationTests: XCTestCase {
     XCTAssertThrowsError(try ParsableArgumentsCodingKeyValidator.validate(D.self)) { (error) in
       if let error = error as? ParsableArgumentsCodingKeyValidator.Error {
         XCTAssert(error.missingCodingKeys == ["phrase"])
-        XCTAssert(error.parsableArgumentsType.init() is D)
       } else {
         XCTFail()
       }
@@ -106,11 +104,93 @@ final class CodingKeyValidationTests: XCTestCase {
     XCTAssertThrowsError(try ParsableArgumentsCodingKeyValidator.validate(E.self)) { (error) in
       if let error = error as? ParsableArgumentsCodingKeyValidator.Error {
         XCTAssert(error.missingCodingKeys == ["phrase", "includeCounter"])
-        XCTAssert(error.parsableArgumentsType.init() is E)
       } else {
         XCTFail()
       }
     }
   }
 
+  private struct F: ParsableArguments {
+    @Argument()
+    var phrase: String
+
+    @Argument()
+    var items: [Int]
+  }
+
+  private struct G: ParsableArguments {
+    @Argument()
+    var items: [Int]
+
+    @Argument()
+    var phrase: String
+  }
+
+  private struct H: ParsableArguments {
+    @Argument()
+    var items: [Int]
+
+    @Option()
+    var option: Bool
+  }
+
+  private struct I: ParsableArguments {
+    @Argument()
+    var name: String
+
+    @OptionGroup()
+    var options: F
+  }
+
+  private struct J: ParsableArguments {
+    struct Options: ParsableArguments {
+      @Argument()
+      var numberOfItems: [Int]
+    }
+
+    @OptionGroup()
+    var options: Options
+
+    @Argument()
+    var phrase: String
+  }
+
+  private struct K: ParsableArguments {
+    struct Options: ParsableArguments {
+      @Argument()
+      var items: [Int]
+    }
+
+    @Argument()
+    var phrase: String
+
+    @OptionGroup()
+    var options: Options
+  }
+
+  func testPositionalArgumentsValidation() throws {
+    try PositionalArgumentsValidator.validate(A.self)
+    try PositionalArgumentsValidator.validate(F.self)
+    XCTAssertThrowsError(try PositionalArgumentsValidator.validate(G.self)) { error in
+      if let error = error as? PositionalArgumentsValidator.Error {
+        XCTAssert(error.positionalArgumentFollowingRepeated == "phrase")
+        XCTAssert(error.repeatedPositionalArgument == "items")
+      } else {
+        XCTFail()
+      }
+      XCTAssert(error is PositionalArgumentsValidator.Error)
+    }
+    try PositionalArgumentsValidator.validate(H.self)
+    try PositionalArgumentsValidator.validate(I.self)
+    XCTAssertThrowsError(try PositionalArgumentsValidator.validate(J.self)) { error in
+      if let error = error as? PositionalArgumentsValidator.Error {
+        XCTAssert(error.positionalArgumentFollowingRepeated == "phrase")
+        XCTAssert(error.repeatedPositionalArgument == "numberOfItems")
+      } else {
+        XCTFail()
+      }
+      XCTAssert(error is PositionalArgumentsValidator.Error)
+    }
+    try PositionalArgumentsValidator.validate(K.self)
+  }
 }