dotnet / runtime

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

ConfigurationBinder generates initialization logic for root type and never calls it when only generating Bind interceptors #94071

Open eerhardt opened 1 year ago

eerhardt commented 1 year ago

Description

When trying to use the ConfigurationBinder source generator with the Azure SDK's ClientOptions objects I'm getting compile errors. However, the compile errors come from a method that isn't even invoked.

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="Azure.Storage.Blobs" Version="12.17.0" Aliases="AzureStorageBlobs" />
  </ItemGroup>

</Project>
extern alias AzureStorageBlobs;
global using AzureStorageBlobs.Azure.Storage.Blobs;
global using AzureStorageBlobs.Azure.Storage.Blobs.Models;
using Microsoft.Extensions.Configuration;

Console.WriteLine("Hello, World!");

static void BindToConfiguration(BlobClientOptions options, IConfiguration configuration)
{
    var configurationOptionsSection = configuration.GetSection("ConfigurationOptions");
    configurationOptionsSection.Bind(options);
}

namespace Azure.Storage.Blobs.Models { }

(note the alias, global usings, and empty namespace is to work around #93498 and #94065

Expected behavior

The source generator should work for these options types. The Azure SDK currently uses the reflection-based config binder here:

https://github.com/Azure/azure-sdk-for-net/blob/6d43c499a7cd6bf6b1aba9ea220ba22e54e6f7e1/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientBuilderExtensions.cs#L83

Actual behavior

You get a build error:

C:\Users\eerhardt\source\repos\ConsoleApp101\ConsoleApp101\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(450,56,450,58): error CS0266: Cannot implicitly convert type 'int' to 'Azure.Storage.Blobs.BlobClientOptions.ServiceVersion'. An explicit conversion exists (are you missing a cast?)

This is coming from this generated code:

        public static BlobClientOptions InitializeBlobClientOptions(IConfiguration configuration, BinderOptions? binderOptions)
        {
            BlobClientOptions.ServiceVersion version = 17;
            if (configuration["Version"] is string value73)
            {
                version = ParseEnum<BlobClientOptions.ServiceVersion>(value73, () => configuration.GetSection("Version").Path);
            }

            return new BlobClientOptions(version);
        }

However, this InitializeBlobClientOptions method isn't even called.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

ghost commented 1 year 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 trying to use the ConfigurationBinder source generator with the Azure SDK's ClientOptions objects I'm getting compile errors. However, the compile errors come from a method that isn't even invoked. ### Reproduction Steps ```xml Exe net8.0 enable enable true $(NoWarn);SYSLIB1100;SYSLIB1101 ``` ```csharp extern alias AzureStorageBlobs; global using AzureStorageBlobs.Azure.Storage.Blobs; global using AzureStorageBlobs.Azure.Storage.Blobs.Models; using Microsoft.Extensions.Configuration; Console.WriteLine("Hello, World!"); static void BindToConfiguration(BlobClientOptions options, IConfiguration configuration) { var configurationOptionsSection = configuration.GetSection("ConfigurationOptions"); configurationOptionsSection.Bind(options); } namespace Azure.Storage.Blobs.Models { } ``` (note the alias, global usings, and empty namespace is to work around #93498 and #94065 ### Expected behavior The source generator should work for these options types. The Azure SDK currently uses the reflection-based config binder here: https://github.com/Azure/azure-sdk-for-net/blob/6d43c499a7cd6bf6b1aba9ea220ba22e54e6f7e1/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientBuilderExtensions.cs#L83 ### Actual behavior You get a build error: ``` C:\Users\eerhardt\source\repos\ConsoleApp101\ConsoleApp101\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(450,56,450,58): error CS0266: Cannot implicitly convert type 'int' to 'Azure.Storage.Blobs.BlobClientOptions.ServiceVersion'. An explicit conversion exists (are you missing a cast?) ``` This is coming from this generated code: ```csharp public static BlobClientOptions InitializeBlobClientOptions(IConfiguration configuration, BinderOptions? binderOptions) { BlobClientOptions.ServiceVersion version = 17; if (configuration["Version"] is string value73) { version = ParseEnum(value73, () => configuration.GetSection("Version").Path); } return new BlobClientOptions(version); } ``` However, this `InitializeBlobClientOptions` method isn't even called. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: eerhardt
Assignees: -
Labels: `area-Extensions-Configuration`
Milestone: -
tarekgh commented 1 year ago

I thought this might be type conversion issue but it looks a simple enough fixing it by casting the int to the enum ServiceVersion

ericstj commented 1 year ago

The enum bug is already fixed with https://github.com/dotnet/runtime/pull/93160 which we'll backport in https://github.com/dotnet/runtime/pull/94070.

This issue is tracking the fact that we generate a InitializeBlobClientOptions method but never call it.

I was able to isolate that repro down to the case where we only ever Bind to a type and that type has a constructor initialization strategy. Since the generator only produces Bind logic (and not Get) it never calls the initialization method.

Simplified repro: bindCtor.zip

Expect: All methods generated are necessary Actual: InitializeMyConfig is generated but never called.

In fixing this we should see if there's a way to make the unreachable code analyzer catch this sort of thing on our generated code - at least in our tests.