search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

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

Open eerhardt opened 8 months ago

eerhardt commented 8 months ago

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:

https://github.com/dotnet/runtime/blob/613a13fb4af81f8ed0b0d27dff58fa0c6129e5ba/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs#L15-L20

See also:

Should NpgsqlConnectionStringBuilder be suppressing these warnings as well?

cc @vitek-karas @sbomer @roji

ghost commented 8 months ago

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: ```xml net8.0 enable enable true true false ``` (Note that `` needs a nuget.config entry for ``) Program.cs: ```csharp 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: https://github.com/dotnet/runtime/blob/613a13fb4af81f8ed0b0d27dff58fa0c6129e5ba/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs#L15-L20 See also: * https://github.com/dotnet/linker/pull/2148 * https://github.com/dotnet/linker/pull/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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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:

https://github.com/dotnet/runtime/blob/5598dac557c35d264c527e22239f1b33e0489376/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs#L44-L45

https://github.com/dotnet/runtime/blob/5598dac557c35d264c527e22239f1b33e0489376/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs#L98-L107

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:

https://github.com/dotnet/runtime/blob/5598dac557c35d264c527e22239f1b33e0489376/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectEventDescriptor.cs#L323-L327

https://github.com/dotnet/runtime/blob/5598dac557c35d264c527e22239f1b33e0489376/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs#L425-L430

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:

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 commented 8 months ago

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 commented 7 months ago

FYI - I opened https://github.com/npgsql/npgsql/pull/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.