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

[release/8.0] Improve binder gen binding logic (init, member binding, polymorphism) #92118

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Sep 15, 2023

Backport of #91717 to release/8.0.
Fixes #90974, #91324

Customer Impact

Fixes two reported bugs. We expect users to hit them:

  1. Generated code throws when binding to already initialized objects without ctors. It shouldn't throw since it doesn't need to construct the object.
using Microsoft.Extensions.Configuration;

Environment.SetEnvironmentVariable("Value", "42");
IConfiguration b = new ConfigurationBuilder().AddEnvironmentVariables().Build();

Base d = new Derived();
b.Bind(d); // Throws exception saying it can't init, whereas it isn't expected to.
Console.WriteLine(d.Value);

internal abstract class Base
{
    public int Value { get; set; }
}

internal sealed class Derived : Base { }
  1. Generator produces code that fails to compile when a member is collection of types without a ctor.

Given this shape

public class MyClass

{
    public List<MyAbstractClass> Stuff { get; set; } = new List<MyAbstractClass>();
}

public abstract class MyAbstractClass {  }

The generator would emit calls to a method called InitializeMyAbstractClass, but not actually provide an implementation. With this fix, the application would compile successfully and throw at runtime if it needed to configure Stuff.

Additional considerations

Wrt to runtime behavior, root level Bind calls for objects without a known/supported ctor would now bind to the input instance as expected. However if binding to a property declared as an abstract type and there's a matching config value in the payload, then the generator would throw an exception saying it can't init the property -- whether the property instance is null or not.

We need a follow up to reuse non-null instances rather than throw, and only throw if the property instance is null. This will make the generator impl consistent with reflection.

Value of this backport

As mentioned we've fixed root level binding, but the issue remains for properties. Though we need a follow up to address the gap (#92137), securing the fix for root level binding would still be really valuable to users. It's reasonable to believe that a non-trivial amount of real-world config payloads contain just primitives (e.g. numbers, booleans, strings) etc.

We also fix the compilation issue mentioned above. In general we've prioritized fixing compilation issues -- they are super disruptive. It's also an immediate blocker for customers trying to use the generator, whereas if the app compiled they could make progress and perhaps provide feedback / raise further issues.

In summary, we should consider these fixes as valuable enough to take this backport in as-is (and not block on #92137).

Testing

Automated unit and emitted source regression tests were added to verify the fix for the reported scenarios.

Risk

Low. Contained fix for off-by-default component.

@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

Backport of #91717 to release/8.0.

Customer Impact

Fixes two reported bugs. We expect users to hit them:

  1. Generator wrongly emits exception-throwing logic when binding to initialized objects that have no ctors

This is inconsistent with reflection.

using Microsoft.Extensions.Configuration;

Environment.SetEnvironmentVariable("Value", "42");
IConfiguration b = new ConfigurationBuilder().AddEnvironmentVariables().Build();

Base d = new Derived();
b.Bind(d); // Throws exception saying it can't init, whereas it isn't expected to.
Console.WriteLine(d.Value);

internal abstract class Base
{
    public int Value { get; set; }
}

internal sealed class Derived : Base { }
  1. Fixes compilation error when an abstract/interface type is a collection or object member

Given this shape

class MyClass
{
    public MyAbstractClass Member { get; set; }
}

public abstract MyAbstractClass { get; }

The generator would emit calls to a method called InitializeMyAbstractClass, but not actually provide an implementation. The generator knows that it can't init the member, so we've updated the UX such that we generate no logic for that property, and issue a compilation warning that it will be dropped from binding logic.

Testing

Extensive unit and emitted source regression tests were added to verify the fix for the reported scenarios. We also added tests for additional edge case scenarios.

Risk

Low. Contained fix for off-by-default component.

Author: layomia
Assignees: layomia
Labels:

area-Extensions-Configuration

Milestone: 8.0.0

@layomia layomia added the Servicing-consider Issue for next servicing release review label Sep 15, 2023
@ericstj
Copy link
Member

ericstj commented Sep 15, 2023

The generator knows that it can't init the member, so we've updated the UX such that we generate no logic for that property, and issue a compilation warning that it will be dropped from binding logic.

What about the case where the property exposes an existing instance and we're binding to that? Could we still set the members on that type without creating the instance?
https://sharplab.io/#v2:D4AQTAjAsAULIGYAE4kFkCeBhANgQwGcDYBvWJClZTAQQCMCAXAJzwGNHdCD0BTAWzq9mSEkgDmvRgG4kBKbIC+SALxIAdrwDu6DABFhASwBuvACZciACgCU02ItjxkeBi3aMUYXfSasOljykSOSUiEiufh4oEAAMAPxIvu4caAJCImKSMnIKSI4wBc5eugbMJuaBSABcPm7+nPhEpKEU4QD2pszlZrwxCUn1HmmCwqISCvI5BYpAA==

@ericstj
Copy link
Member

ericstj commented Sep 15, 2023

It looks like this case wasn't supported already and remains unsupported. I'll file a new bug on it.
#92137

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

This scenario is important to fix - binding to abstract types. This PR makes progress towards that goal. Of course fixing scenarios that have bad-code gen is a top priority.

I wish this were smaller, but I understand that some of the refactoring here was necessary for future changes and has been reviewed and approved by multiple folks familiar with the codebase.

I'll approve this to move forward with the work in this component. @artl93 would like your eyes here too.

@@ -177,60 +177,7 @@ public class MyClass { }
}

[Fact]
public async Task BindCanParseMethodParam()
Copy link
Member

Choose a reason for hiding this comment

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

I see this test was moved to Bind_CanParseTargetConfigType_FromMethodParam as a baseline test instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was a bit brittle as the entry point code evolved.

Assert.Equal(12, d.Where(diag => diag.Id == Diagnostics.TypeNotSupported.Id).Count());
Assert.Equal(10, d.Where(diag => diag.Id == Diagnostics.PropertyNotSupported.Id).Count());
Assert.Equal(47, d.Where(diag => diag.Id == Diagnostics.TypeNotSupported.Id).Count());
Assert.Equal(44, d.Where(diag => diag.Id == Diagnostics.PropertyNotSupported.Id).Count());
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this exactly number is really something to assert against if it will churn a lot and is influenced by things outside this test. No need to address that here, but consider for future.

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

M2 Approved.

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 15, 2023
@ericstj ericstj merged commit 90d54f9 into dotnet:release/8.0 Sep 16, 2023
107 of 114 checks passed
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants