Pass information to ParserError in order to show user a better error message (#52)

* revert some of the cleanup from 6b3a0a1

* Pass information to ParserError to diagnose mutually exclusive flags

Author: glessard

Sources/ArgumentParser/Parsable Properties/Flag.swift

@@ -222,7 +222,7 @@ extension Flag where Value: CaseIterable, Value: RawRepresentable, Value.RawValu
           switch (hasUpdated, previous, exclusivity) {
           case (true, let p?, .exclusive):
             // This value has already been set.
-            throw ParserError.duplicateExclusiveValues(previous: p.inputOrigin, duplicate: origin)
+            throw ParserError.duplicateExclusiveValues(previous: p.inputOrigin, duplicate: origin, originalInput: values.originalInput)
           case (false, _, _), (_, _, .chooseLast):
             values.set(value, forKey: key, inputOrigin: origin)
           default:

Sources/ArgumentParser/Parsing/InputOrigin.swift

@@ -16,11 +16,14 @@
 /// This is usually an index into the `SplitArguments`.
 /// In some cases it can be multiple indices.
 struct InputOrigin: Equatable, ExpressibleByArrayLiteral {
-  enum Element: Hashable {
+  enum Element: Comparable, Hashable {
     case argumentIndex(SplitArguments.Index)
   }
 
   private var _elements: Set<Element> = []
+  var elements: [Element] {
+    Array(_elements).sorted()
+  }
 
   init() {
   }
@@ -61,3 +64,12 @@ struct InputOrigin: Equatable, ExpressibleByArrayLiteral {
     _elements.forEach(closure)
   }
 }
+
+extension InputOrigin.Element {
+  static func < (lhs: Self, rhs: Self) -> Bool {
+    switch (lhs, rhs) {
+    case (.argumentIndex(let l), .argumentIndex(let r)):
+      return l < r
+    }
+  }
+}

Sources/ArgumentParser/Parsing/ParserError.swift

@@ -21,7 +21,7 @@ enum ParserError: Error {
   case missingValueForOption(InputOrigin, Name)
   case unexpectedValueForOption(InputOrigin.Element, Name, String)
   case unexpectedExtraValues([(InputOrigin, String)])
-  case duplicateExclusiveValues(previous: InputOrigin, duplicate: InputOrigin)
+  case duplicateExclusiveValues(previous: InputOrigin, duplicate: InputOrigin, originalInput: [String])
   /// We need a value for the given key, but it’s not there. Some non-optional option or argument is missing.
   case noValue(forKey: InputKey)
   case unableToParseValue(InputOrigin, Name?, String, forKey: InputKey)

Sources/ArgumentParser/Usage/UsageGenerator.swift

@@ -188,8 +188,8 @@ extension ErrorMessageGenerator {
       return unexpectedValueForOptionMessage(origin: o, name: n, value: v)
     case .unexpectedExtraValues(let v):
       return unexpectedExtraValuesMessage(values: v)
-    case .duplicateExclusiveValues(previous: let previous, duplicate: let duplicate):
-      return duplicateExclusiveValues(previous: previous, duplicate: duplicate)
+    case .duplicateExclusiveValues(previous: let previous, duplicate: let duplicate, originalInput: let arguments):
+      return duplicateExclusiveValues(previous: previous, duplicate: duplicate, arguments: arguments)
     case .noValue(forKey: let k):
       return noValueMessage(key: k)
     case .unableToParseValue(let o, let n, let v, forKey: let k):
@@ -301,8 +301,23 @@ extension ErrorMessageGenerator {
     }
   }
 
-  func duplicateExclusiveValues(previous: InputOrigin, duplicate: InputOrigin) -> String? {
-    return "Value at position \(duplicate) has already been set from value at position \(previous)."
+  func duplicateExclusiveValues(previous: InputOrigin, duplicate: InputOrigin, arguments: [String]) -> String? {
+    func elementString(_ origin: InputOrigin, _ arguments: [String]) -> String? {
+      guard case .argumentIndex(let split) = origin.elements.first else { return nil }
+      var argument = "\'\(arguments[split.inputIndex.rawValue])\'"
+      if case let .sub(offsetIndex) = split.subIndex {
+        let stringIndex = argument.index(argument.startIndex, offsetBy: offsetIndex+2)
+        argument = "\'\(argument[stringIndex])\' in \(argument)"
+      }
+      return "flag \(argument)"
+    }
+
+    // Note that the RHS of these coalescing operators cannot be reached at this time.
+    let dupeString = elementString(duplicate, arguments) ?? "position \(duplicate)"
+    let origString = elementString(previous, arguments) ?? "position \(previous)"
+
+    //TODO: review this message once environment values are supported.
+    return "Value to be set with \(dupeString) had already been set with \(origString)"
   }
 
   func noValueMessage(key: InputKey) -> String? {

Tests/UnitTests/ErrorMessageTests.swift

@@ -134,3 +134,19 @@ extension ErrorMessageTests {
     AssertErrorMessage(Qwz.self, ["-x"], "Unknown option '-x'")
   }
 }
+
+private enum OutputBehaviour: String, CaseIterable { case stats, count, list }
+private struct Options: ParsableArguments {
+  @Flag(name: .shortAndLong, help: "Program output")
+  var behaviour: OutputBehaviour
+
+  @Flag() var bool: Bool
+}
+
+extension ErrorMessageTests {
+  func testDuplicateFlags() {
+    AssertErrorMessage(Options.self, ["--list", "--bool", "-s"], "Value to be set with flag \'-s\' had already been set with flag \'--list\'")
+    AssertErrorMessage(Options.self, ["-cbl"], "Value to be set with flag \'l\' in \'-cbl\' had already been set with flag \'c\' in \'-cbl\'")
+    AssertErrorMessage(Options.self, ["-bc", "--stats", "-l"], "Value to be set with flag \'--stats\' had already been set with flag \'c\' in \'-bc\'")
+  }
+}