Marking libraries as IsTrimmable

[MichalStrehovsky]

Library or service name.
All libraries.

Is your feature request related to a problem? Please describe.
I've filed this as Azure/azure-libraries-for-net#1274 where our customer hit it, but this is probably applicable to a lot of the SDK assemblies.

I work on the .NET Runtime team and one of the customers I was working with had trouble with excessive size of their app. Looking at their app, I saw that a lot of the big assemblies in their app are produced in this repo (e.g. Microsoft.Azure.Management.Network.Fluent.dll that has 4.7 MB).

The customer was using the PublishTrimmed .NET SDK option to remove unnecessary code from their app. By default, the .NET SDK removes unused code only from libraries that manifest themselves as trimming friendly. This is to prevent breaking code that uses unpredictable reflection (and might end up reflecting on parts of the program that were trimmed).

It would likely a be a good idea to manifest the Azure SDK libraries that are not particularly reflection heavy as trimming friendly. Manifesting them as such is easy - one just need to add an [AssemblyMetadata("IsTrimmable", "True")] attribute. The entire process is described at https://docs.microsoft.com/en-us/dotnet/core/deploying/prepare-libraries-for-trimming. Would it be possible to mark the SDK libraries as such? It would be especially helpful for libraries that are over 1 MB, but everything helps.

Cc @vitek-karas @sbomer who work on IL Trimming in .NET.

[jsquire]

Thank you for your feedback. Tagging and routing to team members for discussion and consideration.

[jsquire]

//cc: @AlexGhiondea, @weshaggard, @maririos

[vitek-karas]

If possible assemblies should be marked as trimmable only once all of the trimmer related warnings are resolved in them. Currently the only way to get all of the warnings is to use the library in an app and trim that app - the setup is described here https://docs.microsoft.com/en-us/dotnet/core/deploying/prepare-libraries-for-trimming#show-all-warnings.

[pakrym]

Most new libraries have a limited amount of reflection in them. There is some in the core but we can annotate it appropriately.

What worries me more is how do we test libraries and ensure that trimmed versions are functional.

@vitek-karas @MichalStrehovsky what testing strategies do you employ for BCL libraries?

[vitek-karas]

We mostly rely on the trimmer warnings. We run the trimmer in a special way (similar to building an app using the libraries as mentioned above) and gather all warnings from the BCL. We then go and fix those warnings. In cases where the fix is complex/tricky we add a special trimming test - for example. These tests are basically tiny apps which are published with trimming and executed.

[pakrym]

@vitek-karas is there some doc describing how 3-rd parties can run the trimmer this way?

[vitek-karas]

The above page has that info - the runtime doesn't run it this exact way, but that's because runtime is special in many ways. We know it's not exactly user friendly right now... working on it...

[pakrym]

My bad. Not enough coffee. Missed the link.

Thank you for the information!

[heaths]

@vitek-karas is DynamicallyAccessMembersAttribute and DynamicallyAccessedMemberTypes referenced by trimming tools via strong types, or more like nullable attributes and some other attributes through the years that only need to match name.

Where we do use some reflection, in some cases it's for potentially critical components. One solution, if we go down this route, is to document that those components are not supported in trimmed environments, but if we can make it work that would be ideal. I know for most of our assemblies we are still trying to avoid multi-targeting.

[vitek-karas]

They are just like nullable attributes only recognized by namespace.typename. In runtime repo we already have several libraries which target netstandard and which compile the definitions into the library. You can literally copy the definition of the attribute from our repo. For example DynamicallyAccessedMembersAttribute.

[snakefoot]

Since .NET7 now uses <TrimMode>full</TrimMode> by default, then it is no longer necessary for library-owners to explicitly opt-in.

Does anyone know if NET7 linker will skip triming assemblies that specifies this attribute:

[AssemblyMetadata("IsTrimmable", "False")]

Right now the NET7 will perform the full trimming by default, and give a small IL2104-warning about something was discovered during trimming. All errors from the vague warning, can then be discovered during runtime as real errors.

[vitek-karas]

Since .NET7 now uses full by default, then it is no longer necessary for library-owners to explicitly opt-in.

That is only the default for console apps, other types of apps may have different defaults (for example MAUI, Xamarin, Blazor all default to partial). I think it would be good to still go through the process as described above.

[AssemblyMetadata("IsTrimmable", "False")] should tell the linker to treat the assembly as "copy" which means leave it as-is. Note that it won't "skip" it, it will still analyzer it, produce warnings from it and use its code to mark/keep other code, but it will keep the entry assembly as a hole. (@sbomer for confirmation of this).

[snakefoot]

[AssemblyMetadata("IsTrimmable", "False")] should tell the linker to treat the assembly as "copy" which means leave it as-is

Would be lovely if you could update documentation about this, since everything is very vague at the moment:

