-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix handling of enum default values in RDF and RDG #63086
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
Conversation
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.
Pull Request Overview
This PR fixes handling of enum default values in Request Delegate Factory (RDF) and Request Delegate Generator (RDG) to ensure proper type casting and formatting. The changes address an issue where enum default values were not being handled correctly in code generation scenarios.
Key changes:
- Enhanced default value string generation to properly handle enum types with correct casting syntax
- Added proper type conversion for enum default values in expression trees
- Added comprehensive test coverage for enum default values including nullable enums
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/Shared/RoslynUtils/SymbolExtensions.cs | Added enum-specific handling in default value string generation with proper casting syntax |
src/Http/Http.Extensions/src/RequestDelegateFactory.cs | Added CreateDefaultValueExpression method to handle enum type conversions in expression trees |
src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.SpecialTypes.cs | Added test cases for enum default values and modernized array syntax |
Comments suppressed due to low confidence (1)
src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.SpecialTypes.cs:212
- The test case shows nullable enum with non-null value marked as
false
for the last parameter, but there's no test case for nullable enum with null value marked astrue
. Consider adding a test case like["TodoStatus?", "null", null, true]
to ensure null handling is properly tested.
["TodoStatus?", "TodoStatus.Done", (TodoStatus?)TodoStatus.Done, false],
f1169d2
to
672b04b
Compare
@BrennanConroy Can I trouble you for a review? This helps unblock dotnet-monitor's migration to native AOT. |
@@ -2358,7 +2393,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al | |||
{ | |||
// Convert(bodyValue ?? SomeDefault, Todo) | |||
return Expression.Convert( | |||
Expression.Coalesce(BodyValueExpr, Expression.Constant(parameter.DefaultValue)), | |||
Expression.Coalesce(BodyValueExpr, CreateDefaultValueExpression(parameter.DefaultValue, typeof(object))), |
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.
parameter.ParameterType
?
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.
BodyValueExpr
is typed object (ref) so we need the types to match here.
["ushort", "ushort.MinValue", ushort.MinValue, true], | ||
["TodoStatus", "TodoStatus.Done", TodoStatus.Done, true], | ||
["TodoStatus", "TodoStatus.InProgress", TodoStatus.InProgress, true], | ||
["TodoStatus", "TodoStatus.NotDone", TodoStatus.NotDone, true], |
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.
["TodoStatus", "TodoStatus.NotDone", TodoStatus.NotDone, true], | |
["TodoStatus", "TodoStatus.NotDone", TodoStatus.NotDone, true], | |
["TodoStatus", "1", 1, true], |
Enum types don't match
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.
The compiler technically doesn't allow this syntax without an explicit conversion via (TodoStatus)1
. I believe you can only do TodoStatus someEnum = 1
if you emit the IL yourself. We can test with the explicit cast though.
["decimal", "decimal.One", decimal.One, true], | ||
["decimal", "decimal.Zero", decimal.Zero, true], | ||
["long", "long.MaxValue", long.MaxValue, true], | ||
["long", "long.MinValue", long.MinValue, true], |
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.
["long", "long.MinValue", long.MinValue, true], | |
["long", "long.MinValue", long.MinValue, true], | |
["long", "3.14", 3.14, true], |
Non-enum types don't match
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.
Ditto the about about casting.
} | ||
catch | ||
{ | ||
converted = targetType.IsValueType ? Activator.CreateInstance(targetType) : null; |
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.
When would this happen?
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.
Actually, I think we can get rid of this case. The Convert.ChangeType
can throw if the conversion is not supported (which I think our IsAssignableFrom covers) or if the conversion overflows (which I don't think can functionally happen if the compiler catches you doing something like short foo = 523525242
.
Closes #62910.