NullabilityInfoContext + Trimming

[eerhardt]

In 6.0 we have introduced new NullabilityInfoContext APIs which gives developers access to nullable reference type annotation information. We also have been working on trimming applications to make them smaller. Part of that trimming work introduced removing the nullable attributes from trimmed apps:

runtime/src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml

Lines 133 to 147 in 0851fee

	<type fullname="System.Diagnostics.CodeAnalysis.AllowNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.DisallowNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.MaybeNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.NotNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>

runtime/src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml

Lines 180 to 182 in 0851fee

	<type fullname="System.Runtime.CompilerServices.NullableContextAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>

Today this removal only happens on apps using the Mono runtime. But #48217 tracks moving these entries to be shared with coreclr as well.

This presents a problem because these APIs will no longer work correctly if these attributes are removed. See dotnet/linker#1991 and #54985 (comment).

To address this we should do the following:

1. Introduce a new feature switch System.Reflection.NullabilityInfoSupport
2. By default, when this switch isn't specified, its value is true. By default the NullabilityInfo APIs work.
3. When the switch is set to false, we do the following:
   a. Change the above ILLink.LinkAttributes.xml files to only remove the nullable attributes when the feature switch is set to false. This is similar to the EventSourceSupport or DebuggerSupport feature switches which will remove their corresponding attributes when these are disabled.
   b. Raise a "LibraryBuild" linker warning. So if someone in the app is using these APIs, and the switch is false, the app developers gets a warning. This is similar to StartupHookSupport which has a "LibraryBuild" warning when StartupHooks are enabled in a trimmed app.
   c. throw an exception from the NullabilityInfoContext APIs. When this feature switch is set, the attributes are going to be removed when the app is trimmed. So we should always throw an exception (whether the app is trimmed or not) to tell the developer that these APIs don't work with this feature switch setting.

App models' SDKs (like Blazor WASM, Maui, etc.) can decide the default setting of System.Reflection.NullabilityInfoSupport for their app model. For example, Blazor WASM will default this switch to false. That way they continue to get the size savings of removing these attributes. However, app developers can always override the defaults in their app. So now a Blazor WASM app that needs to call the new NullabilityInfoContext APIs can do so by setting NullabilityInfoSupport=true in their .csproj. The attributes will no longer be trimmed, and the APIs will work correctly.

Since the size savings for desktop apps isn't as significant as mobile apps, I don't believe console/WinForms/web server/etc apps would default this setting to false. If an app dev wanted to squeeze every bit of size out of their app, they can set it to false in these app models to get the size savings.

cc @vitek-karas @marek-safar @MichalStrehovsky @buyaa-n @jeffhandley @terrajobst

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In 6.0 we have introduced new NullabilityInfoContext APIs which gives developers access to nullable reference type annotation information. We also have been working on trimming applications to make them smaller. Part of that trimming work introduced removing the nullable attributes from trimmed apps:

runtime/src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml

Lines 133 to 147 in 0851fee

	<type fullname="System.Diagnostics.CodeAnalysis.AllowNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.DisallowNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.MaybeNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>
	<type fullname="System.Diagnostics.CodeAnalysis.NotNullAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>

runtime/src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml

Lines 180 to 182 in 0851fee

	<type fullname="System.Runtime.CompilerServices.NullableContextAttribute">
	<attribute internal="RemoveAttributeInstances" />
	</type>

Today this removal only happens on apps using the Mono runtime. But #48217 tracks moving these entries to be shared with coreclr as well.

This presents a problem because these APIs will no longer work correctly if these attributes are removed. See dotnet/linker#1991 and #54985 (comment).

To address this we should do the following:

1. Introduce a new feature switch System.Reflection.NullabilityInfoSupport
2. By default, when this switch isn't specified, its value is true. By default the NullabilityInfo APIs work.
3. When the switch is set to false, we do the following:
   a. Change the above ILLink.LinkAttributes.xml files to only remove the nullable attributes when the feature switch is set to false. This is similar to the EventSourceSupport or DebuggerSupport feature switches which will remove their corresponding attributes when these are disabled.
   b. Raise a "LibraryBuild" linker warning. So if someone in the app is using these APIs, and the switch is false, the app developers gets a warning. This is similar to StartupHookSupport which has a "LibraryBuild" warning when StartupHooks are enabled in a trimmed app.
   c. throw an exception from the NullabilityInfoContext APIs. When this feature switch is set, the attributes are going to be removed when the app is trimmed. So we should always throw an exception (whether the app is trimmed or not) to tell the developer that these APIs don't work with this feature switch setting.

App models' SDKs (like Blazor WASM, Maui, etc.) can decide the default setting of System.Reflection.NullabilityInfoSupport for their app model. For example, Blazor WASM will default this switch to false. That way they continue to get the size savings of removing these attributes. However, app developers can always override the defaults in their app. So now a Blazor WASM app that needs to call the new NullabilityInfoContext APIs can do so by setting NullabilityInfoSupport=true in their .csproj. The attributes will no longer be trimmed, and the APIs will work correctly.

Since the size savings for desktop apps isn't as significant as mobile apps, I don't believe console/WinForms/web server/etc apps would default this setting to false. If an app dev wanted to squeeze every bit of size out of their app, they can set it to false in these app models to get the size savings.

cc @vitek-karas @marek-safar @MichalStrehovsky @buyaa-n @jeffhandley @terrajobst

Author:	eerhardt
Assignees:	-
Labels:	area-System.Reflection, linkable-framework
Milestone:	6.0.0

[MichalStrehovsky]

Not a big fan of introducing extra switches that people have to learn about. Illink is doing an invalid optimization by stripping the attributes in the first place. The way we want trimming to work is that the behavior of the app before/after trimming is the same (or there's warnings). This optimization breaks that. Sure, we can make it so that the official Nullability querying API has always the same behavior through the newly proposed feature switch that makes the API throw, but it doesn't fix existing .NET 5 apps and NuGet packages that query nullability by looking at the physical custom attributes manually. It also doesn't fix any other apps that might be looking for other custom attributes we decided to strip on behalf of the user. It's also extra mental overhead and code to maintain.

Illink has a pretty good idea of what things are targets of reflection in general. E.g. it will strip parameter name metadata off methods that are not seen as a target of reflection. Attribute stripping should fall into that category - illink should only be allowed to strip (publicly documented) attributes if the thing the attribute is attached to is not a visible target of reflection. For non-public attributes, the stripping can stay more aggressive. This is tracked in https://github.com/mono/linker/issues/2078.

It will be a size regression, but we will be trading size for correctness and we'll never have to think about introducing new feature switches when an attribute-cracking API is added in the future.