Bugfix SmartParameter source code generation#2041
Conversation
SmartParameter codegen must use the target type for casting, not the type of the source value. Basically, this is a follow up to PR dotnet#1228 (1678a49), which fixed this problem for SmartArgument. Fixes dotnet#2011.
adamsitnik
left a comment
There was a problem hiding this comment.
@mawosoft overall it looks good to me, but could you please add a test that verifies the fix?
|
@adamsitnik As I said, I have to take a second look to see if the fix is complete, but with added tests, I should get an answer to this. |
|
Just adding a failing test to one of the mentioned files should be enough |
mawosoft
left a comment
There was a problem hiding this comment.
@adamsitnik I added some tests, should be ready for review now.
I didn't rebase this branch, so the NativeAOT test failures will still pop up.
| // - TODO op_Explicit is currently not supported by InProcessEmitToolchain (See TryChangeType() in Toolchains/InProcess.Emit.Implementation/Runnable/RunnableReflectionHelpers.cs) | ||
| public static explicit operator TargetType(PublicSource @this) => @this != null ? new TargetType(@this.Data) : null; |
There was a problem hiding this comment.
Not sure if op_Explicit support should be added to TryChangeType().
adamsitnik
left a comment
There was a problem hiding this comment.
Overall looks great, as soon as you remove the reflection, I am going to merge it!
Thanks for writing so many tests @mawosoft ! 👍
PR fixes the following issues: fixes dotnet#2011, fixes dotnet#1812, fixes dotnet#1177.
adamsitnik
left a comment
There was a problem hiding this comment.
! thank you one more time @mawosoft !
SmartParameter codegen must use the target type for casting, not the type of the source value.
Basically, this is a follow up to PR #1228 (1678a49), which fixed this problem for SmartArgument.
EDIT:
Fixes #2011. Probably also #1812 and a few similar issues reported in the past.Fixes #2011, fixes #1812, fixes #1177.
@adamsitnik
Not sure yet if the fix is complete or if the shortcuts in
SmartParamBuilder.CreateForParams()that bypass the creation ofSmartParameterinstances have to be removed. Same forBenchmarkConverter.GetValidValues().(test failures are unrelated, though)