Skip to content

Fix parameter index not incrementing in nested subcommands #32

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.

Sign up for GitHub

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

Merged
agocke merged 5 commits into from
Jan 17, 2026
Merged

Fix parameter index not incrementing in nested subcommands #32

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ab93bc
Initial plan
Copilot Jan 17, 2026
abfe17e
Add failing test for nested subcommand with two parameters bug
Copilot Jan 17, 2026
a59607a
Fix parameter index bug by incrementing _paramIndex after match
Copilot Jan 17, 2026
fd20886
Refactor CheckFields to be side-effect free with isParameter signal
Copilot Jan 17, 2026
215bbe9
Replace tuple booleans with FieldKind enum for clarity
Copilot Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Serde.CmdLine/Deserializer.DeserializeType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ int ITypeDeserializer.TryReadIndex(ISerdeInfo info)
// Parameters are positional, so we check the current param index
if (_deserializer._paramIndex == param.Ordinal)
{
_deserializer._paramIndex++;
Copy link
Contributor

@agocke agocke Jan 17, 2026

Choose a reason for hiding this comment

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

@copilot is the rest of this method non-side effecting? Do we want to move the increment out of the method and somehow add signaling that we matched a parameter?

Copy link
Contributor Author

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

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

Good catch! I've refactored CheckFields to be side-effect free. It now returns (int fieldIndex, bool incArgs, bool isParameter) where isParameter signals when a parameter is matched. The caller increments _paramIndex based on this signal (lines 65-68), following the same pattern as incArgs for argIndex.

Commit: fd20886

return (param.FieldIndex, false);
}
}
Expand Down
43 changes: 43 additions & 0 deletions test/Serde.CmdLine.Test/SubCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,47 @@ public sealed partial record FirstCommand : SubCommand
[Command("second")]
public sealed partial record SecondCommand : SubCommand;
}

/// <summary>
/// Test to verify that two command parameters in a nested subcommand are parsed correctly.
/// This test is designed to reproduce a bug where the second parameter is parsed as a
/// duplicate of the first parameter due to _paramIndex not being incremented.
/// </summary>
[Fact]
public void NestedSubCommandWithTwoParameters()
{
string[] testArgs = [ "copy", "source.txt", "dest.txt" ];
var cmd = CmdLine.ParseRawWithHelp<CommandWithParams>(testArgs).Unwrap();
Assert.Equal(new CommandWithParams
{
SubCommandWithParams = new SubCommandWithParams.CopyCommand
{
Source = "source.txt",
Destination = "dest.txt"
}
}, cmd);
}

[GenerateDeserialize]
private partial record CommandWithParams
{
[CommandGroup("command")]
public SubCommandWithParams? SubCommandWithParams { get; init; }
}

[GenerateDeserialize]
private abstract partial record SubCommandWithParams
{
private SubCommandWithParams() { }

[Command("copy")]
public sealed partial record CopyCommand : SubCommandWithParams
{
[CommandParameter(0, "source")]
public string? Source { get; init; }

[CommandParameter(1, "destination")]
public string? Destination { get; init; }
}
}
}