Skip to content
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.

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

POC - Refactor CLI Project for AOT Compatibility and Trimming Support #1706

Closed
wants to merge 1 commit into from

Conversation

phil-scott-78
Copy link
Contributor

@phil-scott-78 phil-scott-78 commented Dec 1, 2024

This commit is a proof of concept showing a potential path to refactoring the Spectre.Console.Cli project to replace reflection-based object creation with explicit and type-safe constructs to enable support for Ahead-Of-Time (AOT) compilation and code trimming. AOT mode is only enabled for .NET 9. One change over previous attempt that I think allowed for success (or at least not as big of a flop):

  • Feature Switches added in .NET 9 to keep the code much cleaner and explicit about how we are handling things with dynamical instantiation. Compared to previous attempts there are very few #IF preprocessors and uses of UnconditionalSuppressMessage attributes.

  • Attacking it with a better understanding of how reflection is used. I moved around quite a bit with the idea that Settings have properties that are either

    • Intrinsic types
    • Single parameter classes
    • Arrays of intrinsic types
    • Dictionaries with a string key and an intrinsic value

    Using that I was able to create look ups that allows avoiding reflection

It involves multiple changes to critical areas, and there isn't an AOT xunit runner. The only way to verify things still work in AOT mode might be to add scenarios to the TrimTest project to try and break it when published. It's a tedious, tedious process. I suspect someone smarter than me could figure out how to bootstrap the xunit.runner.console package then get CI set up for publishing and running those tests manually, but I gave it a go and came up short.

Changes

Introduce DynamicallyAccessedMembers Annotations:

  • Annotated various types and generic parameters (e.g., CommandApp, CommandSettings) with [DynamicallyAccessedMembers] to ensure necessary metadata is preserved during trimming for AOT scenarios.

Explicit Object Creation:

  • Introduced CreateInstanceHelpers to encapsulate type-safe and explicit instance creation logic. This replaces Activator.CreateInstance for array, multimap, and command settings instantiation.

Refactor Command Attributes:

  • Updated constructors of CommandArgumentAttribute and CommandOptionAttribute to explicitly accept Type for argument and option types, with corresponding logic to register instance builders for these types.

Remove Reflection-Heavy Logic:

  • Replaced uses of Activator.CreateInstance across multiple files with explicit methods from CreateInstanceHelpers.
  • Removed reflection-reliant dynamic code annotations ([RequiresDynamicCode]) from critical paths where alternatives are now implemented.

Enhanced Type Conversion:

  • Consolidated type conversion logic into TypeConverterHelper with static converters for intrinsic and complex types.
  • Ensured compatibility by defaulting to explicit conversion mechanisms where possible.

Array and Collection Initialization:

  • Migrated array and collection initialization to CreateInstanceHelpers to handle both primitive and complex types without runtime reflection.

Builder and Registrar Adjustments:

  • Refactored ITypeRegistrar and related classes to include explicit type member annotations, ensuring required metadata survives trimming.
  • Adjusted configurators (Configurator, IConfigurator) to support type-safe default and added commands.

Testing Adjustments:

  • Added internal visibility to support AOT testing scenarios.
  • Introduced new project files for trimming-specific tests.

Risks

Unit Testing:

  • I tried my best to figure out if I could get the TrimTest console project to run the Cli unit tests when published to AOT via xunit.runner.console, but I'm just not smart enough. I'd feel infinitely better about things if it could do so. As is, the features included there do all work. This project is needed to light up the analyzers properly.

Explicit Type Requirement:

  • Attributes now require explicit Type definitions for non-intrinsic types (e.g., DirectoryInfo). Users must specify the argumentType or optionType parameter for such types. Failure to do so will result in runtime exceptions or incorrect behavior.

Compatibility with Existing Registrations:

  • Type Registrar Changes: Registrations now require explicit [DynamicallyAccessedMembers] annotations on implementation types. Users with their own custom registration and resolver will need to adapt to match.

Array Parameters:

  • Users who, for whatever reason, might have created their own structs and are using them as IEnumerable parameters are going to get failures in AOT scenarios. We can't dynamically create those arrays.

Developer experience moving forward:

  • There is no xunit test running that will do things when published in AOT, requiring a seperate step to publish the TrimTest project and see what warnings are there and I suppose to ensure it still works. Right now we have no analyzer warnings, and theoretically that means there is a promse the code will work the same in AOT mode as non, but still a risk. Actual maintenance should be straightforward - the analyzers should warn when trying to use reflection, which isn't something we do much anymore now that the project is stable. A DI refactoring will need to be treated with care though.

I have two kids and no time, why can't I have no kids and two time?

  • With so many changes, I feel like this kind of release should be done as a separate POC package created from a long running branch. But that would require someone with time to keep that branch in sync with the main branch until enough people have had time to kick the tires. This month has been some of the first time I've been able to hop on and knock some stuff out in maybe over a year, and I can't guarantee I'll have time like that until next fall. I would not be upset if this PR just sat until the 1.0 release and some of the ideas get incorporated along with DI in one big 2.0-preview release

