Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Sep 19, 2023

Conversation

buyaa-n
Copy link
Member

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

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
Copy link

ghost commented Sep 12, 2023

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
Copy link
Contributor

layomia commented Sep 13, 2023

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
Copy link
Member Author

buyaa-n commented Sep 13, 2023

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
Copy link
Contributor

layomia commented Sep 14, 2023

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
Copy link
Member Author

buyaa-n commented Sep 15, 2023

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
Copy link

ghost commented Sep 15, 2023

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
Copy link
Contributor

layomia commented Sep 15, 2023

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
Copy link
Member Author

buyaa-n commented Sep 15, 2023

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
Copy link
Member Author

buyaa-n commented Sep 15, 2023

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

Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

Thanks!

@buyaa-n
Copy link
Member Author

buyaa-n commented Sep 19, 2023

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
103 of 105 checks passed
@buyaa-n buyaa-n deleted the named-params branch September 19, 2023 00:12
@buyaa-n
Copy link
Member Author

buyaa-n commented Sep 19, 2023

/backport to release/8.0

@github-actions
Copy link
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
3 participants