From b66f637363a6b5577c8a3e3da5ba0d4187d6b824 Mon Sep 17 00:00:00 2001 From: Nate Cook Date: Sat, 21 Mar 2020 13:26:59 -0500 Subject: [PATCH] Enforce that options must be used by command before matching a subcommand We were incorrectly skipping over dash-prefixed inputs when looking for the next subcommand. This means that input like `command sub1 --foo sub2` would match the sub1 and sub2 subcommands, even if `--foo` wasn't defined by sub1. This manifested in issues where a value expected by `--foo` would be eaten by the subcommand matcher. Fixes #92. --- .../Parsing/CommandParser.swift | 14 ++-- .../DefaultSubcommandEndToEndTests.swift | 71 +++++++++++++++++++ .../NestedCommandEndToEndTests.swift | 41 +++++------ 3 files changed, 96 insertions(+), 30 deletions(-) create mode 100644 Tests/ArgumentParserEndToEndTests/DefaultSubcommandEndToEndTests.swift diff --git a/Sources/ArgumentParser/Parsing/CommandParser.swift b/Sources/ArgumentParser/Parsing/CommandParser.swift index b4bd60652..8403c9fdd 100644 --- a/Sources/ArgumentParser/Parsing/CommandParser.swift +++ b/Sources/ArgumentParser/Parsing/CommandParser.swift @@ -52,13 +52,13 @@ extension CommandParser { /// - Returns: A node for the matched subcommand if one was found; /// otherwise, `nil`. fileprivate func consumeNextCommand(split: inout SplitArguments) -> Tree? { - if let command = split.peekNextValue() { - if let subcommandNode = currentNode.firstChild(withName: command.1) { - _ = split.popNextValue() - return subcommandNode - } - } - return nil + guard let (origin, element) = split.peekNext(), + element.isValue, + let value = split.originalInput(at: origin), + let subcommandNode = currentNode.firstChild(withName: value) + else { return nil } + _ = split.popNextValue() + return subcommandNode } /// Throws a `HelpRequested` error if the user has specified either of the diff --git a/Tests/ArgumentParserEndToEndTests/DefaultSubcommandEndToEndTests.swift b/Tests/ArgumentParserEndToEndTests/DefaultSubcommandEndToEndTests.swift new file mode 100644 index 000000000..9ebd7d6c3 --- /dev/null +++ b/Tests/ArgumentParserEndToEndTests/DefaultSubcommandEndToEndTests.swift @@ -0,0 +1,71 @@ +//===----------------------------------------------------------*- swift -*-===// +// +// This source file is part of the Swift Argument Parser open source project +// +// Copyright (c) 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// +//===----------------------------------------------------------------------===// + +import XCTest +import ArgumentParserTestHelpers +import ArgumentParser + +final class DefaultSubcommandEndToEndTests: XCTestCase { +} + +// MARK: - + +private struct Main: ParsableCommand { + static var configuration = CommandConfiguration( + subcommands: [Default.self, Foo.self, Bar.self], + defaultSubcommand: Default.self + ) +} + +private struct Default: ParsableCommand { + enum Mode: String, CaseIterable, ExpressibleByArgument { + case foo, bar, baz + } + + @Option(default: .foo) var mode: Mode +} + +private struct Foo: ParsableCommand {} +private struct Bar: ParsableCommand {} + +extension DefaultSubcommandEndToEndTests { + func testDefaultSubcommand() { + AssertParseCommand(Main.self, Default.self, []) { def in + XCTAssertEqual(.foo, def.mode) + } + + AssertParseCommand(Main.self, Default.self, ["--mode=bar"]) { def in + XCTAssertEqual(.bar, def.mode) + } + + AssertParseCommand(Main.self, Default.self, ["--mode", "bar"]) { def in + XCTAssertEqual(.bar, def.mode) + } + + AssertParseCommand(Main.self, Default.self, ["--mode", "baz"]) { def in + XCTAssertEqual(.baz, def.mode) + } + } + + func testNonDefaultSubcommand() { + AssertParseCommand(Main.self, Foo.self, ["foo"]) { _ in } + AssertParseCommand(Main.self, Bar.self, ["bar"]) { _ in } + + AssertParseCommand(Main.self, Default.self, ["default", "--mode", "bar"]) { def in + XCTAssertEqual(.bar, def.mode) + } + } + + func testParsingFailure() { + XCTAssertThrowsError(try Main.parseAsRoot(["--mode", "qux"])) + XCTAssertThrowsError(try Main.parseAsRoot(["qux"])) + } +} diff --git a/Tests/ArgumentParserEndToEndTests/NestedCommandEndToEndTests.swift b/Tests/ArgumentParserEndToEndTests/NestedCommandEndToEndTests.swift index ab445effc..326654ef7 100644 --- a/Tests/ArgumentParserEndToEndTests/NestedCommandEndToEndTests.swift +++ b/Tests/ArgumentParserEndToEndTests/NestedCommandEndToEndTests.swift @@ -58,14 +58,13 @@ fileprivate func AssertParseFooCommand(_ type: A.Type, _ arguments: [String], extension NestedCommandEndToEndTests { func testParsing_package() throws { - AssertParseFooCommand(Foo.Package.Clean.self, ["package", "clean"]) { clean in - XCTAssertEqual(clean.foo.verbose, false) - XCTAssertEqual(clean.package.force, false) + AssertParseFooCommand(Foo.Package.self, ["package"]) { package in + XCTAssertFalse(package.force) } - AssertParseFooCommand(Foo.Package.Clean.self, ["-f", "package", "clean"]) { clean in + AssertParseFooCommand(Foo.Package.Clean.self, ["package", "clean"]) { clean in XCTAssertEqual(clean.foo.verbose, false) - XCTAssertEqual(clean.package.force, true) + XCTAssertEqual(clean.package.force, false) } AssertParseFooCommand(Foo.Package.Clean.self, ["package", "-f", "clean"]) { clean in @@ -98,11 +97,6 @@ extension NestedCommandEndToEndTests { XCTAssertEqual(config.package.force, true) } - AssertParseFooCommand(Foo.Package.Config.self, ["-f", "package", "config"]) { config in - XCTAssertEqual(config.foo.verbose, false) - XCTAssertEqual(config.package.force, true) - } - AssertParseFooCommand(Foo.Package.Config.self, ["package", "-v", "config", "-f"]) { config in XCTAssertEqual(config.foo.verbose, true) XCTAssertEqual(config.package.force, true) @@ -132,19 +126,20 @@ extension NestedCommandEndToEndTests { } func testParsing_fails() throws { - XCTAssertThrowsError(try Foo.parse(["package"])) - XCTAssertThrowsError(try Foo.parse(["clean", "package"])) - XCTAssertThrowsError(try Foo.parse(["config", "package"])) - XCTAssertThrowsError(try Foo.parse(["package", "c"])) - XCTAssertThrowsError(try Foo.parse(["package", "build"])) - XCTAssertThrowsError(try Foo.parse(["package", "build", "clean"])) - XCTAssertThrowsError(try Foo.parse(["package", "clean", "foo"])) - XCTAssertThrowsError(try Foo.parse(["package", "config", "bar"])) - XCTAssertThrowsError(try Foo.parse(["package", "clean", "build"])) - XCTAssertThrowsError(try Foo.parse(["build"])) - XCTAssertThrowsError(try Foo.parse(["build", "-f"])) - XCTAssertThrowsError(try Foo.parse(["build", "--build"])) - XCTAssertThrowsError(try Foo.parse(["build", "--build", "12"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["clean", "package"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["config", "package"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["package", "c"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["package", "build"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["package", "build", "clean"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["package", "clean", "foo"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["package", "config", "bar"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["package", "clean", "build"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["build"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["build", "-f"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["build", "--build"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["build", "--build", "12"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["-f", "package", "clean"])) + XCTAssertThrowsError(try Foo.parseAsRoot(["-f", "package", "config"])) } }