Summary: This commit refactors the Spectre.Console.Cli project to replace reflection-based object creation with explicit and type-safe constructs to enable support for Ahead-Of-Time (AOT) compilation and code trimming. AOT mode is only enabled for .NET 9.

Introduce DynamicallyAccessedMembers Annotations:
* Annotated various types and generic parameters (e.g., CommandApp, CommandSettings) with [DynamicallyAccessedMembers] to ensure necessary metadata is preserved during trimming for AOT scenarios.

Explicit Object Creation:
* Introduced CreateInstanceHelpers to encapsulate type-safe and explicit instance creation logic. This replaces Activator.CreateInstance for array, multimap, and command settings instantiation.

Refactor Command Attributes:
* Updated constructors of CommandArgumentAttribute and CommandOptionAttribute to explicitly accept Type for argument and option types, with corresponding logic to register instance builders for these types.

Remove Reflection-Heavy Logic:
* Replaced uses of Activator.CreateInstance across multiple files with explicit methods from CreateInstanceHelpers.
* Removed reflection-reliant dynamic code annotations ([RequiresDynamicCode]) from critical paths where alternatives are now implemented.

Enhanced Type Conversion:
* Consolidated type conversion logic into TypeConverterHelper with static converters for intrinsic and complex types.
* Ensured compatibility by defaulting to explicit conversion mechanisms where possible.

Array and Collection Initialization:
* Migrated array and collection initialization to CreateInstanceHelpers to handle both primitive and complex types without runtime reflection.

Builder and Registrar Adjustments:
* Refactored ITypeRegistrar and related classes to include explicit type member annotations, ensuring required metadata survives trimming.
* Adjusted configurators (Configurator, IConfigurator) to support type-safe default and added commands.

Testing Adjustments:
* Added internal visibility to support AOT testing scenarios.
* Introduced new project files for trimming-specific tests.

Risks
=========================
Unit Testing: I tried my best to figure out if I could get the TrimTest console project to run the Cli unit tests when published to AOT via xunit.runner.console, but I'm just not smart enough. I'd feel infinitely better about things if it could do so. As is, the features included there do all work. This project is needed to light up the analyzers properly.

Explicit Type Requirement: Attributes now require explicit Type definitions for non-intrinsic types (e.g., DirectoryInfo). Users must specify the argumentType or optionType parameter for such types. Failure to do so will result in runtime exceptions or incorrect behavior.

Compatibility with Existing Registrations:

Type Registrar Changes: Registrations now require explicit [DynamicallyAccessedMembers] annotations on implementation types. Users with their own custom registration and resolver will need to adapt to match

Array parameters: Users who for whatever reason might have created their own struct and are using them as IEnumerable parameters are going to get failures in AOT scenarios. We can't dynamically create those arrays.
@phil-scott-78 phil-scott-78 added the area-CLI Command-Line Interface label Dec 1, 2024
@phil-scott-78
Copy link
Contributor Author

An analyzer/fix could resolve the issue with AOT, but it seems there are about 400k total downloads of the analyzer vs 3.3 million for the Cli package.

image

@phil-scott-78
Copy link
Contributor Author

forgot to include my requisite @agocke mention on this

@patriksvensson
Copy link
Contributor

An analyzer/fix could resolve the issue with AOT, but it seems there are about 400k total downloads of the analyzer vs 3.3 million for the Cli package.

image

We could probably include the analyzer automatically with the CLI package.

Side note: What is that horrible font? 😄

@phil-scott-78
Copy link
Contributor Author

phil-scott-78 commented Dec 2, 2024

Comic Code. Seems to help with my dyslexia when reading code. The ugly inconsistency keeps everything from kinda blending together, but I can see how it would be an affront to scandinavian design sensibilities

@patriksvensson
Copy link
Contributor

@phil-scott-78 Ah, I didn't know! Interesting!

@agocke
Copy link

agocke commented Dec 2, 2024

Haven’t had time to look through fully, but I want to caution use of feature switches. They’re very useful, but they should be used carefully. Changing behavior drastically using a feature switch, without a corresponding trim warning, might surprise users when the behavior is much different than they expected. Ideally the feature switch is used to shift to an alternate, but compatible implementation. When that’s not possible, we prefer to outright disable features and throw at the earliest opportunity. Otherwise it’s very hard to figure out why things don’t work.

Our general philosophy is “trim warning or it works.”

@phil-scott-78
Copy link
Contributor Author

Had some time when the kids were at preschool and threw together a quick POC of an analyzer and fixes. Needs a decent amount of polish and work to get all the cases, but I had a bit of a battle figuring out how to run the analyzer only in AOT mode and also how to test that scenario. Wanted to push that up before I forget. https://github.com/phil-scott-78/spectre-console-analyzer/tree/aot-analyzer

Screen.Recording.2024-12-03.182436.mp4

@github-actions github-actions bot added the ⭐ top pull request Top pull request. label Dec 5, 2024
@github-actions github-actions bot mentioned this pull request Dec 5, 2024
@phil-scott-78 phil-scott-78 deleted the cli-aot branch December 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CLI Command-Line Interface ⭐ top pull request Top pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants