Skip to content
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

Conversation

randomeizer
Copy link
Contributor

@randomeizer randomeizer commented Sep 19, 2022

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

@randomeizer randomeizer changed the title Contains fixes and test cases for #322 Fixes incorrect value copying when ParseArguments has the same field name+type as the ParseCommand (Issue #322) Sep 20, 2022
Copy link
Member

@natecook1000 natecook1000 left a 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 👍🏻

@randomeizer
Copy link
Contributor Author

Nits picked, ready to go 🙂

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000
Copy link
Member

@swift-ci Please test macos platform

@natecook1000 natecook1000 merged commit 587d26a into apple:main Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants