Microsoft.NET.Sdk.Web global using statements break Serilog/NLog/log4net consumers on upgrade

[nblumhardt]

From dotnet/templating#3359 (comment)

The .NET 6 version of Microsoft.NET.Sdk.Web introduces a default global using statement for Microsoft.Extensions.Logging. This brings MEL's ILogger into scope everywhere, breaking existing code that uses Serilog's (or another framework's) ILogger interface, with errors like:

/home/pi/dl/nix/seq/tmp/ubuntu.20.04-arm64/src/Seq/Server/Tasks/TaskRunner.cs(15,18): error CS0104: 'ILogger' is an ambiguous reference between 'Microsoft.Extensions.Logging.ILogger' and 'Serilog.ILogger' [/home/pi/dl/nix/seq/tmp/ubuntu.20.04-arm64/src/Seq/Seq.csproj]

The breakage can be worked around by adding <EnableDefaultGlobalUsing>false</EnableDefaultGlobalUsing> or similar to the project's CSPROJ file, but this is a pretty nasty papercut that penalizes users for choosing a logging library other than Microsoft's. The chore will be particularly onerous because of the number of projects affected (and volume of errors within these projects).

I'd also expect that many users will turn to the Serilog project asking for guidance or reporting the breakage as a bug, since the default assumption people make when encountering an issue like this is that "Serilog is broken on .NET 6" - which increases our support burden :-)

CC @DamianEdwards

[KalleOlaviNiemitalo]

For C# projects, implicit usings were disabled by default after 6.0 preview 7; see #19521. I don't think C# projects using Serilog can hit this problem when upgrading to 6.0 rc1 or higher.

The issue seems still valid for new C# projects created with 6.0 templates, though, and perhaps also for all Visual Basic projects.

[nblumhardt]

That sounds much better, thanks for the pointer, @KalleOlaviNiemitalo 👍

Just to clarify, for new projects created from 6.0 templates with <ImplicitUsings>enable</ImplicitUsings>, how will a user who adopts Serilog actually disambiguate ILogger? Will they then need to go back to the CSPROJ and disable all implicit usings to get Serilog's ILogger resolving correctly?

[maxkoshevoi]

Isn't Serrilog uses ILogger abstraction from Microsoft.Extensions.Logging when UseSerilog is added?

[davidfowl]

Will they then need to go back to the CSPROJ and disable all implicit usings to get Serilog's ILogger resolving correctly?

No you don't have to. It would look like this in the csproj:

  <ItemGroup>
    <Using Remove="Microsoft.Extensions.Logging" />
  </ItemGroup>

[davidfowl]

This might be another alternative:

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

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Serilog.Extensions.Hosting" Version="4.1.2" />
  </ItemGroup>

  <ItemGroup>
    <Using Include="Serilog.ILogger" Alias="ILogger" />
  </ItemGroup>

</Project>

OR

global using ILogger = Serilog.ILogger;

using Serilog;

var builder = WebApplication.CreateBuilder(args);
builder.Host.UseSerilog();
var app = builder.Build();

app.MapGet("/", (ILogger logger) => "Hello World!");

app.Run();

Tell customers to introduce a global alias for serilog's ILogger as an override. (This can also be scoped to a file if that is too global).

[nblumhardt]

Thanks for all the info, @davidfowl 👍 - this covers the major concerns I had, and all three options look workable.

Still a little bit sad that .NET 6 will come with a disincentive for using Serilog, even though it's a small one. Having to know how to apply the override (or finding this info) is sure to deter some users.

But, I understand that there are many other trade-offs to balance. Looking forward to .NET 6 all the same 🙂

[davidfowl]

Before you close, lets continue to mull this over. It's a big problem because lots of apps use serilog.

[DamianEdwards]

Here's an idea to discuss (without endorsement or otherwise): Serilog could include a .targets file in its NuGet package such that in projects targeting .NET 6 it either:

• Removes the implicit global using for Microsoft.Extensions.Logging, i.e. <Using Remove="Microsoft.Extensions.Logging" />
• Adds an alias for Serilog.ILogger, i.e. <Using Include="Serilog.ILogger" Alias="ILogger" />

[KalleOlaviNiemitalo]

This would be only for direct Serilog references, right? Not transitive.

If one package adds an Using item for Microsoft.Extensions.Logging and another removes it, then what controls the evaluation order? Does NuGet package restore make it alphabetical?

[swythan]

Note that some users (e.g. my team) use Serilog as an output system, but use Microsoft.Extensions.Logging abstractions for most of the actual logging. It would be confusing to deal with the NuGet reference to Serilog causing ILogger to be switched from one definition to the other.

That said; I'll probably be disabling global usings everywhere, anyway.

[nblumhardt]

Congrats on getting RC1 out there! Guess the clock is ticking (has ticked?) on a conclusion, here.

I've been giving this issue some thought, I don't have any solution beyond the initial suggestions from @davidfowl. Adding targets via the Serilog NUPKG seems like it will shift the issue and break a different set of consumers, as @swythan points out above.

Is the ILogger case is just one of many? Names of types in newer namespaces, and higher in the stack, are more likely to conflict existing code than old, low-level types like DateTime and IEnumerable are. Apart from Serilog's, lots of codebases define their own ILogger. I'd guess there are many projects and libraries with Routes, Endpoints, ConnectionInfos out there. A non-zero number of developers will experience friction when trying to update or consume existing projects and libraries.

I don't have the telemetry, and I'm late to the discussion 😅, but I can't help wanting to suggest that although global usings seem great for System, Collections.Generic, and Tasks, the higher-level frameworks like ASP.NET Core would benefit from a more targeted and visible mechanism.

Throwaway example, namespace re-exports:

namespace Microsoft.AspNetCore.Prelude
{
    public using Microsoft.Extensions.Logging.ILogger;
}

could enable:

using Microsoft.AspNetCore.Prelude;

This would be inserted by templates into Program.cs, Startup.cs, and controllers, but would leave other project files to import and resolve names in the usual way.

If there's any possibility of something like that being available in the future, is there a chance to hold off on global usings apart from just the few truly global ones?

Just my $0.02, and still very much first impressions - only putting this forward because there's no time left in .NET 6, and we don't seem to have more ideas coming on this thread. Would be great to get this thread heading towards a concrete conclusion (even if it's do-nothing/close). Cheers!

[jbgbox]

Hello! Very interesting discussion. I have a couple things to chime in on. I found this issue quite easily with a search on the exact compiler warning in VS2022 - "ilogger is an ambiguous reference". I really appreciate @davidfowl work arounds because its super helpful in the context of the issue I typically run into. What I have been doing is including the namespace on the references for those implementations where I need particular functionality. Another example of this is with System.Text.Json vs Newtonsoft.Json. I am sure there are others, as mentioned, these just seem very visible to a typical developer experience.

Global Using is appealing to me because I like to avoid dependency hell and keep all my references nice and tidy. That said, I understand this is more of an issue when upgrading to a higher version of dependencies when migrating or porting. Could this be more along the lines of a tooling solution? The compiler knows exactly which references are ambiguous. The tooling also knows how to automatically insert a "local using" OR add the namespace to the class. These are options to "repair" this warning. It's not hard to "fix" this when I am developing something brand new.

Similar to renaming, why not give the developer an option in the tooling to apply the "fix" to all ambiguous references of the same type? Like I do when I want to rename something?

Just a thought on what I think would be wise and helpful without having to break some code to do it.

[marcpopMSFT]

Old bug triage: The discussion is interesting but implicit usings has shipped in three releases now and if behavior were going to change, it would have changed by now. I'll go ahead and close this.