-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Support open delegate for Nullable<> #42837
Conversation
Tagging subscribers to this area: @safern, @ViktorHofer |
/azp list |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
// Auto-generated message 69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts. To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others. |
@davidwrighton and @lambdageek will follow up on this. |
f0b3a09
to
d51eaa3
Compare
@lambdageek This change looks good from the coreclr point of view, but I'm not prepared to merge with a comment about what you'd like done for the mono interpreter. I could disable the test for the interpreter runs and for wasm, or either you or @amos402 could look into fixing the interpreter. |
@davidwrighton Please disable the test and ping me with the issue. The interpreter has a separate path through our (convoluted) delegate code and I don't think it's necessary to hold up this PR. |
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 is a good change. I'd like to apologize for how long it took for us to get started on review of it.
Port of dotnet/runtime#42837. Won't win any perf prizes, but will do for now. I don't think the amount of work required for good support will ever meet the bar.
Port of dotnet/runtime#42837. Won't win any perf prizes, but will do for now. I don't think the amount of work required for good support will ever meet the bar.
Because of the boxing behavior specialization for
Nullable<>
, after the boxing, a nullable object may be boxed by the type it referred to or justnull
, the original type information lost.For example,
Thus it's reasonable that a closed delegate binding for
Nullable<>
such asDelegate.CreateDelegate(invokeType, boxedNullable, method)
is prohibited.However, if for an open delegate case for
Nullable<>
, theinvokeType
must point to a delegate in which the first parameter isref Nullable<XXX>
, so it's no need to worry about the type information lost.Currently, there's a special check for nullable type:
runtime/src/coreclr/src/vm/comdelegate.cpp
Lines 2614 to 2617 in 38bf4df
This specialized check is unnecessary, we can just remove it.
case 1: a closed delegate with boxed nullable value, no matter which type it would be, it just can't pass the type validation because the type won't match the nullable type.
case 2: an open delegate targeted by
xx Func(ref Nullable<XXX> self)
, theself
type for the target method would use its reference type, thefromHandle
andtoHandle
pass toIsLocationAssignable
would be the same one.It makes the code like as follow can't be used on .NetCore(.Net framework as well), on the other side, mono can.
The present version throws an
ArgumentException
for these cases.