Handle deconstruct pattern in ref safety analysis #75511
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jjonescz
force-pushed
the
75484-RefSafety-Deconstruct
branch
from
cd7baee
to
769f731
Compare
|
@dotnet/roslyn-compiler for reviews, thanks |
| // (13,20): error CS8352: Cannot use variable 'x2' in this context because it may expose referenced variables outside of their declaration scope | ||
| // return x2; | ||
| Diagnostic(ErrorCode.ERR_EscapeVariable, "x2").WithArguments("x2").WithLocation(13, 20)); | ||
| } |
There was a problem hiding this comment.
nit: Consider also testing a nested deconstruction scenario: ((var x, var y), var z). I expect it should just fall out by composition
|
@cston for second review. Thanks |
| var placeholder = new BoundDeconstructValuePlaceholder( | ||
| x.Syntax, | ||
| variableSymbol: null, | ||
| isDiscardExpression: true, |
There was a problem hiding this comment.
I didn't understand why we should pass 'isDiscardExpression: true' here.
There was a problem hiding this comment.
Originally this code path was written to handle discards in the pattern which correspond to out _ discard arguments of the equivalent Deconstruct call.
But now I realize there can be also nested subpatterns and we need to be smarter about extracting all variable accesses from that.
There was a problem hiding this comment.
I think MAMM should not have an effect on the underlying Deconstruct calls of patterns. So I have removed the code which was trying to check invocation ref safety of an equivalent Deconstruct call (it wouldn't be so easy to do because one argument to the call could correspond to multiple variables in case of binary patterns).
I also added more tests where the Deconstruct method used by the pattern has the receiver scoped. Turned out that didn't work - the using (new PatternInput(this, _localScopeDepth)) caused all the pattern ref-struct vars to have val escape scope of _localScopeDepth. Per ref safety rules of Deconstruct(out var ...) like calls, ref-struct out vars should have that scope only if the receiver is unscoped. So I added a condition to that effect.
|
@jcouv @RikkiGibson for another look, thanks |
| { | ||
| R M() | ||
| { | ||
| (var x, _) = new S(); |
There was a problem hiding this comment.
It feels like we should also have tests where the Deconstruct receiver is something we can return a reference to. For example, if it is a by-ref parameter.
There was a problem hiding this comment.
Thanks, that actually doesn't work as expected for normal deconstruction (as opposed to a pattern matching one) which seems like an orthogonal issue (this PR fixes deconstruction in pattern matching). Filed #75850.
|
@dotnet/roslyn-compiler for reviews, thanks |
Fixes #75484.