Fix invalid state error when decoding an unparsed optional value#290
Conversation
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`.
| /// 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? { |
There was a problem hiding this comment.
This name could probably be better?
There was a problem hiding this comment.
I think this would work better as an (Any) -> Any? function, since it doesn't really have anything to do with Mirror, just the fact that Mirror.Child.value is an Any. How about calling it nilOrValue?
| /// 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. |
There was a problem hiding this comment.
Maybe there is a Foundation change we could make too? I have run into this problem previously as well, not sure if this is a known behavior of Mirror.Child or optionals in Swift.
| var fullName: String? = "Full Name" | ||
| } | ||
|
|
||
| fileprivate struct Hogera: ParsableArguments { |
There was a problem hiding this comment.
If we move structs into the extension code block then we could reuse names for different test collections. This could cut down on the amount of metasyntactic type names (and modifications of those names) used.
There was a problem hiding this comment.
For sure! I don't think tests are very consistent around this, so if you'd like to structure yours that way, that'd be great.
|
Thanks for this, @werm098! Do you have a reduced example that shows the incorrect behavior? |
Sure thing, I can create a reduced example project (will post later). I also added to the unit tests for this incorrect behavior too: https://github.com/apple/swift-argument-parser/pull/290/files#diff-aae587b62a491288c0ad8d5dde7f8afd599171fdea4556602063ebe1ccc21259R66 |
|
@natecook1000 Minimum reproducing project: MVR.zip You can change in the dependencies between 0.4.1 and 0.3.2 for an error vs no error respectively. |
natecook1000
left a comment
natecook1000
left a comment
There was a problem hiding this comment.
Sorry for the delay here — this fix looks good! Just a couple nits below. Thanks again for taking this on.
| /// 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? { |
There was a problem hiding this comment.
I think this would work better as an (Any) -> Any? function, since it doesn't really have anything to do with Mirror, just the fact that Mirror.Child.value is an Any. How about calling it nilOrValue?
| var fullName: String? = "Full Name" | ||
| } | ||
|
|
||
| fileprivate struct Hogera: ParsableArguments { |
There was a problem hiding this comment.
For sure! I don't think tests are very consistent around this, so if you'd like to structure yours that way, that'd be great.
|
@swift-ci Please test |
2 participants
Fixes “Internal error. Invalid state while parsing command-line arguments.” that is encountered when an unparsed value is optional.
Root causes:
ParsedArgumentsContainer.decodeNilreturns 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.valueofMirror.Childis defined asAnybut this is confusing because the value could beOptional<Any>which is not equal tonileven when theOptionalcase is.none.Checklist