Fixes incorrect value copying when ParseArguments has the same field name+type as the ParseCommand (Issue #322)

[randomeizer]

Description

This fix addresses the core issue described in #322.

As noted in the issue, if you have a ParsedCommand/AsyncParsedCommand which contains a ParsedArguments implementation, and the ParsedCommand and ParsedArguments 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:

struct MyOptions: ParsedArguments {
  @Flag(name: .customLong("my-custom-flag"))
  var myFlag: Bool = false
}

struct MyCommand: ParsedCommand {
  @Flag()
  var myFlag: Bool = false

  @OptionGroup var options: MyOptions

  mutating func run() throws {
    print("My Flag: \(myFlag)")
    print("My Custom Flag: \(options.myFlag)")
  }
}

This is then output with flags set:

> swift run MyCommand
My Flag: false
My Custom Flag: false
> swift run MyCommand --my-flag
My Flag: true
My Custom Flag: true
> swift run MyCommand --my-custom-flag
My Flag: true
My Custom Flag: true
> swift run MyCommand --my-flag --my-custom-flag
My Flag: true
My Custom Flag: true

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 from InputKey. The root ArgumentDecoder is handed the flat list of ParsedValues, which contains a reference to the key as an InputKey. That InputKey just has the field name, with no link to any .customLong values. That same list of ParsedValues is handed to any sub-decoders for @OptionGroup-annotated ParsedArguments implementations. When the sub-decoder looks for its named field, its InputKey matches the parent struct's field, and they both end up sharing the same value.

For example, if I have an instance of MyCommand called cmd, the two fields have this InputKey:

• 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, our MyCommand 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 an InputKey, along with passing in a parent InputKey where relevant, mostly when dealing with ParsedArguments.

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 parent InputKey at various points, particularly in validate 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 into InputKey to keep everything tidy.

InputKey ended up being larger than the ParsedValues 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

• I've added at least one test that validates that my change is working, if appropriate
• I've followed the code style of the rest of the project
• I've read the Contribution Guidelines
• I've updated the documentation if necessary

[natecook1000]

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 👍🏻

[randomeizer]

Nits picked, ready to go 🙂

[natecook1000]

LGTM!

[natecook1000]

@swift-ci Please test

[natecook1000]

@swift-ci Please test macos platform