Unactionable Trim warnings when referencing both Npgsql and Microsoft.Extensions.Http.Resilience

[eerhardt]

Description

When referencing 2 totally different packages independently, I'm able to publish an app without any warnings. But when I combine them and use them together, I'm getting trim warnings that I can't fix as a end-user developer.

Reproduction Steps

dotnet publish the following app:

csproj:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <InvariantGlobalization>true</InvariantGlobalization>
    <PublishAot>true</PublishAot>
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Npgsql.DependencyInjection" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.2.0-preview.24066.3" />
  </ItemGroup>
  
</Project>

(Note that <PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.2.0-preview.24066.3" /> needs a nuget.config entry for <add key="dotnet8" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" />)

Program.cs:

var builder = WebApplication.CreateSlimBuilder(args);

builder.Services.AddNpgsqlDataSource(builder.Configuration.GetConnectionString("npgsql") ?? throw new Exception("configure npgsql"));

builder.Services.ConfigureHttpClientDefaults(http =>
{
    http.AddStandardResilienceHandler();
});

var app = builder.Build();
app.MapGet("/", () => "hello, world");
app.Run();

Expected behavior

No publish warnings.

Actual behavior

ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetConverter()' which requires unreferenced code. Generic TypeConverters may require the generic types to be annotated. For example, NullableConverter requires the underlying type to be DynamicallyAccessedMembers All. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetDefaultEvent()' which requires unreferenced code. The built-in EventDescriptor implementation uses Reflection which requires unreferenced code. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetDefaultProperty()' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetEditor(Type)' which requires unreferenced code. Editors registered in TypeDescriptor.AddEditorTable may be trimmed. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetEvents(Attribute[])' which requires unreferenced code. The public parameterless constructor or the 'Default' static field may be trimmed from the Attribute's Type. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetProperties()' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetProperties(Attribute[])' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. The public parameterless constructor or the 'Default' static field may be trimmed from the Attribute's Type. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2112: Npgsql.NpgsqlConnectionStringBuilder.GetProperties(Hashtable): 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'Npgsql.NpgsqlConnectionStringBuilder.GetProperties(Hashtable)' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. [C:\DotNetTest\ApiTest\ApiTest.csproj]

Note that if you comment out either line 2 or 3 in the Program.cs, there are no warnings. But when you have both, then the warnings show up.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

The reason this shows up is because AddStandardResilienceHandler() brings in a bunch of System.ComponentModel interfaces like
ICustomTypeDescriptor.

I see the same warnings are being suppressed on the base DbConnectionStringBuilder:

runtime/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs

Lines 15 to 20 in 613a13f

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2113:ReflectionToRequiresUnreferencedCode",
	Justification = "The use of GetType preserves ICustomTypeDescriptor members with RequiresUnreferencedCode, but the GetType callsites either "
	+ "occur in RequiresUnreferencedCode scopes, or have individually justified suppressions.")]
	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2112:ReflectionToRequiresUnreferencedCode",
	Justification = "The use of GetType preserves implementation of ICustomTypeDescriptor members with RequiresUnreferencedCode, but the GetType callsites either "
	+ "occur in RequiresUnreferencedCode scopes, or have individually justified suppressions.")]

See also:

• Fix warning suppressions on types for type hierarchy linker#2148
• Fix warning origin for DAM on types linker#2162

Should NpgsqlConnectionStringBuilder be suppressing these warnings as well?

cc @vitek-karas @sbomer @roji

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When referencing 2 totally different packages independently, I'm able to publish an app without any warnings. But when I combine them and use them together, I'm getting trim warnings that I can't fix as a end-user developer.

Reproduction Steps

dotnet publish the following app:

csproj:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <InvariantGlobalization>true</InvariantGlobalization>
    <PublishAot>true</PublishAot>
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Npgsql.DependencyInjection" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.2.0-preview.24066.3" />
  </ItemGroup>
  
</Project>

(Note that <PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.2.0-preview.24066.3" /> needs a nuget.config entry for <add key="dotnet8" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" />)

