-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
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.
...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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@@ -36,12 +36,12 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration | |||
{ | |||
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see this change in the "version 0" baseline - did you encounter any test issues or did you just remember to do this?
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.