From 023730bdda30adc0e40a37af94ef420c26585fdd Mon Sep 17 00:00:00 2001 From: Yu Ao Date: Mon, 30 Mar 2020 03:57:50 +0800 Subject: [PATCH] Add `PositionalArgumentsValidator` (#101) * Add `PositionalArgumentsValidator` * Update tests for PositionalArgumentsValidator * Use argument property names in PositionalArgumentsValidator.Error --- .../ParsableArgumentsValidation.swift | 97 +++++++++++++++++-- .../ArgumentParser/Parsing/ArgumentSet.swift | 66 ------------- Tests/ArgumentParserUnitTests/CMakeLists.txt | 2 +- ...=> ParsableArgumentsValidationTests.swift} | 88 ++++++++++++++++- 4 files changed, 176 insertions(+), 77 deletions(-) rename Tests/ArgumentParserUnitTests/{CodingKeyValidationTests.swift => ParsableArgumentsValidationTests.swift} (58%) diff --git a/Sources/ArgumentParser/Parsable Types/ParsableArgumentsValidation.swift b/Sources/ArgumentParser/Parsable Types/ParsableArgumentsValidation.swift index a504500ca..919709bc2 100644 --- a/Sources/ArgumentParser/Parsable Types/ParsableArgumentsValidation.swift +++ b/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 } diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift index 83c12542e..745bae98a 100644 --- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift +++ b/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 { diff --git a/Tests/ArgumentParserUnitTests/CMakeLists.txt b/Tests/ArgumentParserUnitTests/CMakeLists.txt index f42a00e46..2599ff895 100644 --- a/Tests/ArgumentParserUnitTests/CMakeLists.txt +++ b/Tests/ArgumentParserUnitTests/CMakeLists.txt @@ -1,5 +1,5 @@ add_library(UnitTests - CodingKeyValidationTests.swift + ParsableArgumentsValidationTests.swift ErrorMessageTests.swift HelpGenerationTests.swift NameSpecificationTests.swift diff --git a/Tests/ArgumentParserUnitTests/CodingKeyValidationTests.swift b/Tests/ArgumentParserUnitTests/ParsableArgumentsValidationTests.swift similarity index 58% rename from Tests/ArgumentParserUnitTests/CodingKeyValidationTests.swift rename to Tests/ArgumentParserUnitTests/ParsableArgumentsValidationTests.swift index 72cff19a2..14cc86daa 100644 --- a/Tests/ArgumentParserUnitTests/CodingKeyValidationTests.swift +++ b/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) + } }