dotnet / runtime

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

ConfigurationBinder source generator generates code with the wrong nullable annotations #94105

Open eerhardt opened 10 months ago

eerhardt commented 10 months ago

Description

When attempting to use the ConfigurationBinder source generator in dotnet/extensions (see https://github.com/dotnet/extensions/pull/4625), I'm getting a nullable warning that is causing the generated code to not compile (I would need to disable nullable validation for the whole project if I wanted to ignore the nullable warning, which isn't acceptable).

I'm not sure what the general pattern is here that causes this issue, so I logged the issue for the scenario specifically. I think it might need to include nested generics where one of the nested generic types is nullable.

Reproduction Steps

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
    <NoWarn>$(NoWarn);SYSLIB1100;SYSLIB1101</NoWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.0-rtm.23511.16" />
    <PackageReference Include="Microsoft.Extensions.Http.Resilience" Version="8.0.0-rc.2.23510.2" />
  </ItemGroup>

</Project>
// needed to work around https://github.com/dotnet/runtime/issues/94065
global using Polly;
global using Polly.Hedging;
global using Polly.RateLimiting;
global using Polly.Timeout;

using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Http.Resilience;

var c = new ConfigurationBuilder().Build();
c.Get<HttpStandardHedgingResilienceOptions>();

Expected behavior

Should be without warnings

Actual behavior

Get a nullable warning from generated code:

CS8619  Nullability of reference types in value of type 'Func<HedgingActionGeneratorArguments<HttpResponseMessage>, Func<ValueTask<Outcome<HttpResponseMessage>>>?>' doesn't match target type 'Func<HedgingActionGeneratorArguments<HttpResponseMessage>, Func<ValueTask<Outcome<HttpResponseMessage>>>>'. ConsoleApp101   C:\Users\eerhardt\source\repos\ConsoleApp101\ConsoleApp101\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs   1418        

The generated code looks like:

            if (AsConfigWithChildren(configuration.GetSection("ActionGenerator")) is IConfigurationSection section256)
            {
                Func<HedgingActionGeneratorArguments<HttpResponseMessage>, Func<ValueTask<Outcome<HttpResponseMessage>>>>? temp258 = instance.ActionGenerator;
                if (temp258 is not null)
                {

That middle line is the issue, it is of type:

Func<HedgingActionGeneratorArguments<HttpResponseMessage>, Func<ValueTask<Outcome<HttpResponseMessage>>>> but the type of instance.ActionGenerator is: Func<HedgingActionGeneratorArguments<TResult>, Func<ValueTask<Outcome<TResult>>>?>

Note the ? inside the last > brace.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

cc @tarekgh @ericstj

ghost commented 10 months ago

Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When attempting to use the ConfigurationBinder source generator in dotnet/extensions (see https://github.com/dotnet/extensions/pull/4625), I'm getting a nullable warning that is causing the generated code to not compile (I would need to disable nullable validation for the whole project if I wanted to ignore the nullable warning, which isn't acceptable). I'm not sure what the general pattern is here that causes this issue, so I logged the issue for the scenario specifically. I think it might need to include nested generics where one of the nested generic types is nullable. ### Reproduction Steps ```xml Exe net8.0 enable enable true $(NoWarn);SYSLIB1100;SYSLIB1101 ``` ```csharp // needed to work around https://github.com/dotnet/runtime/issues/94065 global using Polly; global using Polly.Hedging; global using Polly.RateLimiting; global using Polly.Timeout; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Http.Resilience; var c = new ConfigurationBuilder().Build(); c.Get(); ``` ### Expected behavior Should be without warnings ### Actual behavior Get a nullable warning from generated code: ``` CS8619 Nullability of reference types in value of type 'Func, Func>>?>' doesn't match target type 'Func, Func>>>'. ConsoleApp101 C:\Users\eerhardt\source\repos\ConsoleApp101\ConsoleApp101\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs 1418 ``` The generated code looks like: ```csharp if (AsConfigWithChildren(configuration.GetSection("ActionGenerator")) is IConfigurationSection section256) { Func, Func>>>? temp258 = instance.ActionGenerator; if (temp258 is not null) { ``` That middle line is the issue, it is of type: `Func, Func>>>` but the type of `instance.ActionGenerator` is: `Func, Func>>?>` Note the `?` inside the last `>` brace. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information cc @tarekgh @ericstj
Author: eerhardt
Assignees: -
Labels: `area-Extensions-Configuration`
Milestone: -
eiriktsarpalis commented 10 months ago

That's surprising, doesn't the generated code suppress nullability warnings in source?

eerhardt commented 10 months ago

That's surprising, doesn't the generated code suppress nullability warnings in source?

No, it explicitly enables it:

https://github.com/dotnet/runtime/blob/44a5abd54c8c6888cdbb7fac490279fd7ee3334e/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs#L39-L43

eiriktsarpalis commented 10 months ago

That seems surprising given that this was picked up from the JSON generator:

https://github.com/dotnet/runtime/blob/c1f43410e677b7a4d34113582ae4703aa6c1cc99/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs#L136-L145

I think we should consistently disable nullability warnings in all generated code (except perhaps in debug builds of the generator?)

eiriktsarpalis commented 10 months ago

@eerhardt I'm preparing a PR that disables nullability warnings in generated code. I'm not super familiar with this library though, do you know if there's a minimal repro I can use that doesn't require the dependency on Polly?

eerhardt commented 10 months ago

Unfortunately, no I don't. I haven't had time to pare it down.

I would try making a type of Func<HedgingActionGeneratorArguments<TResult>, Func<ValueTask<Outcome<TResult>>>?>

where HedgingActionGeneratorArguments and Outcome are generic types.

eiriktsarpalis commented 10 months ago

Is that warning a side-effect of the generator erroneously generating binding code for delegate types?

eerhardt commented 10 months ago

Yes, but I believe the issue will also occur if you replace the Func with some other generic types.

eiriktsarpalis commented 10 months ago

This was addressed in .NET 8 via https://github.com/dotnet/runtime/pull/94267 by disabling nullability warnings altogether. Moving to Future for a fix that addresses the specific annotation issue.

eiriktsarpalis commented 1 month ago

Moving to 10.0.0