https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options

https://github.com/dotnet/linker/blob/main/docs/design/trimmed-assemblies.md

[sbomer]

[AssemblyMetadata("IsTrimmable", "False")] should tell the linker to treat the assembly as "copy" which means leave it as-is. Note that it won't "skip" it, it will still analyzer it, produce warnings from it and use its code to mark/keep other code, but it will keep the entry assembly as a hole. (@sbomer for confirmation of this).

That's actually not true - the only supported value right now is "True" and the linker will output a warning if you add this to an assembly: warning IL2102: Invalid AssemblyMetadata("IsTrimmable", "False") attribute in assembly 'trimmed'. Value must be "True"

Thanks for the feedback - I opened dotnet/docs#30790 to track the doc improvement.

[sbomer]

Right now the NET7 will perform the full trimming by default, and give a small IL2104-warning about something was discovered during trimming. All errors from the vague warning, can then be discovered during runtime as real errors.

In case this helps, you can set <TrimmerSingleWarn>false</TrimmerSingleWarn> to show detailed warnings when you publish an app. Also consider setting <IsTrimmable>true</IsTrimmable> in library projects, which will both show trim analyzer warnings and add [AssemblyMetadata("IsTrimmable", "True")] to the library.

[vitek-karas]

You're right @sbomer - I confused this with the MSBuild IsTrimmable metadata which does support both True and False.

[snakefoot]

So it is impossible to mark an assembly as IsTrimmable=false? Library maintainers (both open source and at Microsoft) are going to have fun explaining why their nuget-package are throwing unknown type exceptions.

[heaths]

Since .NET7 now uses <TrimMode>full</TrimMode> by default, then it is no longer necessary for library-owners to explicitly opt-in.

