Repository navigation
Honor property-level TypeConverterAttribute in configuration binding - #134424
vladimir-aubrecht wants to merge 3 commits into
Conversation
Resolve converters for the correct property and constructor parameter, preserve built-in conversion fallbacks, and diagnose reflection fallback in source-generated binding. Cover inheritance, hidden members, unsupported types and binding reachability. AI-assisted implementation with GitHub Copilot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generate opt-in property converter fallbacks and init/required member accessors. Align override metadata, ignored properties, configuration sections and explicit null handling across reflection and generated binding. Add shared regressions, generator diagnostics and compatibility documentation.
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration |
Include IsExternalInit in the independent test snippets and report unexpected compiler diagnostic IDs and messages in both validation modes.
|
I'm not sure I follow the benchmarks - were there something critical in the fixes mentioned? Could we rather compare with the main branch? |
rosebyte
left a comment
There was a problem hiding this comment.
This PR seems to do much more than just add support for TypeConverterAttribute on properties. I suggest moving the accessor code and the changes related to init and required into separate PRs, or leaving them out for now, as we may need further discussion about how to handle those features.
| } | ||
|
|
||
| TypeSpec propTypeSpec = GetEffectiveTypeSpec(property.TypeRef); | ||
| if (property.TypeConverter is not null) |
There was a problem hiding this comment.
I suspect we can return prematurely here if the property is a getter only collection with ConfigurationKeyName attribute.
public class MyOptions : OurOptions
{
[ConfigurationKeyName("Renamed")]
public override List<int> Values { get; } = new();
}| return !IsCollectionAndCannotOverride() && !IsDictWithUnsupportedKey(); | ||
|
|
||
| bool IsAccessible() => property.CanGet || property.CanSet; | ||
| bool IsAccessible() => property.CanGet; |
There was a problem hiding this comment.
I understand this is a general generator / reflection binder fix, it would be great to have these fixes as separate PRs and make PR reviewers less sweat.
| { | ||
| RequiresConstructorAccessor = requiresConstructorAccessor, | ||
| ConstructorAccessor = constructorAccessor, | ||
| BindInitPropertiesAfterConstruction = options.EnableTypeConverters && (!requiresConstructorAccessor || constructorAccessor is not null), |
There was a problem hiding this comment.
Why do we need to account for the required keyword just to give users a new way to convert values? This feels like a separate problem that deserves its own care and focus. Could you please elaborate on why this change is needed? If it is necessary, we should address it in a separate PR and resolve any resulting conflicts in this one.
| IsStatic = property.IsStatic; | ||
| SetOnInit = setterIsPublic && (property.IsRequired || isInitOnly); | ||
| CanSet = setterIsPublic && !isInitOnly; | ||
| CanSet = setterIsPublic && (!isInitOnly || initOnlySetter is not null); |
There was a problem hiding this comment.
Similarly to required, why does adding support for property-level converters require changing how we handle init properties? I would expect the converted value to use the existing construction and assignment paths. Expanding support for binding init properties, particularly on existing instances, feels like a separate change that deserves its own consideration.
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <EnableConfigurationBindingGeneratorTypeConverters Condition="'$(EnableConfigurationBindingGeneratorTypeConverters)' == ''">false</EnableConfigurationBindingGeneratorTypeConverters> |
There was a problem hiding this comment.
Do we need a separate opt-in switch here? I would expect applying TypeConverterAttribute to be sufficient to request the conversion.
I see that existing attributes were previously ignored, but the reflection binder now honours them unconditionally. Why do we need an additional opt-in for generated binding? Unless there is a specific compatibility constraint, I would prefer both implementations to honour the attribute without another project setting.
|
|
||
| EmitBlankLineIfRequired(); | ||
| _writer.WriteLine($$""" | ||
| private static readonly Lazy<global::System.ComponentModel.TypeConverter> {{fieldName}} = |
There was a problem hiding this comment.
Could we measure the published size impact for trimmed and NativeAOT applications? Avoiding TypeDescriptor lookup addresses reflection-based discovery, but the code-size cost of using TypeConverter was a separate concern in #83599.
| <value>Did not generate binding logic for a property on a type</value> | ||
| </data> | ||
| <data name="PropertyTypeConverterRequiresReflectionMessageFormat" xml:space="preserve"> | ||
| <value>Binding logic was not generated for a binder call because a 'TypeConverterAttribute', init-only setter, or required-member initialization could not be handled statically. Binding requires the reflection-based binder, which is not compatible with trimming or Native AOT.</value> |
There was a problem hiding this comment.
Could this diagnostic identify the affected property and converter?
Summary
Honor
TypeConverterAttributeon configuration properties, so a model can customize conversion without globally registering a converter for the property's type.The reflection binder uses the property's converter, including inherited overrides and matching constructor-bound properties. Property-level conversion takes precedence over the type's normal conversion; if the property converter cannot convert from
string, binding falls back to the type's converter and built-in binding behavior.Source-generated binding
Property converters require an explicit opt-in:
EnableConfigurationBindingGeneratorTypeConvertersdefaults tofalse; publishing with AOT or trimming does not enable it automatically.With the opt-in enabled, the generator resolves accessible converters statically and calls them directly, without runtime
TypeDescriptorlookup. A type-level converter is supported as a fallback for an attributed property, not as a general standalone type-level customization mechanism.The changes also preserve reflection/generated parity for:
IConfigurationSection, which is bound directly without constructing a converter.new.[ConfigurationIgnore]on overrides and properties that are not reachable for binding.nullversus a missing key, conversion failures, nullable values, and existingbyte[]append behavior.initandrequiredmembers, including binding existing instances, constructor defaults/normalization, interfaces, and nested generic types.Generated init setters and required-member construction use
UnsafeAccessorwhere available (.NET 8+, or .NET 9+ for generic declaring types). An unavailable accessor or statically unresolved converter producesSYSLIB1105: warning and reflection fallback for normal builds, or an error for AOT/trimmed publishing.Compatibility and design scope
No public managed API is added. The reflection binder's handling of previously ignored property attributes is a behavioral change. The generator's new converter/accessor paths remain opt-in; the fix for ignored overrides applies independently of that switch. Compatibility and migration notes are included in
PACKAGE.md.Related design discussion: #83599. This draft covers the property-attribute scenario from #36545, not the broader extensibility proposal. It does not add
IParsable<T>, a converter-registration API, or dynamicTypeDescriptorregistration support. Future mechanisms can coexist with this path, but their conversion precedence would need to be defined.The opt-in follows one alternative discussed in #83599; it should not be taken as an already agreed extensibility design. In particular:
TypeConverterabstraction. AOT/trim success does not establish that its code-size cost is acceptable. Binary-size impact has not been measured.Validation
On the rebased PR head, on macOS arm64:
Both full test projects were built and run with
dotnet build /t:Test.Additional checks were completed before the conflict-free rebase;
git range-diffconfirmed that both feature patches were unchanged:net11.0/osx-arm64NativeAOT and trimmed applications were published and executed, covering converters, fallback/null semantics, ignored properties, interface setters, and nested/shadowed generic init/required members.Runtime binding microbenchmarks
BenchmarkDotNet short runs on macOS arm64 / .NET 11, with exact assembly hashes verified. The baseline is the earlier implementation on this branch before the parity fixes, not upstream main.
These measurements are limited runtime-binder regression checks, not a claim about source-generator performance or published application size.
Resolves #36545
Note
This pull request was prepared with assistance from GitHub Copilot.