feat: Add logics to escape MSBuildArgument text representation when contains MSBuild special chars

[filzrev]

This PR intended to fix issue #2719.

There is existing PR(#2729) that trying to resolve issue by escaping MSBuild special chars

Instead, this PR try to resolve issue by surrounding MSBuild parameter value by quotes.
(See: dotnet/sdk#8792 (comment) for details)

What's changed in this PR

1. Argument.cs
Add internal method GetEscapedTextRepresentation to MsBuildArgument.
This method returns platform-dependent text representation that is used for script file.

2. DotNetCliCommand.cs
   Modify code to pass GetEscapedTextRepresentation value instead of TextRepresentation

3. MsBuildArgumentTests.cs
   Add unit tests for various argument patterns.

Additional integration tests

I've confirmed benchmarks works as expected both Windows/Ubuntu(on WSL) environments.

public class CustomConfig : ManualConfig
{
    public CustomConfig()
    {
        WithOptions(ConfigOptions.DisableOptimizationsValidator);
        WithOptions(ConfigOptions.StopOnFirstError);
        WithOptions(ConfigOptions.KeepBenchmarkFiles);
        WithOptions(ConfigOptions.GenerateMSBuildBinLog);

        WithUnionRule(DefaultConfig.Instance.UnionRule);

        var baseJob = Job.ShortRun
            .WithStrategy(RunStrategy.Monitoring)
            .DontEnforcePowerPlan()
            .Freeze();

        AddJob(baseJob.WithArguments([new MsBuildArgument("/p:DefineConstants2=V1;V111")]).WithId("V1"));
        AddJob(baseJob.WithArguments([new MsBuildArgument("/p:DefineConstants2=V2;V222")]).WithId("V2"));

        AddLogger(ConsoleLogger.Default);

        AddExporter(DefaultConfig.Instance.GetExporters().ToArray());
        AddAnalyser(DefaultConfig.Instance.GetAnalysers().ToArray());
        AddColumnProvider(DefaultColumnProviders.Instance);
    }

[timcassell]

I'm confused by this PR. The implementation does not match the MSBuild documentation for special characters. Wrapping values with quotes instead just works? Is it documented anywhere? Why does comma need to be escaped also? And why is it only escaping if the value contains *=* and no other case?

[filzrev]

Wrapping values with quotes instead just works?

It just works when using quoted argument. (both with/without % escape)

Is it documented anywhere?
No official documentation found that relating to this behaviors.
MSBuild command-line reference page has following statement.

If you run MSBuild from a shell other than the Windows command prompt, lists of arguments to a switch (separated by semicolons or commas) might need single or double quotes to ensure that lists are passed to MSBuild instead of interpreted by the shell.

And MSBuild team suggesting to use this workaround(dotnet/sdk#8792 (comment))

Why does comma need to be escaped also?

It's not listed on MSBuild special characters.
But comma can be used as separator char on some arguments. and requires quites/escape.

(List of argument that support comma can be found by search comma on https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-command-line-reference?view=vs-2022)

And why is it only escaping if the value contains = and no other case?

It's because other arguments seems to accepts raw ; separator char without quotes/escape.
(e.g. -getProperty:Prop1;Prop2 -noWarn:123;456)

[filzrev]

Wrapping values with quotes instead just works?
It just works when using quoted argument. (both with/without % escape)

I've confirmed it's not works when escaped ; separator chars(%3B) are passed.

It works when running from command line.
But when running via .bat file. It need to escape %3B to %%%%3B for .bat file. (double escape is required because BenchmarkDotNet use call to invoke dotnet command)

I thought this issue also applied to PR #2729.

It might be better to keep default MSBuildArgument behavior.
And enable escape/quotes as opt-in options.

I'll update this PR later.

[timcassell]

In that case, would it be better to simply wrap the entire argument with quotes? We don't actually use bat files, we just run the command directly (I think those files are used for debug purposes only).

[filzrev]

In that case, would it be better to simply wrap the entire argument with quotes?

As far as I've confirmed. It's not works without % escapes when wrapping entire argument with quotes.

We don't actually use bat files, we just run the command directly (I think those files are used for debug purposes only).

Thanks for letting me know.
I've confirmed these behaviors with generated .bat/.sh with keepFiles options.
I also try to confirm behaviors with DotNetCliBuilder/DotNetCliCommand.

[filzrev]

I've updated MSBuildArgument escape logics.

Escape Logic changes

1. Argument quoting is unified to \" on both Windows/non-Windows environment.
2. Escape following 3 characters. (These char needs escaping when used inside \"{value}\" syntax)
   2.1. Space (%20)
   2.2. Double Quote (%22)
   2.3. BackSlash (%5C)
3. If escaped value contains [MSBuild Special characters]. return value surrounded with \".
   (Otherwise returns original argument)

Other changes

1. Change MsBuildArgument escaping feature to opt-in
Escaping feature is enabled when passing escape: true to constructor.

2. Add integration tests to BenchmarkDotNet.IntegrationTests.ManualRunning projects.
Add tests to verify MSBuildArgument escaped value is properly passed as MSBuild property.

[timcassell]

I think this would be better as a combo of #2729 (escape all special characters opt-in) and #2732 (new type specifically for MsBuild properties). I do like how you handle the separation of display and command line here, though.