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

Issue:
Exclusivity isn't adjustable for Boolean flags with flag inversion settings
```
struct Options: ParsableCommand {
  @Flag(inversion: .prefixedNo)
  var x: Bool
}
options = Options.parseOrExit()
print(options.x)
```

```
$ options --x --no-x
false
$ options --no-x --x
true
```

Implicitly, they behave as if one had passed `FlagExclusivity.chooseLast`, but this is not adjustable.

Pull request:
<!--
    Thanks for contributing to the Swift Argument Parser!

    Before you submit your request, please replace each 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
-->

### Description
Constructors for Bool Flags with inverted pairs do not have a `FlagExclusivity` parameter (https://github.com/apple/swift-argument-parser/issues/60). This PR adds a `FlagExclusivity` parameter to two of the Flag constructors.

### Detailed Design
On `Flag`,
```
public init(name: NameSpecification = .long, default initial: Bool? = false,
            inversion: FlagInversion, help: ArgumentHelp? = nil)
```
becomes
```
public init(name: NameSpecification = .long, default initial: Bool? = false,
            inversion: FlagInversion, exclusivity: FlagExclusivity = .chooseLast,
            help: ArgumentHelp? = nil)
```

In the process, the logic to handle flag exclusivity collision was refactored to a new static function on `ArgumentSet`, and is used for `Bool` flags with inversions as well as flags built from `CaseIterable` enums.

### Documentation Plan
The new parameter has been documented in the doc comments.

### Test Plan
New tests are added to `FlagsEndToEndTests` and to `ErrorMessageTests`

### Source Impact
The parameter is additive with a default value, and therefore is source compatible.

### 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 resolves https://github.com/apple/swift-argument-parser/issues/60 and https://github.com/apple/swift-argument-parser/issues/61

Patch:
diff --git a/Sources/ArgumentParser/Parsable Properties/Flag.swift b/Sources/ArgumentParser/Parsable Properties/Flag.swift
index 67f97f131..1fa0c1ab6 100644
--- a/Sources/ArgumentParser/Parsable Properties/Flag.swift
+++ b/Sources/ArgumentParser/Parsable Properties/Flag.swift
@@ -129,14 +129,17 @@ extension Flag where Value == Optional<Bool> {
   ///   - name: A specification for what names are allowed for this flag.
   ///   - inversion: The method for converting this flags name into an on/off
   ///     pair.
+  ///   - exclusivity: The behavior to use when an on/off pair of flags is
+  ///     specified.
   ///   - help: Information about how to use this flag.
   public init(
     name: NameSpecification = .long,
     inversion: FlagInversion,
+    exclusivity: FlagExclusivity = .chooseLast,
     help: ArgumentHelp? = nil
   ) {
     self.init(_parsedValue: .init { key in
-      .flag(key: key, name: name, default: nil, inversion: inversion, help: help)
+      .flag(key: key, name: name, default: nil, inversion: inversion, exclusivity: exclusivity, help: help)
     })
   }
 }
@@ -188,15 +191,18 @@ extension Flag where Value == Bool {
   ///   - initial: The default value for this flag.
   ///   - inversion: The method for converting this flag's name into an on/off
   ///     pair.
+  ///   - exclusivity: The behavior to use when an on/off pair of flags is
+  ///     specified.
   ///   - help: Information about how to use this flag.
   public init(
     name: NameSpecification = .long,
     default initial: Bool? = false,
     inversion: FlagInversion,
+    exclusivity: FlagExclusivity = .chooseLast,
     help: ArgumentHelp? = nil
   ) {
     self.init(_parsedValue: .init { key in
-      .flag(key: key, name: name, default: initial, inversion: inversion, help: help)
+      .flag(key: key, name: name, default: initial, inversion: inversion, exclusivity: exclusivity, help: help)
       })
   }
 }
@@ -247,17 +253,7 @@ extension Flag where Value: CaseIterable, Value: RawRepresentable, Value.RawValu
         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.
-          let previous = values.element(forKey: key)
-          switch (hasUpdated, previous, exclusivity) {
-          case (true, let p?, .exclusive):
-            // This value has already been set.
-            throw ParserError.duplicateExclusiveValues(previous: p.inputOrigin, duplicate: origin, originalInput: values.originalInput)
-          case (false, _, _), (_, _, .chooseLast):
-            values.set(value, forKey: key, inputOrigin: origin)
-          default:
-            break
-          }
-          hasUpdated = true
+          hasUpdated = try ArgumentSet.updateFlag(key: key, value: value, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity)
         }))
       }
       return exclusivity == .exclusive
@@ -292,13 +288,9 @@ 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
-          if hasUpdated && exclusivity == .exclusive {
-            throw ParserError.unexpectedExtraValues([(origin, String(describing: value))])
-          }
-          if !hasUpdated || exclusivity == .chooseLast {
-            values.set(value, forKey: key, inputOrigin: origin)
-          }
-          hasUpdated = true
+          // 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)
         }))
       }
       return exclusivity == .exclusive
diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift
index a3b41cf73..976f8223d 100644
--- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift
+++ b/Sources/ArgumentParser/Parsing/ArgumentSet.swift
@@ -169,24 +169,39 @@ 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):
+      // This value has already been set.
+      throw ParserError.duplicateExclusiveValues(previous: previous.inputOrigin, duplicate: origin, originalInput: values.originalInput)
+    case (false, _, _), (_, _, .chooseLast):
+      values.set(value, forKey: key, inputOrigin: origin)
+      return true
+    default:
+      return hasUpdated
+    }
+  }
   
   /// Creates an argument set for a pair of inverted Boolean flags.
-  static func flag(key: InputKey, name: NameSpecification, default initialValue: Bool?, inversion: FlagInversion, help: ArgumentHelp?) -> ArgumentSet {
+  static func flag(key: InputKey, name: NameSpecification, default initialValue: Bool?, inversion: FlagInversion, exclusivity: FlagExclusivity, help: ArgumentHelp?) -> ArgumentSet {
     // The flag is required if initialValue is `nil`, otherwise it's optional
     let helpOptions: ArgumentDefinition.Help.Options = initialValue != nil ? .isOptional : []
     
     let help = ArgumentDefinition.Help(options: helpOptions, help: help, defaultValue: initialValue.map(String.init), key: key)
     let (enableNames, disableNames) = inversion.enableDisableNamePair(for: key, name: name)
-    
-    let enableArg = ArgumentDefinition(kind: .named(enableNames), help: help, update: .nullary({ (origin, name, values) in
-      values.set(true, forKey: key, inputOrigin: origin)
+
+    var hasUpdated = false
+    let enableArg = ArgumentDefinition(kind: .named(enableNames),help: help, update: .nullary({ (origin, name, values) in
+        hasUpdated = try ArgumentSet.updateFlag(key: key, value: true, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity)
     }), initial: { origin, values in
       if let initialValue = initialValue {
         values.set(initialValue, forKey: key, inputOrigin: origin)
       }
     })
     let disableArg = ArgumentDefinition(kind: .named(disableNames), help: ArgumentDefinition.Help(options: [.isOptional], key: key), update: .nullary({ (origin, name, values) in
-      values.set(false, forKey: key, inputOrigin: origin)
+        hasUpdated = try ArgumentSet.updateFlag(key: key, value: false, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity)
     }), initial: { _, _ in })
     return ArgumentSet(exclusive: [enableArg, disableArg])
   }


Test patch:
diff --git a/Tests/EndToEndTests/FlagsEndToEndTests.swift b/Tests/EndToEndTests/FlagsEndToEndTests.swift
index 9ab9661c3..18d5c48ad 100644
--- a/Tests/EndToEndTests/FlagsEndToEndTests.swift
+++ b/Tests/EndToEndTests/FlagsEndToEndTests.swift
@@ -27,6 +27,9 @@ fileprivate struct Bar: ParsableArguments {
 
   @Flag(inversion: .prefixedNo)
   var extattr2: Bool?
+
+  @Flag(inversion: .prefixedEnableDisable, exclusivity: .chooseFirst)
+  var logging: Bool
 }
 
 extension FlagsEndToEndTests {
@@ -74,6 +77,13 @@ extension FlagsEndToEndTests {
     AssertParse(Bar.self, ["--extattr", "--no-extattr", "--extattr"]) { options in
       XCTAssertEqual(options.extattr, true)
     }
+    AssertParse(Bar.self, ["--enable-logging"]) { options in
+      XCTAssertEqual(options.logging, true)
+    }
+// Can't test this yet, because .chooseFirst flags don't work
+//    AssertParse(Bar.self, ["--enable-logging", "--disable-logging"]) { options in
+//      XCTAssertEqual(options.logging, false)
+//    }
   }
 }
 
diff --git a/Tests/UnitTests/ErrorMessageTests.swift b/Tests/UnitTests/ErrorMessageTests.swift
index 4713abfb2..ec9ca7157 100644
--- a/Tests/UnitTests/ErrorMessageTests.swift
+++ b/Tests/UnitTests/ErrorMessageTests.swift
@@ -137,10 +137,14 @@ extension ErrorMessageTests {
 
 private enum OutputBehaviour: String, CaseIterable { case stats, count, list }
 private struct Options: ParsableArguments {
-  @Flag(name: .shortAndLong, help: "Program output")
+  @Flag(name: .shortAndLong, default: .list, help: "Program output")
   var behaviour: OutputBehaviour
 
-  @Flag() var bool: Bool
+  @Flag(inversion: .prefixedNo, exclusivity: .exclusive) var bool: Bool
+}
+private struct OptOptions: ParsableArguments {
+  @Flag(name: .short, help: "Program output")
+  var behaviour: OutputBehaviour?
 }
 
 extension ErrorMessageTests {
@@ -148,5 +152,9 @@ extension ErrorMessageTests {
     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\'")
+
+    AssertErrorMessage(Options.self, ["--no-bool", "--bool"], "Value to be set with flag \'--bool\' had already been set with flag \'--no-bool\'")
+
+    AssertErrorMessage(OptOptions.self, ["-cbl"], "Value to be set with flag \'l\' in \'-cbl\' had already been set with flag \'c\' in \'-cbl\'")
   }
 }