Program.cs:

var builder = WebApplication.CreateSlimBuilder(args);

builder.Services.AddNpgsqlDataSource(builder.Configuration.GetConnectionString("npgsql") ?? throw new Exception("configure npgsql"));

builder.Services.ConfigureHttpClientDefaults(http =>
{
    http.AddStandardResilienceHandler();
});

var app = builder.Build();
app.MapGet("/", () => "hello, world");
app.Run();

Expected behavior

No publish warnings.

Actual behavior

ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetConverter()' which requires unreferenced code. Generic TypeConverters may require the generic types to be annotated. For example, NullableConverter requires the underlying type to be DynamicallyAccessedMembers All. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetDefaultEvent()' which requires unreferenced code. The built-in EventDescriptor implementation uses Reflection which requires unreferenced code. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetDefaultProperty()' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetEditor(Type)' which requires unreferenced code. Editors registered in TypeDescriptor.AddEditorTable may be trimmed. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetEvents(Attribute[])' which requires unreferenced code. The public parameterless constructor or the 'Default' static field may be trimmed from the Attribute's Type. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetProperties()' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2113: Npgsql.NpgsqlConnectionStringBuilder: 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'System.ComponentModel.ICustomTypeDescriptor.GetProperties(Attribute[])' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. The public parameterless constructor or the 'Default' static field may be trimmed from the Attribute's Type. [C:\DotNetTest\ApiTest\ApiTest.csproj]
ILC : Trim analysis warning IL2112: Npgsql.NpgsqlConnectionStringBuilder.GetProperties(Hashtable): 'DynamicallyAccessedMembersAttribute' on 'Npgsql.NpgsqlConnectionStringBuilder' or one of its base types references 'Npgsql.NpgsqlConnectionStringBuilder.GetProperties(Hashtable)' which requires unreferenced code. PropertyDescriptor's PropertyType cannot be statically discovered. [C:\DotNetTest\ApiTest\ApiTest.csproj]

Note that if you comment out either line 2 or 3 in the Program.cs, there are no warnings. But when you have both, then the warnings show up.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

The reason this shows up is because AddStandardResilienceHandler() brings in a bunch of System.ComponentModel interfaces like
ICustomTypeDescriptor.

I see the same warnings are being suppressed on the base DbConnectionStringBuilder:

runtime/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs

Lines 15 to 20 in 613a13f

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2113:ReflectionToRequiresUnreferencedCode",
	Justification = "The use of GetType preserves ICustomTypeDescriptor members with RequiresUnreferencedCode, but the GetType callsites either "
	+ "occur in RequiresUnreferencedCode scopes, or have individually justified suppressions.")]
	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2112:ReflectionToRequiresUnreferencedCode",
	Justification = "The use of GetType preserves implementation of ICustomTypeDescriptor members with RequiresUnreferencedCode, but the GetType callsites either "
	+ "occur in RequiresUnreferencedCode scopes, or have individually justified suppressions.")]

See also:

• Fix warning suppressions on types for type hierarchy linker#2148
• Fix warning origin for DAM on types linker#2162

Should NpgsqlConnectionStringBuilder be suppressing these warnings as well?

cc @vitek-karas @sbomer @roji

Author:	eerhardt
Assignees:	-
Labels:	untriaged, area-NativeAOT-coreclr, needs-area-label
Milestone:	-

[MichalStrehovsky]

Looks like the IL2113 warnings only show up for PublishAot. The IL2112 shows up for both PublishAot and PublishTrimmed.

This is also potentially related to #88512 that was filed due to the same .All being applied on DbConnectionStringBuilder. We probably don't need .All. We probably just need something like .AllProperties and .AllEvents. The problems that .All is causing and then we need to suppress and the suppressions are hard, etc. is very likely caused by not having a mechanism for more targeted preservation that would avoid the problematic cases.

[vitek-karas]

@LakshanF as this is related to TypeDescriptor and potential improvements in that area.

