Skip to content

fix(#45761): Stop forwarding dotnet-watch run arguments to all commands #50558

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

Open
ronald-mz wants to merge 5 commits into
base: release/10.0.1xx
Choose a base branch
from
Open

fix(#45761): Stop forwarding dotnet-watch run arguments to all commands #50558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5026d2e
fix(#45761): Stop passing arguments meant for dotnet-watch to all com…
ronald-mz Aug 30, 2025
9db77ff
forgot to re-add the commands to root while tidying
ronald-mz Aug 30, 2025
d16bf3c
remove tab
ronald-mz Aug 30, 2025
db68b03
Fix a brainfart that lead to a test case using a command as a launch-…
ronald-mz Aug 30, 2025
d70c25a
[CombinatorialData]
ronald-mz Aug 30, 2025
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
74 changes: 46 additions & 28 deletions src/BuiltInTools/dotnet-watch/CommandLine/CommandLineOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ internal sealed class CommandLineOptions
}
});

// Options we need to know about that are passed through to the subcommand:
Copy link
Member

@tmat tmat Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

add ... if the subcommand supports them

var shortProjectOption = new Option<string>("-p") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false };
var longProjectOption = new Option<string>("--project") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false };
var launchProfileOption = new Option<string>("--launch-profile", "-lp") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false };
var noLaunchProfileOption = new Option<bool>("--no-launch-profile") { Hidden = true, Arity = ArgumentArity.Zero };

Option[] watchOptions =
[
quietOption,
Expand All @@ -69,12 +75,13 @@ internal sealed class CommandLineOptions
noHotReloadOption,
NonInteractiveOption
];

// Options we need to know about that are passed through to the subcommand:
var shortProjectOption = new Option<string>("-p") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false };
var longProjectOption = new Option<string>("--project") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false };
var launchProfileOption = new Option<string>("--launch-profile", "-lp") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false };
var noLaunchProfileOption = new Option<bool>("--no-launch-profile") { Hidden = true, Arity = ArgumentArity.Zero };
Option[] forwardToRunOptions =
Copy link
Member

@tmat tmat Sep 2, 2025

Choose a reason for hiding this comment

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

I'd call this forwardToSubcommandOptions

Copy link
Member

@tmat tmat Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: You can replace https://github.com/dotnet/sdk/pull/50558/files#diff-6a6fa2fea5af87fda145d79165f529cef772a3227d58494098b7a22511c4aecaR96-R99 with

foreach (var option in forwardToSubcommandOptions)
{
     rootCommand.Options.Add(option);
}

[
shortProjectOption,
longProjectOption,
launchProfileOption,
noLaunchProfileOption
];

var rootCommand = new RootCommand(Resources.Help)
{
Expand Down Expand Up @@ -158,7 +165,7 @@ internal sealed class CommandLineOptions
}
}

var commandArguments = GetCommandArguments(parseResult, watchOptions, explicitCommand, out var binLogToken, out var binLogPath);
var commandArguments = GetCommandArguments(parseResult, watchOptions, forwardToRunOptions, explicitCommand, out var binLogToken, out var binLogPath);

