Enforce that options must be used by command before matching a subcommand (#96)

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.

Author: natecook1000

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<ParsableCommand.Type>? {
-    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

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"]))
+  }
+}

Tests/ArgumentParserEndToEndTests/NestedCommandEndToEndTests.swift

@@ -58,14 +58,13 @@ fileprivate func AssertParseFooCommand<A>(_ 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"]))
   }
 }