dotnet / extensions

This repository contains a suite of libraries that provide facilities commonly needed when creating production-ready applications.
MIT License
2.55k stars 730 forks source link

`Microsoft.Extensions.Http.Resilience` breaks `LoggerMessage` code generation. #5173

Open hugoqribeiro opened 1 month ago

hugoqribeiro commented 1 month ago

Description

We have a .NET 8 project where we add a reference to a package that includes a dependency on Microsoft.Extensions.Http.Resilience. That project also uses [LoggerMessage] to take advantage of the source generator.

That project also includes a reference to a package (.NET Standard 2.0) that is using il-repack, which results in all the types under Microsoft.Extensions.Logging (including Microsoft.Extensions.Logging.LoggerMessageAttribute) being included in the assembly as internal types.

In this scenario, the source generator does not work.

[LoggerMessage(Level = LogLevel.Information, Message = "Something")]
private static partial void LogSomething(ILogger logger);

The error is:

CS8795  Partial method 'Program.LogSomething(ILogger)' must have an implementation part because it has accessibility modifiers.

Investigating this, I believe that the problem is related with how Microsoft.Gen.Logging.LoggingGenerator.LoggingGenerator is looking up for Microsoft.Extensions.Logging.LoggerMessageAttribute.

I think that Microsoft.Extensions.Logging.Generators.LoggerMessageGenerator.Parser did that lookup considering the visibility of the types:

INamedTypeSymbol bestTypeByMetadataName = _compilation.GetBestTypeByMetadataName("Microsoft.Extensions.Logging.LoggerMessageAttribute");

I'm not 100% sure, but it looks like Microsoft.Gen.Logging.LoggingGenerator.LoggingGenerator isn't considering visibility. Which results in the code not being generated.

Reproduction Steps

Here's the csproj definition:

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

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Cegid.Hydrogen.Hosting" Version="8.0.5" />
        <PackageReference Include="Cegid.Lithium.Settings.Client.Rest.SingleAssembly" Version="6.0.0" />
    </ItemGroup>

</Project>

Cegid.Hydrogen.Hosting is the package that depends on Microsoft.Extensions.Http.Resilience. Cegid.Lithium.Settings.Client.Rest.SingleAssembly is the package that uses il-repack and has LoggerMessageAttribute as an internal type.

capture

Expected behavior

The source generator should work as expected, because there is only one public type accessible named Microsoft.Extensions.Logging.LoggerMessageAttribute.

Actual behavior

The source generator does not kick-in.

Regression?

No.

Known Workarounds

Adding this in the csproj, after the package reference, makes the old source generator work:

<PropertyGroup>
   <DisableMicrosoftExtensionsLoggingSourceGenerator>false</DisableMicrosoftExtensionsLoggingSourceGenerator>
</PropertyGroup>

Obviously, this does not work if we remove the reference to the single assembly, as BOTH source generators kick-in and we end up with 2 implementations of the logging method.

Configuration

We're using .NET 8.0.4 and Microsoft.Extensions.Http.Resilience 8.4.0. Nothing else seams relevant for this issue.

Other information

No response

dariusclay commented 1 month ago

Can you provide a minimal reproduction of this issue using il-repack? It would help with investigations.

hugoqribeiro commented 1 month ago

You mean a simplified package after being processed with il-repack?

I believe that I can reproduce this without il-repack, if I add an internal type named Microsoft.Extensions.Logging.LoggerMessageAttribute in the original project...

Will that help?

dariusclay commented 1 month ago

With il-repack, it can be configured to not internalize specific types. Do you know if this is possible to use in your case?

 - /internalize[:<excludefile>]  sets all types but the ones from the first assembly 'internal'. <excludefile> contains one regex per
                                 line to compare against FullName of types NOT to internalize.
hugoqribeiro commented 1 month ago

With il-repack, it can be configured to not internalize specific types. Do you know if this is possible to use in your case?

 - /internalize[:<excludefile>]  sets all types but the ones from the first assembly 'internal'. <excludefile> contains one regex per
                                 line to compare against FullName of types NOT to internalize.

I'm unsure that having 2 public types with the same name will solve the issue. What I've tried was renaming Microsoft.Extensions.Logging to Cegid.Microsoft.Extensions.Logging, but that breaks the package at runtime (because il-repack is unable to do the replaces everywhere, for some reason that I didn't understand (yet).

Let me try reproducing the issue without il-repack...

hugoqribeiro commented 1 month ago

Can you provide a minimal reproduction of this issue using il-repack? It would help with investigations.

OK. This is reproducible by referencing a simple client library.

Here's a solution that reproduces the problem:

Repo: https://github.com/hugoqribeiro/issues Solution: https://github.com/hugoqribeiro/issues/tree/main/src/2024.05.Microsoft.Extensions.Http.Resilience

dariusclay commented 1 month ago

Thanks for the repro

dariusclay commented 1 month ago

I'm unsure that having 2 public types with the same name will solve the issue.

There's also this /union but not sure how that will look either without actually trying it.