Repository: apple/swift-argument-parser (swift)

Issue:
Repeated flag causes error in some cases
When repeating a flag defined from a mutually-exclusive set, one gets an error.
```
enum Enum: String, CaseIterable { case s, t, u }

struct Options: Parsable Command {
  @Flag(name: .short, exclusivity: .exclusive, help: "Help!")
  var flag: Enum
}

let options = Options.parseOrExit()
```
```
$ options -s -s
Error: Value to be set with flag '-s' had already been set with flag '-s'
Usage: options -s -t -u
```

Should repetition in such a case simply be ignored?

Pull request description:
<!--
    Thanks for contributing to the Swift Argument Parser!

    If this pull request adds new API, please add '?template=new.md'
    to the URL to switch to the appropriate template.

    Before you submit your request, please replace the paragraph
    below with the relevant details, and complete the steps in the
    checklist by placing an 'x' in each box:
    
    - [x] I've completed this task
    - [ ] This task isn't completed
-->

Stop failing upon seeing a repeated value from a mutually-exclusive set of flags.

This adds an `Equatable` requirement to the `Flag` constructors from `CaseIterable` enums. Given the other requirements on those enums, `Equatable` conformance is automatic and therefore costs nothing.

The switch statement in `ArgumentSet.updateFlag` is also much simplified, by calling `values.element(forKey:)` only when it is needed.

### Checklist
- [x] I've added at least one test that validates that my change is working, if appropriate
- [x] I've followed the code style of the rest of the project
- [x] I've read the [Contribution Guidelines](CONTRIBUTING.md)
- [x] I've updated the documentation if necessary

This fixes https://github.com/apple/swift-argument-parser/issues/59
This supersedes https://github.com/apple/swift-argument-parser/pull/70

Patch:
diff --git a/Sources/ArgumentParser/Parsable Properties/Flag.swift b/Sources/ArgumentParser/Parsable Properties/Flag.swift
index 39c3af5f7..95a2f963f 100644
--- a/Sources/ArgumentParser/Parsable Properties/Flag.swift
+++ b/Sources/ArgumentParser/Parsable Properties/Flag.swift
@@ -226,7 +226,7 @@ extension Flag where Value == Int {
   }
 }
 