This will teach us to think more than twice before putting any annotation on a virtual/interface method :-)

[eerhardt]

Should NpgsqlConnectionStringBuilder be suppressing these warnings as well?

Since these warnings are being suppressed on the base DbConnectionStringBuilder, is it safe to assume that we can suppress them on NpgsqlConnectionStringBuilder as well? NpgsqlConnectionStringBuilder isn't doing anything special here - it is just trying to implement the ICustomTypeDescriptor overrides (which have RequiresUnreferencedCode on them - so any real callers of those methods will get warnings).

We probably don't need .All.

The root of need for .All (as far as I can tell) comes from a couple places:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs

Lines 44 to 45 in 5598dac

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2062:UnrecognizedReflectionPattern",
	Justification = "_type is annotated as preserve All members, so any Types returned from GetInterfaces should be preserved as well once https://github.com/mono/linker/issues/1731 is fixed.")]

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs

Lines 98 to 107 in 5598dac

	Type[] interfaces = _type.GetInterfaces();
	for (int idx = 0; idx < interfaces.Length; idx++)
	{
	// Only do this for public interfaces.
	Type iface = interfaces[idx];
	if ((iface.Attributes & (TypeAttributes.Public | TypeAttributes.NestedPublic)) != 0)
	{
	// No need to pass an instance into GetTypeDescriptor here because, if someone provided a custom
	// provider based on object, it already would have hit.
	attributes.AddRange(TypeDescriptor.GetAttributes(iface).Attributes);

In order to get all the attributes for a Type, we need to walk all the interfaces the Type implements and get events, properties, etc. on it.

I tried lessening the .All annotations (see this branch), but I ran into these places that recurse up the hierarchy:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectEventDescriptor.cs

Lines 323 to 327 in 5598dac

	Type? start = _componentClass.BaseType;
	while (start != null && start != typeof(object))
	{
	BindingFlags bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic;
	EventInfo test = start.GetEvent(_realEvent.Name, bindingFlags)!;

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs

Lines 425 to 430 in 5598dac

	Type? baseType = _type;
	Type objType = typeof(object);
	do
	{
	eventArray = ReflectGetEvents(baseType);

AFAIK, the trim analysis has special handling of .All, won't trim things from the base types, and won't warn about hierarchy walking like this. But now that we aren't using .All, these places could be broken.

Also note that I ended up needing to preserve:

• PublicProperties
• Interfaces
• PublicEvents
• NonPublicEvents
• PublicMethods (needed because of this code)

Which would probably cause us to have this same problem anyway - it would still need to preserve the ICustomTypeDescriptor interface and all its methods.

[MichalStrehovsky]

AFAIK, the trim analysis has special handling of .All, won't trim things from the base types, and won't warn about hierarchy walking like this. But now that we aren't using .All, these places could be broken.

#88512 is about introducing annotations that would allow walking base hierarchies and looking for members without having to preserve things to the extent that .All does. (we could preserve e.g. all methods, including those on base types, without having to go all in with .All)

The problem that .All is causing here is that it will walk into the list of implemented interfaces and apply .All to all of the methods on those interfaces as well (that's what the warnings are about). If we could say AllEventsInHierarchy | AllPropertiesInHierarchy, it would avoid the problem with marking the methods on the interface as reflection-exposed because it would only go to bases.

The fact we need to mark PublicMethods is annoying and the linked code is atrocious, but maybe it would still be okay because it's just the public methods and there don't appear to be any public methods on DbConnectionStringBuilder that would be RUC (explicitly implemented interface methods are not public).

Being more targeted causes fewer ripple effects.

Not saying we don't have a bug with the suppression not working right, but we wouldn't run into it if we didn't use .All.

[eerhardt]

FYI - I opened npgsql/npgsql#5578 to add the same suppressions to NpgsqlConnectionStringBuilder as we have on DbConnectionStringBuilder. But it would be good to see if there is something we can do in dotnet/runtime to fix this general problem.