Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra ILC warnings when PublishAOT=true on method already marked RequiresUnreferencedCode #75898

Closed
eerhardt opened this issue Sep 20, 2022 · 10 comments · Fixed by #80956, #81532 or #82392
Closed
Assignees
Milestone

Comments

@eerhardt
Copy link
Member

Description

When Publishing AOT, I'm seeing extra warnings being emitted that clutter the warnings and confuse developers.

From what I can tell, the warnings are coming from:

[RequiresDynamicCode(RequiresDynamicCodeMessage)]
[RequiresUnreferencedCode(TrimmingRequiresUnreferencedCodeMessage)]
public static void RegisterProviderOptions<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TOptions, TProvider>(IServiceCollection services) where TOptions : class
{
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<TOptions>, LoggerProviderConfigureOptions<TOptions, TProvider>>());

ServiceDescriptor.Singleton has a [DAM] attribute for the constructor of the 2nd generic arg:

public static ServiceDescriptor Singleton<TService, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>()

and LoggerProviderConfigureOptions has RDC and RUC on its ctor:

[RequiresDynamicCode(LoggerProviderOptions.RequiresDynamicCodeMessage)]
[RequiresUnreferencedCode(LoggerProviderOptions.TrimmingRequiresUnreferencedCodeMessage)]
public LoggerProviderConfigureOptions(ILoggerProviderConfiguration<TProvider> providerConfiguration)

However, these warnings shouldn't be emitted because RegisterProviderOptions has RDC and RUC attributes. So I should only be getting the warnings from my call to RegisterProviderOptions.

cc @MichalStrehovsky @vitek-karas

Reproduction Steps

Using 7.0-rc1, dotnet publish -c Release -r win-x64 the following app:

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <PublishAOT>true</PublishAOT>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="7.0.0-rc.1.22426.10" />
  </ItemGroup>
</Project>
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging.Configuration;
using Microsoft.Extensions.Logging.Console;

LoggerProviderOptions.RegisterProviderOptions<ConsoleLoggerOptions, ConsoleLoggerProvider>(new ServiceCollection());

Expected behavior

I should only get warnings for my program's call to RegisterProviderOptions.

Actual behavior

Along with the expected warnings from my code, I'm getting these extra warnings in the publish output:

ILC : Trim analysis warning IL2026: Microsoft.Extensions.DependencyInjection.ServiceDescriptor.Singleton<IConfigureOptions`1<ConsoleLoggerOptions>,LoggerProviderConfigureOptions`2<ConsoleLoggerOptions,ConsoleLoggerProvider>>(): Using member 'Microsoft.Extensions.Logging.Configuration.LoggerProviderConfigureOptions`2<ConsoleLoggerOptions,ConsoleLoggerProvider>.LoggerProviderConfigureOptions`2<ConsoleLoggerOptions,ConsoleLoggerProvider>(ILoggerProviderConfiguration`1<ConsoleLoggerProvider>)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. TOptions's dependent types may have their members trimmed. Ensure all required members are preserved. [C:\Users\eerhardt\source\repos\WorkerService12\WorkerService12\WorkerService12.csproj]
ILC : Trim analysis warning IL2026: Microsoft.Extensions.DependencyInjection.ServiceDescriptor.Describe<IConfigureOptions`1<ConsoleLoggerOptions>,LoggerProviderConfigureOptions`2<ConsoleLoggerOptions,ConsoleLoggerProvider>>(ServiceLifetime): Using member 'Microsoft.Extensions.Logging.Configuration.LoggerProviderConfigureOptions`2<ConsoleLoggerOptions,ConsoleLoggerProvider>.LoggerProviderConfigureOptions`2<ConsoleLoggerOptions,ConsoleLoggerProvider>(ILoggerProviderConfiguration`1<ConsoleLoggerProvider>)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. TOptions's dependent types may have their members trimmed. Ensure all required members are preserved. [C:\Users\eerhardt\source\repos\WorkerService12\WorkerService12\WorkerService12.csproj]

Regression?

I'm not sure. Probably not.

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 20, 2022
@MichalStrehovsky MichalStrehovsky added this to the 8.0.0 milestone Sep 22, 2022
@MichalStrehovsky MichalStrehovsky removed the untriaged New issue has not been triaged by the area owner label Sep 22, 2022
@agocke agocke added this to AppModel Nov 17, 2022
@agocke
Copy link
Member

agocke commented Nov 17, 2022

@tlakollo Let's work on this as soon as packaging is done and ready for linker in runtime.

@eerhardt
Copy link
Member Author

@tlakollo - any update on this? We'd like to get to 0 warnings in a very basic ASP.NET app soon.

@vitek-karas
Copy link
Member

I'll look into this.

@vitek-karas vitek-karas assigned vitek-karas and unassigned tlakollo Jan 10, 2023
@vitek-karas
Copy link
Member

This is a known problem (don't think we have an issue yet) which we've been discussing for a while. In general the NativeAOT compiler's handling of warnings from DAM annotation of generic arguments is problematic. It performs the DAM validation in places where it should not (which is most likely the reason for the extra warnings you see here) and it also reports these warnings as originating from wrong places (doesn't seem to be hit by this code).

I've discussed this with @MichalStrehovsky and we need to modify how the compiler handles DAM on generics. The broad idea is:

  • Avoid analyzing generic instantiations which are caused by instantiating method bodies or types - basically if the original IL is something like MyGeneric<!0> then we should only analyzer that, and not any specific MyGeneric<Foo> which is produced by the compiler during instantiation of generics.
  • Instead of relying on dependency nodes, try to analyze generic instantiations from data flow as much as possible (since there we can provide the correct message origin)

Unfortunately what this means is that it's not a simple fix and thus will take some time. I guess we (read @vitek-karas 😉) need to prioritize this work.

@DamianEdwards
Copy link
Member

I'm hitting this warning now with the code produced by the new project template so it seems this is going to be on the critical path to getting past our stage 1 goals (create a new project from this template with native AOT enabled and get now warnings).

@eerhardt
Copy link
Member Author

and thus will take some time

Any idea how long? Is it something that could make it into 8.0-preview2? That's when we are targeting getting a basic ASP.NET app published for AOT without any warnings.

@vitek-karas
Copy link
Member

I'm hoping to have it done it about a week. I'll know better couple of days in (still lot to learn in the compiler to do this right).

@MichalStrehovsky
Copy link
Member

(still lot to learn in the compiler to do this right)

Just a note that we don't have to fix all of the generic issues in one go. We have known holes around generics (e.g. #80037), so "just" replacing the existing scheme with one that generates warnings from dataflow (that are suppressible) would be a good first step.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 25, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 25, 2023
@vitek-karas
Copy link
Member

The fix has been reverted due to #81358.

@vitek-karas vitek-karas reopened this Jan 30, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 2, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2023
@eerhardt
Copy link
Member Author

eerhardt commented Feb 8, 2023

Reopening as this fix had to be reverted. See #81783

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 20, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.