[Feature]: Trimmable DotNet Project

[ds5678]

This pull request enables trimmable support in the AsmResolver.DotNet module by moving the problematic code into a new non-trimmable project AsmResolver.DotNet.Dynamic.

[Washi1337]

Overall I like the idea and I can see the use of splitting off the dynamic reading from the base package, but as it is right now I don't think it can be merged just yet.

• The use of InternalsVisibleTo is something that really should be avoided in my opinion, and I don't believe it is necessary in this case.
• There are still some warnings that were introduced by marking AsmResolver.DotNet trimmable.
• Few other minor comments can be found in the review.

Side Note; this is a structural breaking change and thus has to wait for a major version bump to be merged. We might need to make specific branches for v4 and v5 if we want to work on both of them simultaneously.

[ds5678]

There are still some warnings that were introduced by marking AsmResolver.DotNet trimmable.

Which warnings are you referring to? Looking at the app veyor log, it seems there are only 4 warnings total:

C:\projects\asmresolver\src\AsmResolver.DotNet\AssemblyDescriptor.cs(233,47): warning SYSLIB0007: 'HMAC.Create()' is obsolete: 'The default implementation of this cryptography algorithm is not supported' [C:\projects\asmresolver\src\AsmResolver.DotNet\AsmResolver.DotNet.csproj]

C:\projects\asmresolver\test\AsmResolver.DotNet.Tests\Bundles\BundleManifestTest.cs(240,19): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. [C:\projects\asmresolver\test\AsmResolver.DotNet.Tests\AsmResolver.DotNet.Tests.csproj]

C:\projects\asmresolver\test\AsmResolver.DotNet.Tests\ManifestResourceTest.cs(68,19): warning CS0618: 'ManifestResource.GetReader()' is obsolete: 'Use TryGetReader instead.' [C:\projects\asmresolver\test\AsmResolver.DotNet.Tests\AsmResolver.DotNet.Tests.csproj]

C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(499,5): warning MSB8004: Output Directory does not end with a trailing slash.  This build instance will add the slash as it is required to allow proper evaluation of the Output Directory. [C:\projects\asmresolver\test\TestBinaries\Native\CallManagedExport\CallManagedExport.vcxproj]

[ds5678]

We might need to make specific branches for v4 and v5 if we want to work on both of them simultaneously.

If you make a v5 branch, I can redirect this onto that branch.

[Washi1337]

Which warnings are you referring to? Looking at the app veyor log, it seems there are only 4 warnings total:

AppVeyor might be deceiving here, since it does not test for publishing as self-contained trimmed bundles. If you try to publish a simple program that just reads and writes a module, you will be notified of problems in DotNetCorePathProvider.cs, ModuleDefinition.cs and TypeSignature.cs, even without explicitly using these functions.

The first two can be surpressed I think. The latter, however, is related to the actual DynamicMethodDefinition part, as it is resolving Type::GetTypeFromHandleUnsafe to be able to parse type signatures marked with COR_ELEMENT_TYPE_INTERNAL (which only occur in the signatures and bodies of dynamic methods).

[ds5678]

What would be the best way to make that part of the continuous integration?

[Washi1337]

What would be the best way to make that part of the continuous integration?

I am not sure if it is possible directly in an appveyor.yml, as these warnings are only triggered when trying to publish a consumer project. There's the option for custom build scripts which we may be able to use to manually invoke dotnet publish, but I am not sure if it is configurable in such a way that AppVeyor will also parse out and display all the warnings from the raw output. I will have to investigate this more. Maybe you (or someone else) have some suggestions on this as well?

[ds5678]

Maybe you (or someone else) have some suggestions on this as well?

Would you be willing to switch from AppVeyor to GitHub Actions? It's very customizable.

Also, it may be a good idea to switch the default branch from master to development. GitHub uses the default branch for most continuous integration scripts, so switching allows your changes to have immediate effects without first being merged into the master branch.

[Washi1337]

I am fine with using github actions, but whether we use actions or appveyor isn't going to change the fact that this test is going to require a publish on a project that uses AsmResolver.DotNet (i.e. not a publish of a core package itself).

I don't really see a need of switching default branches if the pipeline can be configured to work on multiple branches. I'd rather see the master branch as the front page of the project, since it is the version that most people will be using in the end.

[Washi1337]

As far as I can see, I think to fix the warnings happening in TypeSignature, we need to do the following:

• Remove the ElementType.Internal case from TypeSignature.FromReader, and add a new method specifically for use in the context of dynamic method body reading, that would also consider this element type. We might want to include a reader.PeekByte(), so that we can do something in the lines of the following:

  public static TypeSignature FromDynamicReader(in BlobReadContext context, ref BinaryStreamReader reader)
  {
     if ((ElementType) reader.PeekByte() == ElementType.Internal)
         return ReadInternalTypeSignature(...);
     else
         return TypeSignature.FromReader(in context, ref reader);
  }

• This new type signature reader method I think is only meant to be called within LocalVariableSignature.FromReader, called via CallingConventionSignature.FromReader in DynamicMethodHelper.ReadLocalVariables. Everything else within DynamicMethodDefinition regarding types is constructed by a ReferenceImporter, which should now be free of these problems. We would therefore need a similar alternative to LocalVariableSignature.FromReader that would consider these modified versions of the reader.
• Remove TypeSignature::GetTypeFromHandleUnsafeMethod and move it along with the new FromDynamicReader.

[Washi1337]

It seems like it is possible to run dotnet publish in AppVeyor as a custom script in e.g. the after_test phase, but unfortunately it won't register the warnings in the compiler output as actual messages that appear in the "Messages" tab. Seems like this might be a reason to switch to GitHub actions after all. For now, I will do the testing manually for this PR, but this is definitely something we should look into in the future.