New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes incorrect value copying when ParseArguments has the same field name+type as the ParseCommand (Issue #322) #495
Fixes incorrect value copying when ParseArguments has the same field name+type as the ParseCommand (Issue #322) #495
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for diagnosing and fixing this issue, @randomeizer! I have some small nits for you to fix, but otherwise this looks ready to go 👍🏻
Tests/ArgumentParserEndToEndTests/OptionGroupEndToEndTests.swift
Outdated
Show resolved
Hide resolved
Tests/ArgumentParserEndToEndTests/OptionGroupEndToEndTests.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Nate Cook <natecook@apple.com>
Co-authored-by: Nate Cook <natecook@apple.com>
…licate-fields-in-OptionGroup
Nits picked, ready to go 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@swift-ci Please test |
@swift-ci Please test macos platform |
Description
This fix addresses the core issue described in #322.
As noted in the issue, if you have a
ParsedCommand
/AsyncParsedCommand
which contains aParsedArguments
implementation, and theParsedCommand
andParsedArguments
both have a field which has the same name and type, they will both update each other, even if one has a unique.customLong(...)
name.For example, a simple case which will break:
This is then output with flags set:
The first and last combination are correct, the two in the middle are incorrect - only one or the other should be
true
in those cases.Detailed Design
It turns out that the core issue is in
ArgumentDecoder
, with some assistance fromInputKey
. The rootArgumentDecoder
is handed the flat list ofParsedValues
, which contains a reference to thekey
as anInputKey
. ThatInputKey
just has the field name, with no link to any.customLong
values. That same list ofParsedValues
is handed to any sub-decoders for@OptionGroup
-annotatedParsedArguments
implementations. When the sub-decoder looks for its named field, itsInputKey
matches the parent struct's field, and they both end up sharing the same value.For example, if I have an instance of
MyCommand
calledcmd
, the two fields have thisInputKey
:cmd.myFlag
:InputKey(rawValue: "myFlag")
cmd.options.myFlag
:InputKey(rawValue: "myFlag")
The fix I've implemented is to augment
InputKey
to record any "parent" key relevant to the field during the parsing phase. For example, ourMyCommand
instance will now have the following:cmd.myFlag
:InputKey(name: "myFlag", parent: .root)
cmd.options.myFlag
:InputKey(name: "myFlag", parent: .key(InputKey(name: "options", parent: .root)))
Interestingly, the fix did not require any changes to
ArgumentDecoder
, but it did require changes to anything that created anInputKey
, along with passing in aparent
InputKey
where relevant, mostly when dealing withParsedArguments
.Documentation Plan
I have added source-level documentation to the
InputKey
in particular, along with a couple of other locations for clarity. All the changes were to internal code, so no public API/documentation changes seem to be necessary. Happy to update them if I missed something though.Test Plan
I've added four new tests cases into
OptionGroupEndToEndTests
, for lack of a better place to put it. I'm happy to move them elsewhere if there is a more appropriate location. Specifically, they are:testUniqueNamesForDuplicatedFlag_NoFlags
testUniqueNamesForDuplicatedFlag_RootOnly
testUniqueNamesForDuplicatedFlag_OptionOnly
testUniqueNamesForDuplicatedFlag_RootAndOption
They cover the four example combinations from above. I'm currently not testing other types (
@Option
/@Argument
), can add those also if desired. Again, let me know where is best to put them.All existing tests are currently passing.
Source Impact
The key change was with
InputKey
, as described above, and the related requirements of passing around the parentInputKey
at various points, particularly invalidate
methods. It doesn't change any public APIs.I also too the opportunity to move copy-pasted code that was dropping the leading "_" for
@propertyWrapper
fields intoInputKey
to keep everything tidy.InputKey
ended up being larger than theParsedValues
code, so I moved the whole struct into its own file:InputKey.swift
in the same folder.As noted above, it does not change the public API, so I don't believe any updates to the public API documentation are required.
Checklist