// We assume that forwarded options, if any, are intended for dotnet build.
var buildArguments = buildOptions.Select(option => ((IForwardedOption)option).GetForwardingFunction()(parseResult)).SelectMany(args => args).ToList();
Expand Down Expand Up @@ -216,6 +223,7 @@ internal sealed class CommandLineOptions
private static IReadOnlyList<string> GetCommandArguments(
ParseResult parseResult,
IReadOnlyList<Option> watchOptions,
IReadOnlyList<Option> forwardToRunOptions,
Command? explicitCommand,
out string? binLogToken,
out string? binLogPath)
Expand All @@ -224,37 +232,47 @@ private static IReadOnlyList<string> GetCommandArguments(
binLogToken = null;
binLogPath = null;

bool isRunCommand = explicitCommand == null || explicitCommand?.Name == "run";

foreach (var child in parseResult.CommandResult.Children)
{
var optionResult = (OptionResult)child;

// skip watch options:
if (!watchOptions.Contains(optionResult.Option))
Copy link
Author

@ronald-mz ronald-mz Aug 30, 2025

Choose a reason for hiding this comment

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

Inverting the logic allows a continue to short-circuit the loop.

if (watchOptions.Contains(optionResult.Option))
{
if (optionResult.Option.Name.Equals("--interactive", StringComparison.Ordinal) && parseResult.GetValue(NonInteractiveOption))
{
// skip forwarding the interactive token (which may be computed by default) when users pass --non-interactive to watch itself
continue;
}
continue;
}

// skip forwarding run options to other commands.
if (isRunCommand == false && forwardToRunOptions.Contains(optionResult.Option))
Copy link
Author

@ronald-mz ronald-mz Aug 30, 2025

Choose a reason for hiding this comment

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

The meat and potatoes of this PR. Prevents options which only work with the run command from being passed to other commands.

Copy link
Member

@tmat tmat Sep 2, 2025

Choose a reason for hiding this comment

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

nit: !isRunCommand

Copy link
Member

@tmat tmat Sep 2, 2025

Choose a reason for hiding this comment

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

Can we pass command from the caller and generalize this to if (forwardToSubcommandOptions.Contains(optionResult.Option) && !command.Options.Contains(optionResult.Option)) ?

Copy link
Author

@ronald-mz ronald-mz Sep 2, 2025

Choose a reason for hiding this comment

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

Of course. Give me a bit and I'll get some commits up.

Further Thoughts

This is outside the scope of this fix, but a different PR could go a step further towards generalization by:
nixing forwardToSubcommandOptions in favor of watchableSubCommandOptions.

watchableSubCommandOptions are options that can show up in subcommands that could affect watch's settings.

For example: run --project and build [<PROJECT>|<SOLUTION>] could both set watch's project path.

Copy link
Member

@Youssef1313 Youssef1313 Sep 12, 2025

Choose a reason for hiding this comment

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

This is very concerning to me. Are we ignoring --project completely? That's worse than producing an error.

Also, it means we might be regressing the scenario for the MTP-based dotnet test which actually accepts --project.

@tmat I don't have much context on dotnet watch, but I think we might want to discuss this offline to ensure that we at least are not breaking the MTP-based dotnet test.

Copy link
Author

@ronald-mz ronald-mz Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Youssef1313 No, we are not ignoring --project completely. This makes sure it is not passed to the subcommand. dotnet watch still needs it internally.

Also, please look at #45761 as it seems like there is a catch22 here. If we don't fix it, we leave dotnet watch [subcommand] --project broken for most normal dotnet CLI scenarios.

Copy link
Member

@Youssef1313 Youssef1313 Sep 12, 2025

Choose a reason for hiding this comment

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

What happens for Microsoft.Testing.Platform then? In that case I think it will need to be passed to test command, similar to run.

Copy link
Author

@ronald-mz ronald-mz Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that's the catch 22.

Looking over docs, it looks like dotnet test is the only command that has a difference in MCP. Is there a way to tell if we're running in the MCP environment? If so, we could add that as a condition here.

But, I dislike the additional brittleness that would add to the check. I'd prefer if we could query for the subcommand's acceptable arguments without running the subcommand. If there is a better way to query the acceptable args, please point them out for me. I'll change course to use that.

Copy link
Member

@Youssef1313 Youssef1313 Sep 12, 2025

Choose a reason for hiding this comment

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

MCP is unrelated. MTP is "Microsoft.Testing.Platform" (so only relevant to test command - and is determined by global.json). I think you can just get the test command and look into its options. When you retrieve the test command, it will return the right thing depending on what is there in global.json.

Copy link
Author

@ronald-mz ronald-mz Sep 12, 2025

Choose a reason for hiding this comment

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

Apologies, MCP is a typo. I meant MTP.

Alright, I think I'm in the right place.

Test's parser has GetCommand

sdk/src/Cli/dotnet/Commands/Test/TestCommandParser.cs

Line 176 in f099c19

public static Command GetCommand()

Even better to go one level up to dotnet's parser with GetBuiltInCommand

sdk/src/Cli/dotnet/Parser.cs

Line 189 in f099c19

public static Command GetBuiltInCommand(string commandName) =>

Then we'd be able to check any subcommand's arguments for pass-through viability.

{
continue;
}

// skip Option<bool> zero-arity options with an implicit optionresult - these weren't actually specified by the user:
if (optionResult.Option is Option<bool> boolOpt && boolOpt.Arity.Equals(ArgumentArity.Zero) && optionResult.Implicit)
{
continue;
}
// skip forwarding the interactive token (which may be computed by default) when users pass --non-interactive to watch itself
if (optionResult.Option.Name.Equals("--interactive", StringComparison.Ordinal) && parseResult.GetValue(NonInteractiveOption))
{
continue;
}

// skip Option<bool> zero-arity options with an implicit optionresult - these weren't actually specified by the user:
if (optionResult.Option is Option<bool> boolOpt && boolOpt.Arity.Equals(ArgumentArity.Zero) && optionResult.Implicit)
{
continue;
}

var optionNameToForward = GetOptionNameToForward(optionResult);
if (optionResult.Tokens.Count == 0 && !optionResult.Implicit)
var optionNameToForward = GetOptionNameToForward(optionResult);
if (optionResult.Tokens.Count == 0 && !optionResult.Implicit)
{
arguments.Add(optionNameToForward);
}
else
{
foreach (var token in optionResult.Tokens)
{
arguments.Add(optionNameToForward);
}
else
{
foreach (var token in optionResult.Tokens)
{
arguments.Add(optionNameToForward);
arguments.Add(token.Value);
}
arguments.Add(token.Value);
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions test/dotnet-watch.Tests/CommandLine/CommandLineOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,33 @@ public void ExplicitCommand(string command)
Assert.Empty(args);
}

[Theory]
[CombinatorialData]
public void RunOptions_LaunchProfile_NotPassedThrough(bool beforeCommand)
{
var options = VerifyOptions(beforeCommand ? ["--launch-profile", "p", "test"] : ["test", "--launch-profile", "p"]);
Assert.Equal("test", options.Command);
AssertEx.SequenceEqual([], options.CommandArguments);
}

[Theory]
[CombinatorialData]
public void RunOptions_NoLaunchProfile_NotPassedThrough(bool beforeCommand)
{
var options = VerifyOptions(beforeCommand ? ["--no-launch-profile", "test"] : ["test", "--no-launch-profile"]);
Assert.Equal("test", options.Command);
AssertEx.SequenceEqual([], options.CommandArguments);
}

[Theory]
[CombinatorialData]
public void RunOption_Project_NotPassedThrough(bool beforeCommand)
{
var options = VerifyOptions(beforeCommand ? ["--project", "MyProject.csproj", "test"] : ["test", "--project", "MyProject.csproj"]);
Assert.Equal("test", options.Command);
AssertEx.SequenceEqual([], options.CommandArguments);
}

[Theory]
[CombinatorialData]
public void WatchOptions_NotPassedThrough(
Expand Down
Loading