-extension Flag where Value: CaseIterable, Value: RawRepresentable, Value.RawValue == String {
+extension Flag where Value: CaseIterable, Value: Equatable, Value: RawRepresentable, Value.RawValue == String {
   /// Creates a property that gets its value from the presence of a flag,
   /// where the allowed flags are defined by a `CaseIterable` type.
   ///
@@ -252,8 +252,6 @@ extension Flag where Value: CaseIterable, Value: RawRepresentable, Value.RawValu
         let caseKey = InputKey(rawValue: value.rawValue)
         let help = ArgumentDefinition.Help(options: initial != nil ? .isOptional : [], help: help, defaultValue: defaultValue, key: key)
         return ArgumentDefinition.flag(name: name, key: key, caseKey: caseKey, help: help, parsingStrategy: .nextAsValue, initialValue: initial, update: .nullary({ (origin, name, values) in
-          // TODO: We should catch duplicate flags that hit a single part of
-          // an exclusive argument set in the value parsing, not here.
           hasUpdated = try ArgumentSet.updateFlag(key: key, value: value, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity)
         }))
       }
@@ -279,7 +277,7 @@ extension Flag {
     name: NameSpecification = .long,
     exclusivity: FlagExclusivity = .exclusive,
     help: ArgumentHelp? = nil
-  ) where Value == Element?, Element: CaseIterable, Element: RawRepresentable, Element.RawValue == String {
+  ) where Value == Element?, Element: CaseIterable, Element: Equatable, Element: RawRepresentable, Element.RawValue == String {
     self.init(_parsedValue: .init { key in
       // This gets flipped to `true` the first time one of these flags is
       // encountered.
@@ -289,8 +287,6 @@ extension Flag {
         let caseKey = InputKey(rawValue: value.rawValue)
         let help = ArgumentDefinition.Help(options: .isOptional, help: help, key: key)
         return ArgumentDefinition.flag(name: name, key: key, caseKey: caseKey, help: help, parsingStrategy: .nextAsValue, initialValue: nil as Element?, update: .nullary({ (origin, name, values) in
-          // TODO: We should catch duplicate flags that hit a single part of
-          // an exclusive argument set in the value parsing, not here.
           hasUpdated = try ArgumentSet.updateFlag(key: key, value: value, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity)
         }))
       }
diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift
index f97468a88..b60300da1 100644
--- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift
+++ b/Sources/ArgumentParser/Parsing/ArgumentSet.swift
@@ -170,18 +170,23 @@ extension ArgumentSet {
     return ArgumentSet(arg)
   }
 
-  static func updateFlag(key: InputKey, value: Any, origin: InputOrigin, values: inout ParsedValues, hasUpdated: Bool, exclusivity: FlagExclusivity) throws -> Bool {
-    let previous = values.element(forKey: key)
-    switch (hasUpdated, previous, exclusivity) {
-    case(true, let previous?, .exclusive):
+  static func updateFlag<Value: Equatable>(key: InputKey, value: Value, origin: InputOrigin, values: inout ParsedValues, hasUpdated: Bool, exclusivity: FlagExclusivity) throws -> Bool {
+    switch (hasUpdated, exclusivity) {
+    case (true, .exclusive):
       // This value has already been set.
-      throw ParserError.duplicateExclusiveValues(previous: previous.inputOrigin, duplicate: origin, originalInput: values.originalInput)
-    case (true, _, .chooseFirst):
+      if let previous = values.element(forKey: key) {
+        if (previous.value as? Value) == value {
+          // setting the value again will consume the argument
+          values.set(value, forKey: key, inputOrigin: origin)
+        }
+        else {
+          throw ParserError.duplicateExclusiveValues(previous: previous.inputOrigin, duplicate: origin, originalInput: values.originalInput)
+        }
+      }
+    case (true, .chooseFirst):
       values.update(forKey: key, inputOrigin: origin, initial: value, closure: { _ in })
-    case (false, _, _), (_, _, .chooseLast):
+    case (false, _), (_, .chooseLast):
       values.set(value, forKey: key, inputOrigin: origin)
-    default:
-      break
     }
     return true
   }


Test patch:
diff --git a/Tests/ArgumentParserEndToEndTests/FlagsEndToEndTests.swift b/Tests/ArgumentParserEndToEndTests/FlagsEndToEndTests.swift
index 18e61a70e..e0e2ad177 100644
--- a/Tests/ArgumentParserEndToEndTests/FlagsEndToEndTests.swift
+++ b/Tests/ArgumentParserEndToEndTests/FlagsEndToEndTests.swift
@@ -25,7 +25,7 @@ fileprivate struct Bar: ParsableArguments {
   @Flag(inversion: .prefixedNo)
   var extattr: Bool
 
-  @Flag(inversion: .prefixedNo)
+  @Flag(inversion: .prefixedNo, exclusivity: .exclusive)
   var extattr2: Bool?
 
   @Flag(inversion: .prefixedEnableDisable, exclusivity: .chooseFirst)
@@ -80,6 +80,9 @@ extension FlagsEndToEndTests {
     AssertParse(Bar.self, ["--enable-logging"]) { options in
       XCTAssertEqual(options.logging, true)
     }
+    AssertParse(Bar.self, ["--no-extattr2", "--no-extattr2"]) { options in
+      XCTAssertEqual(options.extattr2, false)
+    }
     AssertParse(Bar.self, ["--disable-logging", "--enable-logging"]) { options in
       XCTAssertEqual(options.logging, false)
     }
@@ -269,6 +272,9 @@ fileprivate struct RepeatOK: ParsableArguments {
 
   @Flag(exclusivity: .chooseLast)
   var shape: Shape
+
+  @Flag(name: .shortAndLong, default: .small, exclusivity: .exclusive)
+  var size: Size
 }
 
 extension FlagsEndToEndTests {
@@ -282,5 +288,9 @@ extension FlagsEndToEndTests {
       XCTAssertEqual(options.color, .silver)
       XCTAssertEqual(options.shape, .oblong)
     }
+
+    AssertParse(RepeatOK.self, ["--large", "--pink", "--round", "-l"]) { options in
+      XCTAssertEqual(options.size, .large)
+    }
   }
 }