Skip to content

Fix: Config binder generator doesn't generate code when named arguments are out of order#91961

Merged
buyaa-n merged 7 commits into
dotnet:mainfrom
buyaa-n:named-params
Sep 19, 2023
Merged

Fix: Config binder generator doesn't generate code when named arguments are out of order#91961
buyaa-n merged 7 commits into
dotnet:mainfrom
buyaa-n:named-params

Conversation

@buyaa-n

@buyaa-n buyaa-n commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Config binder generator doesn't generate code when named arguments are out of order like in:

internal void Configure(IConfiguration configuration) =>
    ConfigurationBinder.Bind(instance: this, configuration: configuration);

The issue also happens for ConfigurationBinder.Get(...) overloads.

Fixes #90908

@ghost

ghost commented Sep 12, 2023

Copy link
Copy Markdown

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

Issue Details

Config binder generator doesn't generate code when named arguments are out of order like in:

internal void Configure(IConfiguration configuration) =>
    ConfigurationBinder.Bind(instance: this, configuration: configuration);

The issue also happens for ConfigurationBinder.Get(...) overloads.

Fixes #90908

Author: buyaa-n
Assignees: -
Labels:

area-System.Configuration

Milestone: -

@layomia

layomia commented Sep 13, 2023

Copy link
Copy Markdown
Contributor

The issue also happens for ConfigurationBinder.Get(...) overloads.

I believe it applies to all the extensions methods -- so in addition to ConfigurationBinder.cs, we'd need to change the parsing in OptionsBuilder...cs and OptionsConfigurationServiceCollection...cs.

@buyaa-n

buyaa-n commented Sep 13, 2023

Copy link
Copy Markdown
Contributor Author

I believe it applies to all the extensions methods -- so in addition to ConfigurationBinder.cs, we'd need to change the parsing in OptionsBuilder...cs and OptionsConfigurationServiceCollection...cs.

Did not found any indexed access of arguments in those files, only TypeArguments which is safe and no need to change.

@layomia

layomia commented Sep 14, 2023

Copy link
Copy Markdown
Contributor

I believe it applies to all the extensions methods -- so in addition to ConfigurationBinder.cs, we'd need to change the parsing in OptionsBuilder...cs and OptionsConfigurationServiceCollection...cs.

Did not found any indexed access of arguments in those files, only TypeArguments which is safe and no need to change.

Per offline discussion, we do need a fix/tests for these as well, e.g.

using System;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
					
public class Program
{
	public static void Main()
	{
		ServiceCollection services = new();
		IConfiguration configuration = new ConfigurationBuilder().Build();
		services.Configure<string>(name: "string", configureBinder: null, config: configuration);
	}
}

@buyaa-n

buyaa-n commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

Per offline discussion, we do need a fix/tests for these as well, e.g.

using System;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
					
public class Program
{
	public static void Main()
	{
		ServiceCollection services = new();
		IConfiguration configuration = new ConfigurationBuilder().Build();
		services.Configure<string>(name: "string", configureBinder: null, config: configuration);
	}
}

@layomia I have been testing similar code to see if I can repro it, but encountering errors similar to #91258 even though I am using latest 8.0.100-rc.2.23464.22 SDK:

\ConfigTestWebApp\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(70,35): 
  error CS0246: The type or namespace name 'IOptionsChangeTokenSource<>' could not be found (are you missing a using directive or an assembly reference?)
\ConfigTestWebApp\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(70,76): 
  error CS0246: The type or namespace name 'ConfigurationChangeTokenSource<>' could not be found (are you missing a using directive or an assembly reference?) 

@ghost

ghost commented Sep 15, 2023

Copy link
Copy Markdown

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

Issue Details

Config binder generator doesn't generate code when named arguments are out of order like in:

internal void Configure(IConfiguration configuration) =>
    ConfigurationBinder.Bind(instance: this, configuration: configuration);

The issue also happens for ConfigurationBinder.Get(...) overloads.

Fixes #90908

Author: buyaa-n
Assignees: buyaa-n
Labels:

area-Extensions-Configuration

Milestone: -

@layomia

layomia commented Sep 15, 2023

Copy link
Copy Markdown
Contributor

That's odd. Checking that out.

In the meantime, you can verify your fix by adding unit tests like the ones you add for ConfigurationBinder

@buyaa-n

buyaa-n commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

In the meantime, you can verify your fix by adding unit tests like the ones you add for ConfigurationBinder

Yes, of course that is the plan, just wanted to let you know about these errors as it might be important.

…, add test for OptionsBuilder... and ServiceCollection extensins
@buyaa-n

buyaa-n commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

I believe it applies to all the extensions methods -- so in addition to ConfigurationBinder.cs, we'd need to change the parsing in OptionsBuilder...cs and OptionsConfigurationServiceCollection...cs.

Did not found any indexed access of arguments in those files, only TypeArguments which is safe and no need to change.

Per offline discussion, we do need a fix/tests for these as well, e.g.

using System;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
					
public class Program
{
	public static void Main()
	{
		ServiceCollection services = new();
		IConfiguration configuration = new ConfigurationBuilder().Build();
		services.Configure<string>(name: "string", configureBinder: null, config: configuration);
	}
}

Added unit tests for those, there were no need a code fix for those extension methods

@buyaa-n buyaa-n requested a review from layomia September 15, 2023 19:01

@layomia layomia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@buyaa-n

buyaa-n commented Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

One CI build not finishes even running for more than 3.5hrs, going to merge

@buyaa-n buyaa-n merged commit 6d7cf0e into dotnet:main Sep 19, 2023
@buyaa-n buyaa-n deleted the named-params branch September 19, 2023 00:12
@buyaa-n

buyaa-n commented Sep 19, 2023

Copy link
Copy Markdown
Contributor Author

/backport to release/8.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6229413419

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Config binder generator doesn't generate code when named arguments are out of order

4 participants