Fix trimming runtime error

[xoofx]

Hey!

I discovered that CommandLineUtils is not trimming compatible with net6.0 and will fail at runtime with an error like:

Unhandled exception. System.MissingMethodException: Constructor on type 'System.ComponentModel.DataAnnotations.RequiredAttribute' not found.
   at System.RuntimeType.CreateInstanceImpl(BindingFlags , Binder , Object[] , CultureInfo )
   at System.Activator.CreateInstance(Type , BindingFlags , Binder , Object[] , CultureInfo , Object[] )
   at McMaster.Extensions.CommandLineUtils.ValidationExtensions.GetValidationAttr[T](String , Object[] )
   at McMaster.Extensions.CommandLineUtils.ValidationExtensions.IsRequired(CommandArgument , Boolean , String )
   at GrpcCurl.GrpcCurlApp.Run(String[] args)
   at Program.<Main>$(String[] args)
   at Program.<Main>(String[] args)

I have been able to workaround it here but it's not super great.

// Workaround for CommandLineUtils with trimming
// Force RequiredAttribute ctor to be kept around
if (new RequiredAttribute().GetHashCode() > 0)
{
    exeName += "";
}

Which is really unfortunate because we usually want to use trimming for small console apps. 😅

This PR is fixing this issue with these attributes used via Activator.CreateInstance and use instead explicit constructors. The previous abstraction GetValidationAttr<T> was removed.

It should make CommandLineUtils a bit more compatible with net6.0 trimming.

I tried to enable trimming analysis by adding net6.0 target... but wow, that triggers quite a few errors. Not a lot, but as it gets viral, it might require more deeper thinking on this.

At least, this PR fixes a basic usage of this library.

[natemcmaster]

Thanks for trying to solve the issue. However, I don't think this approach is a change I can accept. Adding the new() constraint on generic type parameters is a breaking change for users -- anyone using constructor injection would be required to add a public parameterless constructor.

[xoofx]

Thanks for trying to solve the issue. However, I don't think this approach is a change I can accept. Adding the new() constraint on generic type parameters is a breaking change for users -- anyone using constructor injection would be required to add a public parameterless constructor.

I'm sorry. Could you clarify which new()?

All the constraints for e.g where TApp : class, new() were ultimately calling this constructor

                return Activator.CreateInstance<TModel>();

which is just the strict equivalent of the following at compile time:

                return new TModel();

I can keep the original Satisfies around if you want, but the internal code would not use it.
It would not be a breaking change in that case.

But for the other (for TApp), it's better to have compile time validation than runtime failure (e.g Activator.CreateInstance<TModel>(); is parameter less)

[xoofx]

Oh, that's because you have a CommandLineApplication<TModel>.ModelFactory. Ok, I can remove the changes related to this then.

[xoofx]

I pushed a new commit that is reverting most of the changes and fixing only my original issues. Doesn't break the existing API nor it does introduce a new API.
Hope it's ok that way.

[natemcmaster]

Yes, thank you. The new update looks much better. And you confirmed this resolves your runtime trimming issue? I haven't played with this feature of .NET 6 before, so don't know if there is a good way we can add tests to ensure this doesn't regress in the future.

[natemcmaster]

It looks like this causes some unit tests to fail in the CI workflow. Is this working on your computer?

[xoofx]

It looks like this causes some unit tests to fail in the CI workflow. Is this working on your computer?

My apologize, I pushed a fix.

[xoofx]

Oh, one error with formatting, let me fix this

[codecov]

Codecov Report

Merging #491 (aef16d7) into main (1cb2c24) will increase coverage by 0.07%.
The diff coverage is 79.31%.

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
+ Coverage   82.21%   82.28%   +0.07%     
==========================================
  Files         106      106              
  Lines        3289     3302      +13     
==========================================
+ Hits         2704     2717      +13     
  Misses        585      585              

Impacted Files	Coverage Δ
...ommandLineUtils/Validation/ValidationExtensions.cs	65.16% <79.31%> (+4.91%)	⬆️
...ils/Attributes/VersionOptionFromMemberAttribute.cs	95.45% <0.00%> (+0.45%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e81e13b...aef16d7. Read the comment docs.

[xoofx]

Just pushed a commit to try to improve slightly the coverage of ValidationExtensions.cs

[natemcmaster]

Thanks @xoofx! This will be in the 4.0.1 patch.

[xoofx]

Thanks @xoofx! This will be in the 4.0.1 patch.

Cool, thanks a lot, I could remove my workaround! 😉