Skip to content

Conversation

@dusrdev
Copy link
Contributor

@dusrdev dusrdev commented Oct 3, 2025

Bug

Before these changes, the generated code would incorrectly bind anything to required postional arguments (parameters decorated with Argument attribute) - this includes options.

For example:

args = ["--dry-run"];

ConsoleApp.Run(args, ([Argument] string path, bool dryRun) => {
	Console.WriteLine($"path = {path}");
	Console.WriteLine($"dryRun = {dryRun}");
});

Would print:

path = --dry-run
dryRun = False

Which is invalid, and should fail because "path" is missing.

Fix

  • Update Emitter.EmitRun so named options are handled before positional [Argument] parameters:
    • Emit an argumentPosition counter that advances only when a non-option token is consumed, keeping positional slots aligned with the number of positional values actually provided.
    • Treat a token as an option only when it looks like -x/--name (exclude negative numbers) and run the existing switch to parse it; when a match occurs, set optionMatched and continue so the token never reaches positional binding.
    • Fall through to the positional logic only for true non-option tokens, preserving the existing “unknown argument” exception path.
  • Extend RunTest.cs with four regression tests that cover the original single-argument case, multiple positional arguments, an [Argument] with a default value, and a command that mixes [Argument] with a params array, demonstrating the new parser behavior across common patterns.

Tests

All 104 generator tests (including the 4 new ones) pass.

@neuecc
Copy link
Member

neuecc commented Oct 10, 2025

Since bool dryRun is optional, it gets interpreted as a string and assigned to path.
I don't think it's necessarily a bug that dryRun is processed as optional.
With your proposal, the following code would result in an error, but how should it be handled?

args = ["--foo", "--foo", "3", "--bar", "5"];
ConsoleApp.Run(args, ([Argument] string path, int foo, int bar) =>
{
    var obj = new { path, foo, bar };
    Console.WriteLine($"{obj}");
});

I think it's not bad to process things with simple rules (arguments bind with highest priority) rather than running complex interpretations.

@dusrdev
Copy link
Contributor Author

dusrdev commented Oct 10, 2025

@neuecc

In your example, returning an error is the logical move because --foo is first passed as a flag, but the is declared as a parameter that requires value (int).

It does show:

Argument 'foo' failed to parse, provided value: --foo

This is correct behavior as the second --foo is supposed to be parsed into an integer.

And this PR addresses the fact that Argument parameters (i.e required positional) are binding to whatever comes in their position. Which is definitely not the correct behavior according to most commonly used UNIX CLIs. Options should bind only to themeselves.

i.e. if I pass --dry-run it should only ever bind to the correct option value, and never to another option or argument. Which is why options (named required/optional parameters and flags) need to be parsed before arguments (positional parameters), such that the "leftovers" would bind to the positional paramters.

For the sake of example, imagine the following example with "rsync", to which the real parameters are similar to this:

args = ["--dry-run", "destination"];
ConsoleApp.Run(args, ([Argument] string source, [Argument] string destination, bool dryRun) =>
{
	Console.WriteLine(new { source, destination, dryRun });
});

If you are not familiar, "rsync" syncronizes files between 2 locations. In which case:

  • Current behavior: dry-run flag is never parsed, source destination = --dry-run (which it wouldn't be able to find and/or maybe other location that exist, depending on the option name), and destination is correct. The cli will try to syncronize the locations, leading to probably deleting all the files in "destination".
  • After this PR: dryRun parsed correctly, so does destination, but we receive an error that source wasn't provided. Avoiding a potential disaster.

Also,

rather than running complex interpretations.

This PR doesn't add complexity to the interpretation, mainly just re-orders the processing of options to be before arguments, it also shouldn't really affect perf at all, and even if it will, the amount will probably will be insignificant in real world and a necessary price to pay to avoid examples like I provided above.

@neuecc
Copy link
Member

neuecc commented Oct 20, 2025

Thanks for the detailed explanation.
Yeah, I agree it's better to do the parsing first!
I think it's basically fine, but I have some questions about the code.

  • Is the char.IsDigit check necessary?
  • Is optionMatched necessary? It seems like just using continue would be fine.
  • I think this check is only needed when hasArgument is true, so please adjust the output with if (hasArgument).

@dusrdev
Copy link
Contributor Author

dusrdev commented Oct 20, 2025

Thanks for the follow-up!

  • The char.IsDigit check was necessary to prevent a scenario like:
ConsoleApp.Run(["-5", "--dry-run"], ([Argument] int count, bool dryRun) =>)

Without the guard, -5 is treated as an option name and we throw ThrowArgumentNameNotFound, but with the guard in place it binds correctly to count = -5 and the flag still parses.

I did change the guard to only emit when there are positional parameters to bind, and I added another test case ArgumentAllowsLeadingDashValue to check for this type of scenario.

  • I removed the optionMatched flag; each successful option parse now issues continue; directly so we skip positional binding without the extra local.
  • ThrowArgumentNameNotFound emission is also wrapped in if (hasArgument) so the error only appears when positional slots remain.

If you’d like anything else adjusted, just let me know.

@neuecc
Copy link
Member

neuecc commented Oct 21, 2025

Thank you, I think it's very well thought out!
There are other fixes as well, so I'll likely release it today along with those.

@neuecc neuecc merged commit 6e344c9 into Cysharp:master Oct 21, 2025
@neuecc
Copy link
Member

neuecc commented Oct 21, 2025

I've released v5.6.2.

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.

2 participants