Make Config generator test only for down-casts#106145
Conversation
We were hitting compiler errors when the generator emitted test casts for value types. Since those can never be true (value types cannot be derived from, and the compiler can see if the cast will succeed or not). Fix this by only doing a test cast when we are trying to do a runtime downcast.
ericstj
requested review from
eerhardt,
eiriktsarpalis,
steveharter and
tarekgh
ghost
added
the
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
This looks good. Thanks for getting this fixed.
I just had minor suggestions.
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/Types/CollectionSpec.cs
Outdated
Show resolved
Hide resolved
...nsions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.Collections.cs
Show resolved
Hide resolved
tarekgh
left a comment
tarekgh
left a comment
There was a problem hiding this comment.
Added a couple of questions. LGTM.
These are unsupported and ignored by the generator and a diagnostic is emitted. Binding data to them does nothing.
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Looks good, although it seems like the flag is only kicking in when predetermined pairs of types are detected. Can the source generator produce casts for types other than these collection types, and if so couldn't the shouldTryCast value be determined in the general case? (i.e. by looking at the subtype relationship between the source and target types)
It's everywhere that
I audited this file for uses of |
| #region IConfiguration extensions. | ||
| /// <summary>Attempts to bind the configuration instance to a new instance of type T.</summary> | ||
| [InterceptsLocation(@"src-0.cs", 12, 17)] | ||
| [InterceptsLocation(@"src-0.cs", 13, 17)] |
There was a problem hiding this comment.
Good to see this change in the "version 0" baseline - did you encounter any test issues or did you just remember to do this?
5 participants
Fix #94547
We were hitting compiler errors when the generator emitted test casts for value types. Since those can never be true (value types cannot be derived from, and the compiler can see if the cast will succeed or not).
Fix this by only doing a test cast when we are trying to do a runtime downcast.