JSInterop: Enable returning null for a nullable value type #60850
Conversation
When a result value is null, and the return type is a nullable value type, Convert.ChangeType throws instead of returning the null value. dotnet#30366
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
|
Pushed a draft of what I wanted the tests to look like, gotta figure out the test fixture stuff too. |
Regenhardt
force-pushed
the
feature/null-value-type-from-js-runtime
branch
from
6f7d836 to
2dad3d3
Compare
|
Ok I kinda figured out how the tests are supposed to work. Just have to install jdk |
Regenhardt
force-pushed
the
feature/null-value-type-from-js-runtime
branch
from
2dad3d3 to
fe68842
Compare
Regenhardt
force-pushed
the
feature/null-value-type-from-js-runtime
branch
from
fe68842 to
0a86864
Compare
|
Figured it out, tests are green now. |
|
Alternative: use |
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
Ready for review. |
| @@ -89,7 +89,9 @@ public void SetResult(object tcs, object? result) | |||
| // If necessary, attempt a cast | |||
| var typedResult = result is T resultT | |||
| ? resultT | |||
| : (T)Convert.ChangeType(result, typeof(T), CultureInfo.InvariantCulture)!; | |||
| : result == null && default(T) == null // ChangeType can't convert null to value types | |||
There was a problem hiding this comment.
It seems OK to me.
@oroztocil please review
There was a problem hiding this comment.
Why default(T) == null instead of typeof(T).IsGenericType && typeof(T).GetGenericTypeDefinition() == typeof(Nullable<>)?
GetGenericTypeDefinition is an intrinsic that will be folded by the compiler, see dotnet/runtime#103528.
There was a problem hiding this comment.
typeof(T).IsGenericType && typeof(T).GetGenericTypeDefinition() == typeof(Nullable<>)
This seems like a better option to me
There was a problem hiding this comment.
@hez2010 I didn't know about intrinsics and folding, and honestly still don't entirely understand it. I changed it to check for the generic type.
There was a problem hiding this comment.
and honestly still don't entirely understand it
The git is able to detect that the expression will be constant for a given T and it's able to optimize it entirely when it generates the machine code for a given T. For example, if you have something like T = int, it's able to know that the condition will always evaluate to false and it can then use that information to simplify the check (and in this case, unconditionally select one branch or another))
javiercn
added
the
pr: pending author input
|
I'm back from vacation and updated the branch, ready for review. |
|
@Regenhardt thanks for the contribution! |
JSInterop: Enable returning null for a nullable value type
Summary of the changes (Less than 80 chars)
Description
When a result value is null, and the return type is a nullable value type, Convert.ChangeType throws instead of returning the null value, so in case off null this now uses null directly.
Fixes #30366