Prefer most derived member in Configuration Binder source generator by ericstj · Pull Request #101316 · dotnet/runtime

ericstj · 2024-04-19T22:40:08Z

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

ericstj · 2024-04-19T22:40:08Z

Fix#101267 #101273

CC @eerhardt

dotnet-policy-service · 2024-04-19T22:40:33Z

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

tarekgh

LGTM!

eerhardt

LGTM. Thanks for the quick fix here!

Do you think this could be backported to 8.0.x?

eiriktsarpalis

Per #101316 (review) I suspect that this might be introducing a slight regression.

Given that this is a likely candidate for backporting, it might make sense to investigate further.

ericstj · 2024-04-22T15:29:46Z

I'll add a test case for an override that only overrides the Getter. We did have a test for the case where a new member only provides a get - in which case we're intentionally not binding.

ericstj · 2024-04-22T18:41:43Z

Seems we still have a failure of the Reflection binder:

System.InvalidOperationException : Failed to convert configuration value at 'E' to type 'Microsoft.Extensions.Configuration.Binder.Tests.ConfigurationBinderTests+TestSettingsEnum'.
---- System.FormatException : OptionB is not a valid value for TestSettingsEnum.
-------- System.ArgumentException : Requested value 'OptionB' was not found.

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

ericstj · 2024-04-22T20:30:54Z

Interesting: the reflection binder has a different heuristic.

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 987 to 1004 in c1f9161

while(baseType!=typeof(object))
{
PropertyInfo[]properties=baseType.GetProperties(DeclaredOnlyLookup);
foreach(PropertyInfopropertyinproperties)
{
// if the property is virtual, only add the base-most definition so
// overridden properties aren't duplicated in the list.
MethodInfo?setMethod=property.GetSetMethod(true);
if(setMethodisnull||!setMethod.IsVirtual||setMethod==setMethod.GetBaseDefinition())
{
allProperties.Add(property);
}
}
baseType=baseType.BaseType!;
}

It never does any resolution by name, but builds a list in order from derived to base. So it will set both the hidden and non-hidden members through reflection, using the same configuration data. That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

tarekgh · 2024-04-22T20:51:08Z

@ericstj I think we need to keep the runtime behavior for consistency. Even if it looks unusual behavior but seems not many people complained about it.

eerhardt · 2024-04-22T21:19:57Z

@eerhardt -- does that sound right? That seems to indicate that #101273 might be broken even without the source generator.

See my comment on #101273 (comment). For the case I hit the issue, the base enum is a superset of the derived enum. The derived class is trying to limit which values can be set.

eiriktsarpalis · 2024-04-23T11:15:40Z

That's unusual behavior - but we could replicate it if we casted the LHS in the source generator to the base type.

Do we want to? Are there tests validating this behavior?

ericstj · 2024-04-23T16:47:32Z

I'm not planning to change the reflection binder's behavior in this PR. Nor am I planning to change the source generator to bind many different propreties like the reflection binder does.

I'm inclined to more closely match the reflection-binder's selection algorithm for finding the most-derived property. I think that will be the minimal change while unblocking @eerhardt. I'm running some tests and will make a new change to this PR shortly.

…definitions

ericstj · 2024-04-29T15:46:50Z

Failures are known issues and don't block build or testing.

ericstj · 2024-04-29T15:47:07Z

/backport to release/8.0-staging

github-actions · 2024-04-29T15:47:18Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8881506956

…otnet#101316) * Prefer most derived member in Configuration Binder source generator * Skip overridden properties in config source generator - include only definitions

Prefer most derived member in Configuration Binder source generator
654150f

ericstj requested review from eerhardt, eiriktsarpalis and tarekgh April 19, 2024 22:40

ghost added the area-Extensions-Configuration label Apr 19, 2024

dotnet-policy-serviceBot assigned ericstj Apr 19, 2024

tarekgh approved these changes Apr 19, 2024
View reviewed changes

eerhardt reviewed Apr 20, 2024
View reviewed changes

Comment thread...libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs Outdated

eerhardt approved these changes Apr 20, 2024
View reviewed changes

eiriktsarpalis reviewed Apr 20, 2024
View reviewed changes

Comment thread...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs Outdated

eiriktsarpalis requested changes Apr 20, 2024
View reviewed changes

Skip overridden properties in config source generator - include only …
5aac0b5
…definitions

ericstj requested a review from eiriktsarpalis April 26, 2024 21:43

This was referenced Apr 27, 2024
[WASI][AOT] LLVM ERROR: out of memory #101533
Closed
[wasm][firefox] crit: OpenQA.Selenium.WebDriverException: Failed to decode response from marionette #101617
Open
[browser][MT] WasmBrowserTestRunner.RunConsoleMessagesPump - abrupt disconect #101618
Closed

tarekgh approved these changes Apr 27, 2024
View reviewed changes

eiriktsarpalis approved these changes Apr 27, 2024
View reviewed changes

build-analysisBot mentioned this pull request Apr 27, 2024
AbbreviatedMonthGenitiveNames_Get_ReturnsExpected_HybridGlobalization failing with string mismatch #101634
Closed

ericstj merged commit 9b4c5c6 into dotnet:mainApr 29, 2024

github-actionsBot mentioned this pull request Apr 29, 2024
[release/8.0-staging] Prefer most derived member in Configuration Binder source generator #101686
Merged
4 tasks

eerhardt mentioned this pull request Apr 30, 2024
ConfigurationBinder source generator generates non-compilable code with new property changing enum types #101273
Closed

github-actionsBot locked and limited conversation to collaborators May 31, 2024