WIP Initial Trimming Support

[phil-scott-78]

• Marked Spectre.Console and Spectre.Console.Cli as trimmable to enable warnings
• Marked Demo.cspoj and Injection.cspoj to be PublishedAOT and to include all trim warnings

---

Spectre.Console

• Marked Spectre.Console methods that used TypeConverter with suppressed messages
• Marked ExceptionFormatter as requiring dynamic code and added a fallback for AOT scenarios.

---

Spectre.Console.Cli

• Settings were discovered automatically with reflection. Added a new method for adding commands that allows explicit configuration of the settings to prevent them from being trimmed. Marked the old method as requiring dynamic code and pointed users to the new overload
• Added attributes to the default TypeRegister to mark the types as being instantiated dynamically.
• Suppressed a whole slew of warnings with command bulding and running the commands. With the commands and their settings being marked as having DynamicallyAccessedMembers this, theoretically, should work without error.

Biggest worry is in CommandParameterComparer. The MetadataToken is stripped when published as AOT so it can't be used. A simple equal seems to work the same, but I'm not sure the historical reason MetadataToken is used here. @patriksvensson, you remember? I suspect that's the cause of the unit test failures.

I only have a handful of projects to test with myself. I worked through the Injection.csproj and Demo.cspoj in the CLI examples and ensured they worked properly. Also verified cli explain and cli xmldoc still worked when trimming.

This would close #955, #1401 and #1155. Those issues have a lot of good info to get me going, and any feedback on this from @azchohfi, @Gnbrkm41, @CyberSinh , @ricardoboss and @Simonl9l

Also, I apologize for all the #if NET6_0_OR_GREATER. Working around netstandard support really uglied some of these method signatures up.

---

Please upvote :+1: this pull request if you are interested in it.

[phil-scott-78]

Disregard the failed unit tests, that was due to multiple AddCommands and I was lost in the weeds thinking I broke something with that property comparer. Those are fixed, but I'm still convinced I broke something with the comparer, but I'm not sure what...

[patriksvensson]

Awesome work. Now comes the big question, how do we know what to update when we make changes and/or additions. Otherwise the trimming support will slowly become more and more unusable. Could we document this somewhere?

[phil-scott-78]

MS confirmed the bug with NativeAOT. I cleaned up the commit to be explicit about why there is a work around, separate from the main AOT work.

Regarding the future of maintenance once this is enabled, with <IsTrimmable>true</IsTrimmable> set, the compiler will alert us to what we need to do moving forward. If you add code that uses reflection, it'll warn you to make sure you mark your code appropriately with the DynamicallyAccessedMembers attribute. Really the only way things could get screwy is if someone went into a method marked to suppress warnings and started using reflection on types we hadn't considered yet. A scenario like that is FlagValue where it is never instantiated directly so its constructor was getting trimmed out.

The biggest maintenance headache is trying to read the code, especially the generic argument definitions, with the #IF statements to work around NetStandard support.

But give me a bit to grab a beer and I'll add additional comments throughout, especially around FlagValue and where we've suppressed messages.

[patriksvensson]

@phil-scott-78 Sounds good to me!

[agocke]

My recommendation for making the attributes cleaner is using the polysharp nuget package and switch on the flag to generate runtime attributes.

I’ve also been working on annotation so I’ll take a look at this later. ~I’ll say that I’m pretty skeptical that CLI can be made safe without a source generator.~

Missed this

Added a new method for adding commands that allows explicit configuration of the settings to prevent them from being trimmed.

yeah that sounds right. Automatic config probably doesn’t work, but manual should be fine.

[phil-scott-78]

Love the idea of polysharp.

[phil-scott-78]

I'm sold on polysharp. Added it and reworked the PR to use it.

[patriksvensson]

If we're going to use PolySharp, we should probably remove the other polyfill libraries that we use.

[patriksvensson]

Also, might be a good idea to create a new demo where trimming is enabled and leave the current demo code as-is.

[phil-scott-78]

The final four games were pretty boring last night, and the boys went to bed in a reasonable manner, so I had time to implement @agocke's suggestion on polysharp and tweaking some of the attributes. Pushed up those changes.

I'll create a new DemoAOT project. I think that's a good idea. Not only does it keep the original there, having one explicitly with AOT will lead to (hopefully) better discovery.

[phil-scott-78]

Added the new DemoAOT project to demonstrate how to configure the project. One hang up right now is the string constructor trick @0xced included a while back where we automatically convert types with a single param string constructor (e.g. DirectoryInfo). I added an example on how to work around that, but it'd be nice if there was a source generator that could produce those required attributes to keep the compiler from stripping those dynamically called constructors out.

[patriksvensson]

@phil-scott-78 Is this ready for review, or do you have more things to add/fix/change?

[agocke]

One more thing to consider is, rather than a demo app, you can use a "test app" to have the linker/aot compiler verify all the dependencies. Instructions are at https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming?pivots=dotnet-8-0#show-all-warnings-with-test-app but the key part is adding TrimmerRootAssembly that causes the trimmer to treat all entry points into a library as being reachable.

Here's the code for an AOT test app that I wrote and put in the test folder:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>

    <PublishAot>true</PublishAot>
    <!-- Show warnings from dependencies, not just from the app -->
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
  </PropertyGroup>

  <ItemGroup>
    <ProjectReference Include="../../src/Spectre.Console.Cli/Spectre.Console.Cli.csproj" />
    <TrimmerRootAssembly Include="Spectre.Console" />
    <TrimmerRootAssembly Include="Spectre.Console.Cli" />
  </ItemGroup>

