System.CommandLine defaults have large dependency closure

[jkotas]

trafficstars

Repro

dotnet publish -c Release -r win-x64 /p:PublishTrimmed=true

using System.CommandLine;

internal class Program
{
    private static int Main(string[] args)
    {
        var fileOption = new Option<FileInfo?>(
            name: "--file",
            description: "The file to read and display on the console.");

        var rootCommand = new RootCommand("Sample app for System.CommandLine");
        rootCommand.AddOption(fileOption);

        rootCommand.SetHandler(
            (FileInfo file) => ReadFile(file),
            fileOption);
        return rootCommand.Invoke(args);
    }

    private static void ReadFile(FileInfo file)
        => File.ReadLines(file.FullName).ToList()
            .ForEach(line => Console.WriteLine(line));
}

Actual result:

The trimmed application has referenced to many types and binaries that should not be needed:

System.Private.Uri.dll
System.Net.Primitives.dll
System.ObjectModel.dll
System.ComponentModel.dll
System.ComponentModel.Primitives.dll
System.ComponentModel.TypeConverter.dll
System.Collections.NonGeneric.dll
System.Runtime.Serialization.Formatters.dll
System.Diagnostics.Process.dll
...

Expected result:

The trimmed application should only reference types and binaries that are actually needed.

[jkotas]

Context: https://github.com/dotnet/runtime/pull/72082#issuecomment-1185706575

[adamsitnik]

We have made some progress recently, but we are definitely not done yet:

System.Private.Uri.dll
System.Net.Primitives.dll
- System.ObjectModel.dll
- System.ComponentModel.dll
- System.ComponentModel.Primitives.dll
- System.ComponentModel.TypeConverter.dll
- System.Collections.NonGeneric.dll
System.Runtime.Serialization.Formatters.dll
- System.Diagnostics.Process.dll

[adamsitnik]

My guess is that all of the redundant references come from here:

https://github.com/dotnet/command-line-api/blob/c63e231c1e5102c51bd9247a4f3feec9175d601d/src/System.CommandLine/Binding/ArgumentConverter.StringConverters.cs#L14

which is just a dictionary of parsers that in theory can be used by the app:

https://github.com/dotnet/command-line-api/blob/c63e231c1e5102c51bd9247a4f3feec9175d601d/src/System.CommandLine/Binding/ArgumentConverter.StringConverters.cs#L172-L182

Since Argument and Option are generic now, we could introduce a parse method similar to:

internal bool TryParse(string token, out T value)
{
    if (typeof(T) == typeof(IPAddress))
    {
        if (IPAddress.TryParse(token, out var ip))
        {
            value = (T)(object)ip;
            return true;
        }

        value = default;
        return false;
    }
    // else if (typeof(T)...
}

@jkotas Does trimmer recognize such typeof checks? If not, how can I get rid of these references?

[am11]

@adamsitnik, very nice! Should/can we update https://github.com/dotnet/runtime/blob/f561a058b13523c4b045803c9876e823b4cfe0fc/eng/Versions.props#L174 to latest revision? It's four months old and seems like we don't have a darc subscription in runtime repo for command-line-api (dotnet/format, dotnet/sdk etc. have such a subscription).

[jkotas]

Does trimmer recognize such typeof checks?

Trimmer does not recognize patterns like these.

If not, how can I get rid of these references?

This is yet another deserializer. You can look at e.g. how we have solved this problem for System.Text.Json deserializer. The general shape of the solution is:

• Avoid collections of built-in deserializers or discovering deserializers using reflection.
• Expose each built-in deserializer as a public entrypoint with concrete types.
• Use source generators to generate the glue code if the entrypoints with concrete types are cumbersome to use.

[adamsitnik]

Should/can we update

We should, but I should not do it myself, as I introduced all the breaking changes and they are reasonable and easy to fix for me.

I am about to write a doc that lists all the breaking changes, explains the reasoning behind them and how to solve the issues.

@am11 would you be interested in updating dotnet/runtime once I have the doc ready? And share some feedback about whether the changes are move in the right direction or not?

[adamsitnik]

With #2127 the only dependency is System.Linq

And updated perf numbers for startup scenario with default settings (all features enables, https://github.com/adamsitnik/commandline-perf/tree/update):

Method	Args	Mean	Error	StdDev	Median	Ratio	RatioSD
Empty		65.59 ms	0.297 ms	0.875 ms	65.44 ms	0.79	0.01
EmptyAOT		12.95 ms	0.132 ms	0.388 ms	12.88 ms	0.16	0.01
Corefxlab		82.84 ms	0.347 ms	1.022 ms	82.77 ms	1.00	0.02
SystemCommandLine2021		113.17 ms	0.704 ms	2.075 ms	112.73 ms	1.36	0.03
SystemCommandLine2022		94.72 ms	0.420 ms	1.240 ms	94.37 ms	1.14	0.02
SystemCommandLineNow		82.94 ms	0.346 ms	1.019 ms	82.81 ms	1.00	0.00
SystemCommandLineNowR2R		77.37 ms	0.357 ms	1.054 ms	77.22 ms	0.93	0.02
SystemCommandLineNowAOT		14.55 ms	0.118 ms	0.348 ms	14.54 ms	0.18	0.00
Empty	--bool -s test	65.06 ms	0.409 ms	1.205 ms	64.84 ms	0.74	0.02
EmptyAOT	--bool -s test	13.01 ms	0.139 ms	0.411 ms	12.94 ms	0.15	0.00
Corefxlab	--bool -s test	84.97 ms	0.791 ms	2.333 ms	84.66 ms	0.97	0.03
SystemCommandLine2021	--bool -s test	117.71 ms	0.624 ms	1.841 ms	117.39 ms	1.34	0.02
SystemCommandLine2022	--bool -s test	99.46 ms	0.375 ms	1.107 ms	99.15 ms	1.14	0.02
SystemCommandLineNow	--bool -s test	87.58 ms	0.332 ms	0.979 ms	87.46 ms	1.00	0.00
SystemCommandLineNowR2R	--bool -s test	79.16 ms	0.341 ms	1.006 ms	79.17 ms	0.90	0.02
SystemCommandLineNowAOT	--bool -s test	14.88 ms	0.170 ms	0.503 ms	14.85 ms	0.17	0.01

[KalleOlaviNiemitalo]

Expose each built-in deserializer as a public entrypoint with concrete types.

This doesn't seem to have been done, so anyone using Argument<Uri> will have to set a custom parser function and custom localization of error messages.

[adamsitnik]

This doesn't seem to have been done, so anyone using Argument<Uri> will have to set a custom parser function and custom localization of error messages.

That is true. But in the long term (.NET 9) we are going to provide a source-generator for System.CommandLine and all parsable types will be supported out of the box.