Is that for anything built with net7.0 (which we don't do yet) or that targets it? Presumably the latter, but want to confirm.

Still, while we do make little use of reflection like Pavel noted above, we still use it in some places e.g., Azure.Security.KeyVault.Keys for some of the cryptographic operations not defined by netstandard2.0 but available in roughly equivalent runtimes.

/cc @jsquire @pallavit if we want to act on this for semester planning.

[vitek-karas]

So it is impossible to mark an assembly as IsTrimmable=false?

Library maintainers (both open source and at Microsoft) are going to have fun explaining why their nuget-package are throwing unknown type exceptions.

I don't think that's the right view on this:
There are two important aspects:

• The developer will know that there's a potential problem - the publish process will produce a warning if it can't guarantee that it understands the code. Any runtime failure caused by missing type should be preceded by a warning during build/publish.
• Turning off trimming for one assembly doesn't actually fix the problem. Frequently assembly A uses dangerous reflection which works on types from assembly B. Not trimming A will not fix that problem, since the types are being removed from B. The above mentioned reflection usage to get to certain crypto algorithms is exactly that - not trimming the Azure SDK assembly will not preserve the crypto algorithm type it needs which lives in the core framework.

Side note: There are ways to tell the system to not trim an assembly already - it's just not an attribute. There's one way in MSBuild, and then there's a way to embed a certain specific XML (linker descriptor) into the assembly to do the same. But we generally don't tell people about these because in most cases it's not the way to solve the problem (as mentioned above).

[heaths]

What exactly is the current ask then? If people build with the net7.0 SDK, it sounds as if there's nothing more that needs to be done. Application developers using the SDK should get warnings (we could test that) if reflection is used. Do they get actionable output to help them make sure the right types aren't trimmed? For example, if an application used the CryptographyClient which does use reflection in some code paths but didn't use code paths, I'd expect they wouldn't get warnings and all that would be trimmed out. Is that correct?

[snakefoot]

Sounds like the beginning of a good treasure hunt. With hidden doors you can only open if you are Harry Potter. Not sure I fully understand the argument, that if not everyone can figure out how to use the hammer, then no one can hold it. What about making an assembly attribute that disables full trimming for the entire application. And only allows the application to perform partial trimming?

[vitek-karas]

The static analysis can't figure out which types are needed - if it could, it would be able to keep them and avoid warning the user. The warnings are produced when the static analysis doesn't know what the code is doing - so all it can tell is "This code here is doing something which I don't understand". If this warning occurs in one of the Azure SDK assemblies there's little the end user can do about it:

• They don't understand the Azure SDK code, so it probably doesn't make sense to them
• They can't change the Azure SDK code to fix it

The ask here is to enable trim analysis (mentioned in the docs) and see what kind of warnings are produced from within the Azure SDK Code and try to fix them. Once that is done, mark the assemblies as trimmable (not that important in .NET 7 anymore).

Note that "try to fix them" can generally mean two things:

• Refactor the code or use some of the available trim annotation mechanisms so that the static analysis understands what's going on.
• Or if it's inherently not fixable, figure out hopefully a small subset of public APIs affected by it and annotate them with RequiresUnreferencedCode which allows you to provide a nicer warning message to tell the user what to change about their code to make it trimmable (if possible).

[heaths]

Early on, we could provide a configuration file with reflection-referenced types. Do I correct assume this is the file mentioned above that the .NET doesn't want to publicize?

On a related note, @jsquire @pallavit we should revisit multi-targeting, which would also help solve this problem. I'll start a thread offline.

[jkotas]

It is fine to use reflection with trimming as long as the code that uses reflection is annotated or follows idiomatic patterns that are recognized by the linker. For example, the following code is using idiomatic patterns - the linker will keep the named types around as if they were referenced statically:

azure-sdk-for-net/sdk/core/Azure.Core/src/Shared/DiagnosticScope.cs

Lines 369 to 373 in 6b7a014

	private static readonly Type? ActivitySourceType = Type.GetType("System.Diagnostics.ActivitySource, System.Diagnostics.DiagnosticSource");
	private static readonly Type? ActivityKindType = Type.GetType("System.Diagnostics.ActivityKind, System.Diagnostics.DiagnosticSource");
	private static readonly Type? ActivityTagsCollectionType = Type.GetType("System.Diagnostics.ActivityTagsCollection, System.Diagnostics.DiagnosticSource");
	private static readonly Type? ActivityLinkType = Type.GetType("System.Diagnostics.ActivityLink, System.Diagnostics.DiagnosticSource");
	private static readonly Type? ActivityContextType = Type.GetType("System.Diagnostics.ActivityContext, System.Diagnostics.DiagnosticSource");

What exactly is the current ask then?

The ask is to set <IsTrimmable>true</IsTrimmable> property on all Azure SDK projects and fix all warnings that this will generate by either:

• Making the code compatible with trimming by updating it to follow idiomatic reflection patterns or annotating for trimming (e.g. using DynamicallyAccessedMembersAttribute)
• Annotating the code as in-compatible with trimming using RequiresUnreferencedCodeAttribute

[vitek-karas]

What about making an assembly attribute that disables full trimming for the entire application. And only allows the application to perform partial trimming!

That's definitely possible to add, but I don't see much value in it. The "partial" trimming is just a heuristic which makes a simple assumption "Most reflection code acts on user code, so if we don't trim user code, it won't break that much". The example of the crypto algorithms above is exactly the counter example to this assumption - since it's using reflection to access non-user code.

Also it's not a very good strategy going forward - maybe it would make most apps using this assembly work, but it's fragile and any change to the code in the assembly or improvements to the tooling can break the app again (one of the reasons we're trying to move away from the "partial" trimming).
Not counting the fact that it won't produce the best results - as mentioned above, the ask here from the customer is to be able to trim some of the code from the Azure SDK assemblies as they are large - partial trimming would not do that.

[vitek-karas]

Early on, we could provide a configuration file with reflection-referenced types. Do I correct assume this is the file mentioned above that the .NET doesn't want to publicize?

Yes - but if we're going to do any work on this, it would be better to do this "in the code" as it's more maintainable, the configuration file should really be the last resort solution as it's really easy to forget about and break in future changes.

[snakefoot]

Think .NET7 should perform full-trim by default, but when detecting assemblies with warnings, then it should only perform partial triming of the assembly, unless the assembly is explictly marked <IsTrimmable>true</IsTrimmable>.

[vitek-karas]

@snakefoot ignoring the fact that it would be relatively tricky to implement, it's incorrect and would not help much really. As mentioned above any warning is potentially a "global" problem, which can't be fixed by not trimming the assembly where it came from. Again with the example above, not trimming the SDK assembly won't help if the trimmer decides to remove framework type because it can't see it being used by anything.

[heaths]

Some libraries, like the Key Vault client libraries, will need to multi-target to remove reflection. See my comment for reasoning. I recommend just multi-targeting net6.0 (or whatever the then-current LTS is) since all the APIs Key Vault needs are defined therein. Too many permutations otherwise to eliminate reflection-based light-up features entirely.

[MichalStrehovsky]

Too many permutations otherwise to eliminate reflection-based light-up features entirely.

@heaths Just to make sure: reflection in general is not a problem. What is a problem is reflection that cannot be statically analyzed.

Reflection lightup is usually in the form of:

typeof(Foo).GetMethod("Bar") or Type.GetType("Some known string").GetMethod("Method") - those are fine - they can be statically analyzed and all is good.

What is not fine is if the types/members reflected on cannot be statically seen. This typically happens when the above straightforward dataflow is interrupted and e.g. the intermediate Type instance is stored into a local field, or the strings get returned from a method call or something like that. There are two possible fixes for that:

• Rewrite to use the straightforward reflection where everything being reflected on can be figured out by just looking at the method body, or
• Use dataflow annotations to help the analysis reason about cross-method reflection (I would not suggest this route for lightups)

If you specify the <IsTrimmable>true</IsTrimmable> property in the project, it will guide you to the right annotations. Note that the IsTrimmable analyzer works best if you target net6.0 or later - the framework libraries are not properly annotated on netstandard (but this only affects the analyzer - you don't have to ship as net6.0 - just have a way to build the code as net6.0 with the analyzer - I know it's an extra annoyance; we don't have anything better for netstandard2.0 targeting libraries trimming compatibility).

[heaths]

the intermediate Type instance is stored into a field

@MichalStrehovsky we do exactly that for performance gains, or has that not been a problem of late? We do target netstandard2.0, so net461 and netcoreapp2.0 are possible customer targets, so I'm concerned about older runtimes as well.

Here's an example:

azure-sdk-for-net/sdk/keyvault/Azure.Security.KeyVault.Certificates/src/PemReader.cs

Lines 17 to 34 in 124a959

	private static bool s_ecInitializedImportPkcs8PrivateKeyMethod;
	private static MethodInfo s_ecImportPkcs8PrivateKeyMethod;
	private static MethodInfo s_ecCopyWithPrivateKeyMethod;
	static partial void CreateECDsaCertificate(byte[] cer, byte[] key, X509KeyStorageFlags keyStorageFlags, ref X509Certificate2 certificate)
	{
	if (!s_ecInitializedImportPkcs8PrivateKeyMethod)
	{
	// ImportPkcs8PrivateKey was added in .NET Core 3.0 and is only present on Core. We will fall back to a lightweight decoder if this method is missing from the current runtime.
	s_ecImportPkcs8PrivateKeyMethod = typeof(ECDsa).GetMethod("ImportPkcs8PrivateKey", BindingFlags.Instance | BindingFlags.Public, null, new[] { typeof(ReadOnlySpan<byte>), typeof(int).MakeByRefType() }, null);
	s_ecInitializedImportPkcs8PrivateKeyMethod = true;
	}
	if (s_ecCopyWithPrivateKeyMethod is null)
	{
	s_ecCopyWithPrivateKeyMethod = typeof(ECDsaCertificateExtensions).GetMethod("CopyWithPrivateKey", BindingFlags.Static | BindingFlags.Public, null, new[] { typeof(X509Certificate2), typeof(ECDsa) }, null)
	?? throw new PlatformNotSupportedException("The current platform does not support reading an ECDsa private key from a PEM file");
	}

From your description, I'd take it that's a problem? Is there any sort of annotations (attributes, #pragmas, etc.) that can help with static analysis in this case?

[MichalStrehovsky]

From your description, I'd take it that's a problem? Is there any sort of annotations (attributes, #pragmas, etc.) that can help with static analysis in this case?

That should not be a problem. The typeof(ECDsa).GetMethod("ImportPkcs8PrivateKey", BindingFlags.Instance | BindingFlags.Public, null, new[] { typeof(ReadOnlySpan<byte>), typeof(int).MakeByRefType() }, null) is a static analysis slam dunk (it is easy to statically see ECDsa.ImportPkcs8PrivateKey is needed) and trimming will keep it.

The problematic case would be something like:

class Foo
{
    static Type s_type = typeof(ECDsa);
    static MethodInfo GetImportPkcs8PrivateKeyMethod() => s_type.GetMethod("ImportPkcs8PrivateKey");
}

i.e. the type being reflected on and the name of the member crosses a method boundary/field boundary. Sometimes people write code that way. It is hard to programmatically analyze (it might even be more difficult for human) and trimming doesn't attempt to do that. It can be trivially rewritten to the pattern you already have - that pattern poses no problem for trimming.

[heaths]

@MichalStrehovsky @eerhardt is this still needed after all the work @m-redding has been doing with AOT? I'm not sure how, or even if, this fits in anymore.

[eerhardt]

We've been moving away from "partial" trimming, where the IsTrimmable attribute has an effect. But there are scenarios that still use it - for example Blazor WASM and MAUI.

So I'd say, if possible, this work should still be done. I'd expect to use Azure SDK libraries in MAUI apps. However a higher priority would be to address the trimming and AOT warnings from the libraries (i.e. what @m-redding has been doing).

[pallavit]

@jsquire I believe some of the work that @m-redding is doing is similar to this? Please correct me if I am mistaken.

[m-redding]

@pallavit it's related but it's different so I think this issue should remain open. I don't think any of our libraries can be marked as IsTrimmable right now since we had to baseline some unresolvable warnings from Azure.Core.

[github-actions]

Hi @MichalStrehovsky, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

[github-actions]

Hi @MichalStrehovsky, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.