Fix invalid state error when decoding an unparsed optional value (#290)

Fixes “Internal error. Invalid state while parsing command-line arguments.” that is encountered when an unparsed value is optional.

Root causes:
- `ParsedArgumentsContainer.decodeNil` returns false for optional values because it only does a `!contains(key)` check. This should instead return nil if the value of the element is nil.
- The decoder did not know about unparsed input origins that and would result in unexpected behavior when decoding nil default values.
- The `value` of `Mirror.Child` is defined as `Any` but this is confusing because the value could be `Optional<Any>` which is not equal to `nil` even when the `Optional` case is `.none`.

Co-authored-by: Mike <[email protected]>

Author: werm098

Sources/ArgumentParser/Parsable Types/ParsableArguments.swift

@@ -241,7 +241,7 @@ extension ArgumentSet {
             key: InputKey(rawValue: codingKey),
             kind: .default,
             parser: { _ in nil },
-            default: child.value,
+            default: Mirror.realValue(for: child),
             completion: .default)
           definition.help.help = .hidden
           return ArgumentSet(definition)

Sources/ArgumentParser/Parsing/ArgumentDecoder.swift

@@ -104,7 +104,7 @@ final class ParsedArgumentsContainer<K>: KeyedDecodingContainerProtocol where K
   }
 
   func decodeNil(forKey key: K) throws -> Bool {
-    return !contains(key)
+    return element(forKey: key)?.value == nil
   }
 
   func decode<T>(_ type: T.Type, forKey key: K) throws -> T where T : Decodable {
@@ -113,7 +113,11 @@ final class ParsedArgumentsContainer<K>: KeyedDecodingContainerProtocol where K
   }
 
   func decodeIfPresent<T>(_ type: T.Type, forKey key: KeyedDecodingContainer<K>.Key) throws -> T? where T : Decodable {
-    let subDecoder = SingleValueDecoder(userInfo: decoder.userInfo, underlying: decoder, codingPath: codingPath + [key], key: InputKey(key), parsedElement: element(forKey: key))
+    let parsedElement = element(forKey: key)
+    if let parsedElement = parsedElement, parsedElement.inputOrigin.isDefaultValue {
+      return parsedElement.value as? T
+    }
+    let subDecoder = SingleValueDecoder(userInfo: decoder.userInfo, underlying: decoder, codingPath: codingPath + [key], key: InputKey(key), parsedElement: parsedElement)
     do {
       return try type.init(from: subDecoder)
     } catch let error as ParserError {

Sources/ArgumentParser/Parsing/ArgumentSet.swift

@@ -148,22 +148,20 @@ extension ArgumentSet {
 
 extension ArgumentDefinition {
   /// Create a unary / argument that parses using the given closure.
-  init<A>(key: InputKey, kind: ArgumentDefinition.Kind, parsingStrategy: ParsingStrategy = .nextAsValue, parser: @escaping (String) -> A?, parseType type: A.Type = A.self, default initial: A?, completion: CompletionKind) {
-    let initialValueCreator: (InputOrigin, inout ParsedValues) throws -> Void
-    if let initialValue = initial {
-      initialValueCreator = { origin, values in
-        values.set(initialValue, forKey: key, inputOrigin: origin)
-      }
-    } else {
-      initialValueCreator = { _, _ in }
-    }
-    
+  init<A>(key: InputKey, kind: ArgumentDefinition.Kind, parsingStrategy: ParsingStrategy = .nextAsValue, parser: @escaping (String) -> A?, parseType type: A.Type = A.self, default initial: A?, completion: CompletionKind) {    
     self.init(kind: kind, help: ArgumentDefinition.Help(key: key), completion: completion, parsingStrategy: parsingStrategy, update: .unary({ (origin, name, value, values) in
       guard let v = parser(value) else {
         throw ParserError.unableToParseValue(origin, name, value, forKey: key)
       }
       values.set(v, forKey: key, inputOrigin: origin)
-    }), initial: initialValueCreator)
+    }), initial: { origin, values in
+      switch kind {
+      case .default:
+        values.set(initial, forKey: key, inputOrigin: InputOrigin(element: .defaultValue))
+      case .named, .positional:
+        values.set(initial, forKey: key, inputOrigin: origin)
+      }
+    })
 
     help.options.formUnion(ArgumentDefinition.Help.Options(type: type))
     help.defaultValue = initial.map { "\($0)" }

Sources/ArgumentParser/Parsing/InputOrigin.swift

@@ -103,6 +103,12 @@ struct InputOrigin: Equatable, ExpressibleByArrayLiteral {
   }
 }
 
+extension InputOrigin {
+  var isDefaultValue: Bool {
+    return _elements.count == 1 && _elements.first == .defaultValue
+  }
+}
+
 extension InputOrigin.Element {
   static func < (lhs: Self, rhs: Self) -> Bool {
     switch (lhs, rhs) {

Sources/ArgumentParser/Parsing/ParsedValues.swift

@@ -29,7 +29,7 @@ struct InputKey: RawRepresentable, Hashable {
 struct ParsedValues {
   struct Element {
     var key: InputKey
-    var value: Any
+    var value: Any?
     /// Where in the input that this came from.
     var inputOrigin: InputOrigin
     fileprivate var shouldClearArrayIfParsed = true
@@ -45,7 +45,7 @@ struct ParsedValues {
 }
 
 extension ParsedValues {
-  mutating func set(_ new: Any, forKey key: InputKey, inputOrigin: InputOrigin) {
+  mutating func set(_ new: Any?, forKey key: InputKey, inputOrigin: InputOrigin) {
     set(Element(key: key, value: new, inputOrigin: inputOrigin))
   }
 

Sources/ArgumentParser/Parsing/ParserError.swift

@@ -38,7 +38,7 @@ enum ParserError: Error {
 
 /// These are errors used internally to the parsing, and will not be exposed to the help generation.
 enum InternalParseError: Error {
-  case wrongType(Any, forKey: InputKey)
+  case wrongType(Any?, forKey: InputKey)
   case subcommandNameMismatch
   case subcommandLevelMismatch(Int, Int)
   case subcommandLevelMissing(Int)

Sources/ArgumentParser/Utilities/MirrorExtensions.swift

@@ -0,0 +1,26 @@
+//===----------------------------------------------------------*- swift -*-===//
+//
+// This source file is part of the Swift Argument Parser open source project
+//
+// Copyright (c) 2021 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
+//
+//===----------------------------------------------------------------------===//
+
+extension Mirror {
+  /// Returns the "real" value of a `Mirror.Child` as `Any?`.
+  ///
+  /// The `value` of `Mirror.Child` is defined as `Any` but this is confusing because the value
+  /// could be `Optional<Any>` which is not equal to `nil` even when the `Optional` case is `.none`.
+  ///
+  /// The purpose of this function is to disambiguate when the `value` of a `Mirror.Child` is actaully nil.
+  static func realValue(for child: Mirror.Child) -> Any? {
+    if case Optional<Any>.none = child.value {
+      return nil
+    } else {
+      return child.value
+    }
+  }
+}

Tests/ArgumentParserEndToEndTests/UnparsedValuesEndToEndTest.swift

@@ -57,6 +57,73 @@ extension UnparsedValuesEndToEndTests {
   }
 }
 
+// MARK: Two value + unparsed optional variable
+
+fileprivate struct Hogeraa: ParsableArguments {
+  var fullName: String? = "Full Name"
+}
+
+fileprivate struct Hogera: ParsableArguments {
+  @Option() var firstName: String
+  @Flag() var hasLastName = false
+  var fullName: String?
+  mutating func validate() throws {
+    if hasLastName { fullName = "\(firstName) LastName" }
+  }
+}
+
+fileprivate struct Piyo: ParsableArguments {
+  @Option() var firstName: String
+  @Flag() var hasLastName = false
+  var fullName: String!
+  mutating func validate() throws {
+    fullName = firstName + (hasLastName ? " LastName" : "")
+  }
+}
+
+extension UnparsedValuesEndToEndTests {
+  func testParsing_TwoPlusOptionalUnparsed() throws {
+    AssertParse(Hogeraa.self, []) { hogeraa in
+      XCTAssertEqual(hogeraa.fullName, "Full Name")
+    }
+    
+    AssertParse(Hogera.self, ["--first-name", "Hogera"]) { hogera in
+      XCTAssertEqual(hogera.firstName, "Hogera")
+      XCTAssertFalse(hogera.hasLastName)
+      XCTAssertNil(hogera.fullName)
+    }
+    AssertParse(Hogera.self, ["--first-name", "Hogera", "--has-last-name"]) { hogera in
+      XCTAssertEqual(hogera.firstName, "Hogera")
+      XCTAssertTrue(hogera.hasLastName)
+      XCTAssertEqual(hogera.fullName, "Hogera LastName")
+    }
+    
+    AssertParse(Piyo.self, ["--first-name", "Hogera"]) { piyo in
+      XCTAssertEqual(piyo.firstName, "Hogera")
+      XCTAssertFalse(piyo.hasLastName)
+      XCTAssertEqual(piyo.fullName, "Hogera")
+    }
+    AssertParse(Piyo.self, ["--first-name", "Hogera", "--has-last-name"]) { piyo in
+      XCTAssertEqual(piyo.firstName, "Hogera")
+      XCTAssertTrue(piyo.hasLastName)
+      XCTAssertEqual(piyo.fullName, "Hogera LastName")
+    }
+  }
+  
+  func testParsing_TwoPlusOptionalUnparsed_Fails() throws {
+    XCTAssertThrowsError(try Hogeraa.parse(["--full-name"]))
+    XCTAssertThrowsError(try Hogeraa.parse(["--full-name", "Hogera Piyo"]))
+    XCTAssertThrowsError(try Hogera.parse([]))
+    XCTAssertThrowsError(try Hogera.parse(["--first-name"]))
+    XCTAssertThrowsError(try Hogera.parse(["--first-name", "Hogera", "--full-name"]))
+    XCTAssertThrowsError(try Hogera.parse(["--first-name", "Hogera", "--full-name", "Hogera Piyo"]))
+    XCTAssertThrowsError(try Piyo.parse([]))
+    XCTAssertThrowsError(try Piyo.parse(["--first-name"]))
+    XCTAssertThrowsError(try Piyo.parse(["--first-name", "Hogera", "--full-name"]))
+    XCTAssertThrowsError(try Piyo.parse(["--first-name", "Hogera", "--full-name", "Hogera Piyo"]))
+  }
+}
+
 // MARK: Nested unparsed decodable type
 
 
@@ -106,3 +173,86 @@ extension UnparsedValuesEndToEndTests {
     XCTAssertThrowsError(try Foo.parse(["--one", "aaa"]))
   }
 }
+
+// MARK: Nested unparsed optional decodable type
+
+fileprivate struct Barr: ParsableCommand {
+  var baz: Baz? = Baz(name: "Some Name", age: 105)
+}
+
+fileprivate struct Bar: ParsableCommand {
+  @Flag var bar: Bool = false
+  var baz: Baz?
+  var bazz: Bazz?
+  mutating func validate() throws {
+    if bar {
+      baz = Baz(name: "Some", age: 100)
+      bazz = Bazz(name: "Other", age: 101)
+    }
+  }
+}
+
+fileprivate struct Baz: Decodable {
+  var name: String?
+  var age: Int!
+}
+
+fileprivate struct Bazz: Decodable {
+  var name: String?
+  var age: Int
+}
+
+extension UnparsedValuesEndToEndTests {
+  func testUnparsedNestedOptionalValue() {
+    AssertParse(Barr.self, []) { barr in
+      XCTAssertNotNil(barr.baz)
+      XCTAssertEqual(barr.baz?.age, 105)
+      XCTAssertEqual(barr.baz?.name, "Some Name")
+    }
+    
+    AssertParse(Bar.self, []) { bar in
+      XCTAssertFalse(bar.bar)
+      XCTAssertNil(bar.baz)
+      XCTAssertNil(bar.baz?.age)
+      XCTAssertNil(bar.baz?.name)
+      XCTAssertNil(bar.bazz)
+      XCTAssertNil(bar.bazz?.age)
+      XCTAssertNil(bar.bazz?.name)
+    }
+
+    AssertParse(Bar.self, ["--bar"]) { bar in
+      XCTAssertTrue(bar.bar)
+      XCTAssertNotNil(bar.baz)
+      XCTAssertEqual(bar.baz?.name, "Some")
+      XCTAssertEqual(bar.baz?.age, 100)
+      XCTAssertNotNil(bar.bazz)
+      XCTAssertEqual(bar.bazz?.name, "Other")
+      XCTAssertEqual(bar.bazz?.age, 101)
+    }
+  }
+  
+  func testUnparsedNestedOptionalValue_Fails() {
+    XCTAssertThrowsError(try Bar.parse(["--baz", "xyz"]))
+    XCTAssertThrowsError(try Bar.parse(["--bazz", "xyz"]))
+    XCTAssertThrowsError(try Bar.parse(["--name", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--age", "123"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--name", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--age", "123"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "xyz"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "--name", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "xyz", "--name"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "xyz", "--name", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "--age", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "xyz", "--age"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--baz", "xyz", "--age", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "xyz"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "--name", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "xyz", "--name"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "xyz", "--name", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "--age", "None"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "xyz", "--age"]))
+    XCTAssertThrowsError(try Bar.parse(["--bar", "--bazz", "xyz", "--age", "None"]))
+  }
+}

Tests/ArgumentParserUnitTests/InputOriginTests.swift

@@ -0,0 +1,33 @@
+//===----------------------------------------------------------*- swift -*-===//
+//
+// This source file is part of the Swift Argument Parser open source project
+//
+// Copyright (c) 2021 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
+@testable import ArgumentParser
+
+final class InputOriginTests: XCTestCase {}
+
+extension InputOriginTests {
+  func testIsDefaultValue() {
+    func Assert(elements: [InputOrigin.Element], expectedIsDefaultValue: Bool) {
+      let inputOrigin = InputOrigin(elements: elements)
+      if expectedIsDefaultValue {
+        XCTAssertTrue(inputOrigin.isDefaultValue)
+      } else {
+        XCTAssertFalse(inputOrigin.isDefaultValue)
+      }
+    }
+    
+    Assert(elements: [], expectedIsDefaultValue: false)
+    Assert(elements: [.defaultValue], expectedIsDefaultValue: true)
+    Assert(elements: [.argumentIndex(SplitArguments.Index(inputIndex: 1))], expectedIsDefaultValue: false)
+    Assert(elements: [.defaultValue, .argumentIndex(SplitArguments.Index(inputIndex: 1))], expectedIsDefaultValue: false)
+  }
+}

Tests/ArgumentParserUnitTests/MirrorTests.swift

@@ -0,0 +1,59 @@
+//===----------------------------------------------------------*- swift -*-===//
+//
+// This source file is part of the Swift Argument Parser open source project
+//
+// Copyright (c) 2021 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
+@testable import ArgumentParser
+
+final class MirrorTests: XCTestCase {}
+
+extension MirrorTests {
+  private struct Foo {
+    let foo: String?
+    let bar: String
+    let baz: String!
+  }
+  func testRealValue() {
+    func checkChildValue(_ child: Mirror.Child, expectedString: String?) {
+      if let expectedString = expectedString {
+        guard let stringValue = child.value as? String else {
+          XCTFail("child.value is not a String type")
+          return
+        }
+        XCTAssertEqual(stringValue, expectedString)
+      } else {
+        XCTAssertNil(Mirror.realValue(for: child))
+        // This is why we use `unwrapedOptionalValue` for optionality checks
+        // Even though the `value` is `nil` this returns `false`
+        XCTAssertFalse(child.value as Any? == nil)
+      }
+    }
+    func performTest(foo: String?, baz: String!) {
+      let fooChild = Foo(foo: foo, bar: "foobar", baz: baz)
+      Mirror(reflecting: fooChild).children.forEach { child in
+        switch child.label {
+        case "foo":
+          checkChildValue(child, expectedString: foo)
+        case "bar":
+          checkChildValue(child, expectedString: "foobar")
+        case "baz":
+          checkChildValue(child, expectedString: baz)
+        default:
+          XCTFail("Unexpected child")
+        }
+      }
+    }
+    
+    performTest(foo: "foo", baz: "baz")
+    performTest(foo: "foo", baz: nil)
+    performTest(foo: nil, baz: "baz")
+    performTest(foo: nil, baz: nil)
+  }
+}