Out as return only #64318
Out as return only #64318
Conversation
src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs
Outdated
Show resolved
Hide resolved
| if (parameter?.EffectiveScope == DeclarationScope.ValueScoped) | ||
| foreach (var (parameter, argument, refKind) in escapeArguments) | ||
| { | ||
| // This means it's part of an __arglist or function pointer receiver. |
There was a problem hiding this comment.
I'm confused as to why function pointer receiver is a scenario here. Isn't the receiver just the function pointer itself? It seems like nothing can escape into it or out of it.
There was a problem hiding this comment.
I was attempting to maintain the original code here. I'm also unsure why a function pointer could come into play here. Not sure you can really do those on a ref struct. I will try removing and see what happens.
| // (12,79): error CS9075: Cannot return a parameter by reference 'y' because it is scoped to the current method | ||
| // static void F51(scoped ref R x, scoped ref R y) { F1(ref x, __arglist(ref y)); } // 6 | ||
| Diagnostic(ErrorCode.ERR_RefReturnScopedParameter, "y").WithArguments("y").WithLocation(12, 79)); | ||
| comp.VerifyEmitDiagnostics(); |
There was a problem hiding this comment.
We should have errors here, at least when we call F1 and the argument for parameter ref R a is scoped ref. This is because we could assign __refvalue(...) = M(ref a) in the implementation of F1 and ref a will escape.
There was a problem hiding this comment.
This change updates ref passed to arglist as not being being treated like any other ref parameter. So they can escape to caller but not in a cyclic fashion.
| }"; | ||
| var comp = CreateCompilation(source, runtimeFeature: RuntimeFlag.ByRefFields); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (9,59): error CS8374: Cannot ref-assign 'tValue' to 'F' because 'tValue' has a narrower escape scope than 'F'. | ||
| // static void AssignValueToValue<T>(S<T> s, T tValue) { s.F = ref tValue; } // 1 | ||
| Diagnostic(ErrorCode.ERR_RefAssignNarrower, "s.F = ref tValue").WithArguments("F", "tValue").WithLocation(9, 59), | ||
| // (10,59): error CS9079: Cannot ref-assign 'tRef' to 'F' because 'tRef' can only escape the current method through a return statement. | ||
| // static void AssignRefToValue<T>(S<T> s, ref T tRef) { s.F = ref tRef; } // 14 |
There was a problem hiding this comment.
STE of S<T> s is calling method and RSTE of tRef is return only. so it makes sense that you can't escape tRef by-ref into s.F.
fwiw, I don't think there's any reason to do this assignment in real world code.
| }"; | ||
| var comp = CreateCompilation(source, runtimeFeature: RuntimeFlag.ByRefFields); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (9,59): error CS8374: Cannot ref-assign 'tValue' to 'F' because 'tValue' has a narrower escape scope than 'F'. | ||
| // static void AssignValueToValue<T>(S<T> s, T tValue) { s.F = ref tValue; } // 1 | ||
| Diagnostic(ErrorCode.ERR_RefAssignNarrower, "s.F = ref tValue").WithArguments("F", "tValue").WithLocation(9, 59), | ||
| // (10,59): error CS9079: Cannot ref-assign 'tRef' to 'F' because 'tRef' can only escape the current method through a return statement. | ||
| // static void AssignRefToValue<T>(S<T> s, ref T tRef) { s.F = ref tRef; } // 14 |
There was a problem hiding this comment.
STE of S<T> s is calling method and RSTE of tRef is return only. so it makes sense that you can't escape tRef by-ref into s.F.
fwiw, I don't think there's any reason to do this assignment in real world code.
| @@ -11312,7 +11315,10 @@ static void Main() | |||
| }"; | |||
|
|
|||
| var comp = CreateCompilation(new[] { code, InterpolatedStringHandlerAttribute, InterpolatedStringHandlerArgumentAttribute }, targetFramework: TargetFramework.NetCoreApp); | |||
| comp.VerifyDiagnostics(); | |||
| comp.VerifyDiagnostics( | |||
| // (5,104): error CS8352: Cannot use variable 'out CustomHandler' in this context because it may expose referenced variables outside of their declaration scope | |||
There was a problem hiding this comment.
would this scenario work if [UnscopedRef] were applied to the method?
There was a problem hiding this comment.
No. The [UnscopedRef] would only apply to the ref portion of this. That doesn't come into play here though as the violation is the following:
s.Handler = this;In this case s has a STE of containing method and this has STE of return only as we model this as an out.
Also note that we specifically disallow [UnscopedRef] on a struct constructor because it would create some really awkward interactions. At that point the STE and RSTE would be equivalent and the ctor could capture refs to itself. So cyclic self assignment in a ctor. I think we could now model that correctly but have it disallowed because it creates really awkward scenarios and I can't see a scenario in which it would be justified. If one was provided I'd be interested in allowing but it won't help this particular issue.
There was a problem hiding this comment.
Ah, of course. STE of this is not customizable and neither is STE of a scoped ref parameter. Thanks!
Though I guess if it were possible to say ref scoped S, you would be able to value-assign this to it. I wonder if that fact is related to why ref scoped was cut? I wasn't following as closely when that happened.
There was a problem hiding this comment.
I wonder if that fact is related to why ref scoped was cut? I wasn't following as closely when that happened.
The initial design only had essentially two scopes: calling method and containing method. A parameter which was ref scoped had it's STS set to containing method. That decision though was wrong and it allowed for easy exploitation as follows:
void M(ref scoped Span<int> parameter)
{
Span<int> local = stackalloc int[42];
parameter = local; // uh oh
}This works because the STS of both values was containing method so the assignment just falls out. At the time we realized the mistake here we still were also looking at other unintended design issues we'd created and cut this on a focus of
Get back to the original intent of just allowing
reffields in current use with minimal additions to language
Now that we've gotten comfortable representing multiple scopes in the design it is possible to model ref scoped. For example it could be done effectively as:
Span<int> M(Span<int> p1, ref scoped Span<int> p2, scoped Span<int> p3)
// Becomes
'a Span<int> M('a Span<inT>, 'b ref 'c Span<int> p2, 'd Span<int> p3)
lifetime 'a >= 'b
lifetime 'c >= 'dThe relationship between 'd and 'c means that you can assign these values to scoped locals but not the other way around. There is very deliberately no relationship because that requires thinking through a bit more.
This also introduces the other problem that we have to deal with: variance. Essentially we have to make sure that in the above design 'b ref 'c Span<int> cannot be assigned to 'b ref 'd Span<int> as that just recreates the original problem. Yet 'c Span<int> can be assigned to 'd Span<int>. That is just assigning a Span<int> with some value to scoped Span<int> which is always safe.
There was a problem hiding this comment.
right..with a 2-scope solution, where STE of ref scoped parameters is current method, it's sort of unsafe by construction. The caller is essentially saying to the callee, "somehow, I am passing you references to your own local variables".
src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
Outdated
Show resolved
Hide resolved
|
Unix failure is known Helix issue with Unix queue. Merging |
dotnet/roslyn#64318 This change allows anything returnable from a method to be assigned to an `out` parameter. In several places had to add `scoped` to `ref` to inform compiler they could not be captured in an `out` parameter.
dotnet/roslyn#64318 This change allows anything returnable from a method to be assigned to an `out` parameter. In several places had to add `scoped` to `ref` to inform compiler they could not be captured in an `out` parameter.
* Patches for scoped locals dotnet/roslyn#64093 This change enforced that `scoped` on a local set the escape scope to the current block where previously it was incorrectly setting to the containing method. * Make return and out equivalent for ref safety dotnet/roslyn#64318 This change allows anything returnable from a method to be assigned to an `out` parameter. In several places had to add `scoped` to `ref` to inform compiler they could not be captured in an `out` parameter. * Warnings on managed pointer types dotnet/roslyn#64294 Compiler now issues warnings for pointer operations involving managed types * Update compiler version * PR feedback * Ref safety rules attribute Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
* Patches for scoped locals dotnet/roslyn#64093 This change enforced that `scoped` on a local set the escape scope to the current block where previously it was incorrectly setting to the containing method. * Make return and out equivalent for ref safety dotnet/roslyn#64318 This change allows anything returnable from a method to be assigned to an `out` parameter. In several places had to add `scoped` to `ref` to inform compiler they could not be captured in an `out` parameter. * Warnings on managed pointer types dotnet/roslyn#64294 Compiler now issues warnings for pointer operations involving managed types * Update compiler version * Fixup * Ref safety rules attribute Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
Implementation side of dotnet/csharplang#6483
This unifies
outparameters with return values with respect to ref safety. A value that can be returned through one can be returned through the other.closes #62094
closes #64155
closes #64365
Note this does not implement having
[UnscopedRef] refon a parameter widen the ref-safe-to-escape. That will be handled as a follow up change. That is strictly non-breaking, hence lower risk and can be considered in ask mode or post RTM.