Adjust calculation of invocation escape when return type is ref to ref struct #65857
Conversation
Do we need to consider only ref-to-ref-struct parameters here when the method returns a ref-to-ref-struct? In reply to: 1343737470 Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:1889 in a8ea1ae. [](commit_id = a8ea1ae, deletion_comment = False) |
| { | ||
| // SPEC: | ||
| // If `M()` does return ref-to-ref-struct, the *safe-to-escape* is the same as the *safe-to-escape* of all arguments which are ref-to-ref-struct. | ||
| // If `M()` does return ref-to-ref-struct, the *ref-safe-to-escape* is the narrowest *ref-safe-to-escape* contributed by all arguments which are ref-to-ref-struct. |
There was a problem hiding this comment.
is the narrowest ref-safe-to-escape contributed by all arguments which are ref-to-ref-struct
When will the ref-safe-to-escape of the ref-to-ref-struct arguments that contribute be different? When one is [UnscopedRef]? If so, consider testing multiple parameters with a mix of { unannotated, [UnscopedRef] } and also { ref, in, out }.
There was a problem hiding this comment.
We can take a reference to a variable from any scope, so one case where they might be different is e.g.
RS rs1 = M();
{
RS rs2 = M();
ref RS result = ref M(ref rs1, ref rs2);
}There was a problem hiding this comment.
This is tested in ReturnRefToRefStruct_RefEscape_01
| private static void Bad3(scoped ref RefStruct s1, int value) | ||
| { | ||
| s1.RefMethod() = new RefStruct(ref value); // 3 | ||
| } |
There was a problem hiding this comment.
Consider testing a static method with a ref-to-ref-struct argument if you haven't already (same comment as #65857 (comment)).
static void Bad4(scoped ref RefStruct s4, int value)
{
GetRef(ref s4) = new RefStruct(ref value); // 4
}
static ref RefStruct GetRef(ref RefStruct s) => ref s;There was a problem hiding this comment.
Added this in to ReturnRefToRefStruct_ValEscape_03.
| } | ||
|
|
||
| [Fact, WorkItem(65648, "https://github.com/dotnet/roslyn/issues/65648")] | ||
| public void ReturnRefToRefStruct_RefEscape_01() |
There was a problem hiding this comment.
This test verifies the following line of the specification
If
M()does return ref-to-ref-struct, the ref-safe-to-escape is the narrowest ref-safe-to-escape contributed by all arguments which are ref-to-ref-struct.
The behavior in practice is not new, though. I couldn't think of a scenario which tests the behavior more deeply, e.g. something where we pick the narrowest of multiple input ref escapes, and ignore an even narrower STE. since the narrowest STE is necessarily wider or equal to the narrowest RSTE.
|
|
||
| GetReadonlyReference1(ref s1).RefField = ref value; // 4 | ||
| GetReadonlyReference2(in s1).RefField = ref value; // 5 | ||
| GetReadonlyReference3(out s1).RefField = ref value; // 6 |
There was a problem hiding this comment.
Are these cases affected by the changes in this PR? It looks like we already report these errors.
There was a problem hiding this comment.
I think the behavior is not changed, but it still feels relevant to test this case, because we can still end up in a path in escape analysis where the method returns readonly ref to ref struct with various kinds of parameters. Basically, I just don't want anything to fall over in that combination, or to unexpectedly permit any of these assignments (as unlikely as that may be).
|
@dotnet/roslyn-compiler for a second review |
| foreach (var (param, argument, _, isArgumentRefEscape) in argsAndParamsAll) | ||
| { | ||
| // SPEC: | ||
| // If `M()` does return ref-to-ref-struct, the *safe-to-escape* is the same as the *safe-to-escape* of all arguments which are ref-to-ref-struct. It is an error if there are multiple arguments with different *safe-to-escape* because of *method arguments must match*. |
There was a problem hiding this comment.
We felt this was more clear to include. It's meant to explain why we are still calculating the narrowest scope here (Math.Max) instead of requiring equal scopes--you shouldn't worry about the case where the scopes actually differ, since an error will be reported elsewhere.
| // (14,9): error CS8332: Cannot assign to a member of method 'GetReadonlyReference3' or use it as the right hand side of a ref assignment because it is a readonly variable | ||
| // GetReadonlyReference3(out s1).RefField = ref value; // 6 | ||
| Diagnostic(ErrorCode.ERR_AssignReadonlyNotField2, "GetReadonlyReference3(out s1).RefField").WithArguments("method", "GetReadonlyReference3").WithLocation(14, 9)); | ||
| } |
There was a problem hiding this comment.
nit: consider test invocation of a ref R ReturnRef(scoped ref R r, ref R r) method
ghost
added this to the 
Implementation of dotnet/csharplang#6781
Closes #65648