</Project>

[phil-scott-78]

Before review, I definitely want to try out that @agocke test project. I had a few minutes to use a computer today and added it only to find a few more warnings.

[agocke]

I’ve tried to do some annotation on my own. I could share it but don’t know the best way. Comments in this PR? New PR?

[phil-scott-78]

I’ve tried to do some annotation on my own. I could share it but don’t know the best way. Comments in this PR? New PR?

If it's a handful of improvements, comments on the PR should work. You could also do a PR to my fork too, I suppose.

Honestly, whatever feedback you can give I'm excited for, so what's easiest for you to communicate it.

[agocke]

Pushed all my changes to https://github.com/agocke/spectre.console/tree/cli-trimmable. There are a lot of CLI annotations, but I'm not sure there's a simple alternative.

[codymullins]

@agocke pulled in your code to my project - it looks like we need to add [DynamicallyAccessedMembers] to the ITypeRegistrar. I didn't see this attribute being used already, are we doing this in some other way?

Without the above, trimming doesn't seem to work when integrating with Microsoft.Extensions.DependencyInjection:

public class DependencyInjectionRegistrar(IServiceCollection services) : ITypeRegistrar, IDisposable
{
    // ... omitted code

    public void Register(Type service, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementation)
    {
        Services.AddSingleton(service, implementation);
    }

    // ...
}

[agocke]

@codymullins did you also pull the project file changes? Polysharp adds the polyfill for the attributes.

[codymullins]

I forked the repo...not familiar with Polysharp. Let me investigate if I forked anything up 😄

edit - yes, looks like I'm the problem here. the build is failing but I'm sure that's also me - I'll retest and update.

[agocke]

Wanted to give an update on where I am with this: I think I've got the annotations in a good place locally, and they make plain Spectre.Console usable, minus the changes to TypeConverter.

Spectre.Console.Cli is another story. The annotations make it unusable -- all the current APIs are not trim safe. Unfortunately, I don't see a way to use the existing APIs to manually configure it, either. Reflection is deeply baked into the API surface.

What I've started is defining a new parallel set of APIs that allow a combination of configuration and reflection, that could eventually be implemented by a source generator.

Right now I'm kind of stuck on Bind. There's both a lot of reflection in there and a lot of currently internal surface area, so lifting out a new API is taking a while.

An intermediate state that I think would be good is just annotating the current surface. That would make it clear what's usable and what's not. And we need to define a path forward for the TypeConverter behavior in Spectre.Console. The existing TypeConverter is a non-starter.

[phil-scott-78]

I happened to have a chance to look at this today and I also landed at your idea of the intermediate step. I don't think AOT perfection is possible but given a handful of well-placed annotations it does become quite difficult to break things. Trickiest thing, to me at least, is the magic for trying to call a constructor for a type with a single string param constructor.

Other than that, most cases seem to chug along, right? Intrinsic types have their TypeConverter handled pretty well in .NET 8. Custom types we've always pushed people to annotate their fields with a TypeConverter annotation which has the right annotations itself for including the proper DynamicallyAccessedMemberTypes. Once we get past that, we've never really supported anything overly complex out of the box like List<T> or anything beyond arrays of intrinsic types, which seem to work fine.

Of course, there might be countless other things I'm missing here. Could you provide some examples of things that absolutely would not work with the Cli project because of the current TypeConverter? Even just documenting those might be a good step, and maybe improve the error messages too.

[agocke]

The problem is that debugging AOT-specific problems is basically impossible. Our guidance for AOT-safety is to either guarantee behavior, or mark it as unsafe. Suppressing warnings that then bite users during deployment isn't really doing any favors.

I do think I can build a new API for the CLI layer, it's just going to take a while.

The only other option I can think of is explicitly throwing NotSupported exceptions for unsupported features. But that still means -- avoid suppressions. Either use a feature switch like I have in my branch for type converter, or just straight change the behavior (even in non-trimmed scenarios), to stop supporting complex reflection behavior.

[agocke]

Could you provide some examples of things that absolutely would not work with the Cli project because of the current TypeConverter?

So, the big problem with TypeConverter is just that GetConverter is fundamentally unsafe due to reliance on ICustomTypeDescriptor as a fallback. My alternate implementation works around this by implementing a GetConverter method that only uses the TypeConverter attribute and doesn't respect ICustomTypeDescriptor.

Beyond that we still run into problems in binding. GetConverter is a good example

if (parameter.ParameterType.IsArray)
{
    // Return a converter for each array item (not the whole array)
    var elementType = parameter.ParameterType.GetElementType();
    if (elementType == null)
    {
        throw new InvalidOperationException("Could not get element type");
    }

    return (TypeDescriptor.GetConverter(elementType), GetStringConstructor(elementType));
}

Let's say you replace that GetConverter call with a call to my "safe" version that has DynamicallyAccessedMembers on it. That doesn't help because the annotations don't allow you to annotate array element types, just the array itself. So the return type of parameter.ParameterType.GetElementType() will never satisfy those annotations. What we can do instead is pass along an extra piece of information: as part of the parameter we not only keep the parameter Type, we also keep the element Type. The constructor for CommandParameter would require you to pass both.

The trick for this is it just becomes very verbose for the user, and really removes the benefit of doing reflection at all. The user ends up typing a bunch of boilerplate code. Hence the source generator.