Set val escape on expression variables #64414
Conversation
ghost
added
the
src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs
Outdated
Show resolved
Hide resolved
| BoundLocal { DeclarationKind: not BoundLocalDeclarationKind.None } l => l.ExpressionSymbol, | ||
| _ => null | ||
| }; | ||
| if (symbol is SourceLocalSymbol { ValEscapeScope: CallingMethodScope } local) |
There was a problem hiding this comment.
note that we really would prefer to use a sentinel ValEscapeScope in cases like these (e.g. with value 1 << 32). I attempted to do this but it started to really churn and wanted to be expedient for the purposes of getting this fix in.
| // e.g. in `dest = M(ref arg)`, we assume `ref arg` will not escape into `dest`, but `arg` might. | ||
| inferredDestinationValEscape = Math.Max(inferredDestinationValEscape, GetValEscape(argument, scopeOfTheContainingExpression)); | ||
| var valid = CheckValEscape(argument.Syntax, argument, scopeOfTheContainingExpression, escapeTo, false, diagnostics); | ||
|
|
There was a problem hiding this comment.
| && isMixableParameter(parameter) | ||
| // assume any expression variable is a valid mixing destination, | ||
| // since we will infer a legal val-escape for it (if it doesn't already have a narrower one). | ||
| && !ShouldInferDeclarationExpressionValEscape(argument, out _)) |
There was a problem hiding this comment.
I don't think ShouldInferDeclarationExpressionValEscape is the best call here. Consider in the future if we actually did the change where we checked if it had been previously inferred. At that point I would expect Should to return false as we've already inferred the value.
Wouldn't it be more correct to change isMixableParameter to just return false for any declaration expression? Those are not mixable parameters because they're completely inferred.
There was a problem hiding this comment.
I thought this through a bit, and yeah, I don't think there are any scenarios where a declaration variable needs to be checked as a mixing destination, since either:
- it is an out var at the top level of a script, therefore is a class field, therefore its type will never be ref struct
- it is a
out scoped varin which case the val-escape is the narrowest which is even permitted to be referenced at that location. - it is a regular
out varin which case the widest legal val-escape is inferred for it.
| foreach (var (_, argument, _) in escapeArguments) | ||
| { | ||
| if (ShouldInferDeclarationExpressionValEscape(argument, out var localSymbol)) | ||
| { | ||
| localSymbol.SetValEscape(inferredDestinationValEscape); | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is the same between new and updated rules. Should it be factored otu?
There was a problem hiding this comment.
I think it is just beneath the bar of factoring out.
| static RS M2(ref RS rs5) | ||
| { | ||
| // RSTE of rs3 is ReturnOnly. |
| { | ||
| foreach (var (_, argument, refKind) in escapeArguments) | ||
| { | ||
| if (ShouldInferDeclarationExpressionValEscape(argument, out _)) |
There was a problem hiding this comment.
I think we also wanted to change this to use 'isMixableArgument'.
I think the behavior/factoring for the current release is going to be fine, we just want to make sure to iron these kinds of things out when we refactor in 17.5.
This reverts commit 2ca2359.
Revert: Revert "Set val escape on expression variables (#64414)" Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
Revert: Revert "Set val escape on expression variables (dotnet#64414)" Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
Set val escape on expression variables (#64414)
3 participants
Closes #64394
Closes #22361