From bea6bdf5d1bfe6038734eca55ca3ae2ac675b266 Mon Sep 17 00:00:00 2001
From: Nate Cook <natecook@apple.com>
Date: Mon, 15 Jun 2020 14:09:51 -0500
Subject: [PATCH] Show option-based errors before unexpected positional errors

This pushes any errors indicated by unexpected arguments after parsing
out to the same late position. We were previously stopping immediately
when the command is a leaf node; that isn't necessary and created an
awkward second error path.
---
 .../ArgumentParser/Parsing/ArgumentSet.swift  | 33 ++++---------------
 .../Parsing/CommandParser.swift               | 15 ++-------
 .../ArgumentParser/Parsing/ParsedValues.swift |  5 ---
 .../ErrorMessageTests.swift                   | 16 +++++++++
 4 files changed, 24 insertions(+), 45 deletions(-)

diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift
index 031e69bd3..190c440e2 100644
--- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift
+++ b/Sources/ArgumentParser/Parsing/ArgumentSet.swift
@@ -213,7 +213,7 @@ extension ArgumentSet {
   ///
   /// - Parameter all: The input (from the command line) that needs to be parsed
   /// - Parameter commandStack: commands that have been parsed
-  func lenientParse(_ all: SplitArguments) throws -> LenientParsedValues {
+  func lenientParse(_ all: SplitArguments) throws -> ParsedValues {
     // Create a local, mutable copy of the arguments:
     var inputArguments = all
     
@@ -355,21 +355,11 @@ extension ArgumentSet {
     
     // We have parsed all non-positional values at this point.
     // Next: parse / consume the positional values.
-    do {
-      var stripped = all
-      stripped.removeAll(in: usedOrigins)
-      try parsePositionalValues(from: stripped, into: &result)
-    } catch {
-      switch error {
-      case ParserError.unexpectedExtraValues:
-        // There were more positional values than we could parse.
-        // If we‘re using subcommands, that could be expected.
-        return .partial(result, error)
-      default:
-        throw error
-      }
-    }
-    return .success(result)
+    var unusedArguments = all
+    unusedArguments.removeAll(in: usedOrigins)
+    try parsePositionalValues(from: unusedArguments, into: &result)
+    
+    return result
   }
 }
 
@@ -450,16 +440,5 @@ extension ArgumentSet {
         try update([origin], nil, value, &result)
       } while argumentDefinition.isRepeatingPositional
     }
-    
-    // Finished with the defined arguments; are there leftover values to parse?
-    skipNonValues()
-    guard argumentStack.isEmpty else {
-      let extraValues: [(InputOrigin, String)] = argumentStack
-        .map { $0.0 }
-        .map {
-          (InputOrigin(element: $0), unusedInput.originalInput(at: $0)!)
-        }
-      throw ParserError.unexpectedExtraValues(extraValues)
-    }
   }
 }
diff --git a/Sources/ArgumentParser/Parsing/CommandParser.swift b/Sources/ArgumentParser/Parsing/CommandParser.swift
index 951b4d872..1ab4ebd8c 100644
--- a/Sources/ArgumentParser/Parsing/CommandParser.swift
+++ b/Sources/ArgumentParser/Parsing/CommandParser.swift
@@ -110,19 +110,8 @@ extension CommandParser {
     // Build the argument set (i.e. information on how to parse):
     let commandArguments = ArgumentSet(currentNode.element)
     
-    // Parse the arguments into a ParsedValues:
-    let parsedResult = try commandArguments.lenientParse(split)
-    
-    let values: ParsedValues
-    switch parsedResult {
-    case .success(let v):
-      values = v
-    case .partial(let v, let e):
-      values = v
-      if currentNode.isLeaf {
-        throw e
-      }
-    }
+    // Parse the arguments, ignoring anything unexpected
+    let values = try commandArguments.lenientParse(split)
     
     // Decode the values from ParsedValues into the ParsableCommand:
     let decoder = ArgumentDecoder(values: values, previouslyDecoded: decodedArguments)
diff --git a/Sources/ArgumentParser/Parsing/ParsedValues.swift b/Sources/ArgumentParser/Parsing/ParsedValues.swift
index a8cda1baf..e5c0b6f03 100644
--- a/Sources/ArgumentParser/Parsing/ParsedValues.swift
+++ b/Sources/ArgumentParser/Parsing/ParsedValues.swift
@@ -43,11 +43,6 @@ struct ParsedValues {
   var originalInput: [String]
 }
 
-enum LenientParsedValues {
-  case success(ParsedValues)
-  case partial(ParsedValues, Swift.Error)
-}
-
 extension ParsedValues {
   mutating func set(_ new: Any, forKey key: InputKey, inputOrigin: InputOrigin) {
     set(Element(key: key, value: new, inputOrigin: inputOrigin))
diff --git a/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift b/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift
index 28c7a0031..baec3b526 100644
--- a/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift
+++ b/Tests/ArgumentParserUnitTests/ErrorMessageTests.swift
@@ -174,3 +174,19 @@ extension ErrorMessageTests {
     AssertErrorMessage(OptOptions.self, ["-cbl"], "Value to be set with flag \'l\' in \'-cbl\' had already been set with flag \'c\' in \'-cbl\'")
   }
 }
+
+// MARK: -
+
+fileprivate struct Repeat: ParsableArguments {
+  @Option() var count: Int?
+  @Argument() var phrase: String
+}
+
+extension ErrorMessageTests {
+  func testBadOptionBeforeArgument() {
+    AssertErrorMessage(
+      Repeat.self,
+      ["--cont", "5", "Hello"],
+      "Unknown option '--cont'. Did you mean '--count'?")
+  }
+}