Fix 'GetRefEscape' for invocation using old rules #80261
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RikkiGibson
commented
Sep 12, 2025
| // - If `p` is an output parameter, its ref-safe-context is the caller-context. | ||
| // - Otherwise, if `p` is the `this` parameter of a struct type, its ref-safe-context is the function-member. | ||
| // - Otherwise, the parameter is a value parameter, and its ref-safe-context is the function-member. | ||
| if (parameter.RefKind != RefKind.None && !parameter.IsThis) |
Member
Author
There was a problem hiding this comment.
A similar check occurs in line 3001, for the "new rules", where it delegates to helper GetParameterRefEscapeLevel that accounts for UnscopedRef, etc. We could consider also using that helper here. However, I feel the implementation here, is somewhat more clearly in line with the standard.
333fred
approved these changes
Sep 12, 2025
Member
Author
|
Looks like CoreClr test jobs have all timed out. I checked one of the jobs and all the work items are still in Waiting state after 90 minutes. So there might be an infra issue. |
This was referenced Oct 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #80244
The reason this only pops up when indirecting to a local is: the safe-contexts of a local are calc'd when its initializer is visited, using
GetRefEscape()/GetValEscape(), and are stored. When a usage of the local is found, these safe-contexts are looked up and checked against the context of the usage.Simple cases like
return ref rs[1]don't end up actually usingGetRefEscape()forref rs[1]. Instead it usesCheckRefEscape()only, which uses a completely different code path to check the escape, which correctly understood thatthisdoes not escape by-reference.The
GetRefEscape()path needed to be changed to reflect thatthisdoesn't escape by-reference in the old rules.I saw that the
Check()path also sometimes adjusts theMethodInvocationInfoto drop the receiver in order to visit it separately. This didn't seem necessary here.