-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for ref-readonly locals #22269
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,13 +40,11 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr | |
break; | ||
|
||
case BoundKind.Local: | ||
EmitLocalAddress((BoundLocal)expression); | ||
break; | ||
return EmitLocalAddress((BoundLocal)expression, addressKind); | ||
|
||
case BoundKind.Dup: | ||
Debug.Assert(((BoundDup)expression).RefKind != RefKind.None, "taking address of a stack value?"); | ||
_builder.EmitOpCode(ILOpCode.Dup); | ||
break; | ||
return EmitDupAddress((BoundDup)expression, addressKind); | ||
|
||
case BoundKind.ConditionalReceiver: | ||
// do nothing receiver ref must be already pushed | ||
|
@@ -213,10 +211,18 @@ private void EmitComplexConditionalReceiverAddress(BoundComplexConditionalReceiv | |
_builder.MarkLabel(doneLabel); | ||
} | ||
|
||
private void EmitLocalAddress(BoundLocal localAccess) | ||
/// <summary> | ||
/// May introduce a temp which it will return. (otherwise returns null) | ||
/// </summary> | ||
private LocalDefinition EmitLocalAddress(BoundLocal localAccess, AddressKind addressKind) | ||
{ | ||
var local = localAccess.LocalSymbol; | ||
|
||
if (!HasHome(localAccess, addressKind != AddressKind.ReadOnly)) | ||
{ | ||
return EmitAddressOfTempClone(localAccess); | ||
} | ||
|
||
if (IsStackLocal(local)) | ||
{ | ||
if (local.RefKind != RefKind.None) | ||
|
@@ -234,6 +240,21 @@ private void EmitLocalAddress(BoundLocal localAccess) | |
{ | ||
_builder.EmitLocalAddress(GetLocal(localAccess)); | ||
} | ||
return null; | ||
} | ||
|
||
/// <summary> | ||
/// May introduce a temp which it will return. (otherwise returns null) | ||
/// </summary> | ||
private LocalDefinition EmitDupAddress(BoundDup dup, AddressKind addressKind) | ||
{ | ||
if (!HasHome(dup, addressKind != AddressKind.ReadOnly)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a named argument for the second parm here. Makes it more readable. |
||
{ | ||
return EmitAddressOfTempClone(dup); | ||
} | ||
|
||
_builder.EmitOpCode(ILOpCode.Dup); | ||
return null; | ||
} | ||
|
||
private void EmitPseudoVariableAddress(BoundPseudoVariable expression) | ||
|
@@ -345,9 +366,11 @@ private bool HasHome(BoundExpression expression, bool needWriteable) | |
((BoundParameter)expression).ParameterSymbol.RefKind != RefKind.RefReadOnly; | ||
|
||
case BoundKind.Local: | ||
// locals have home unless they are byval stack locals | ||
// locals have home unless they are byval stack locals or ref-readonly | ||
// locals in a mutating call | ||
var local = ((BoundLocal)expression).LocalSymbol; | ||
return !IsStackLocal(local) || local.RefKind != RefKind.None; | ||
return !((IsStackLocal(local) && local.RefKind == RefKind.None) || | ||
(needWriteable && local.RefKind == RefKind.RefReadOnly)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is needWritable protecting the local or the value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The local. It's specifying that you must be able to write to the local pointed to by the address. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then why is the last condition only checking ref readonly In reply to: 140599487 [](ancestors = 140599487) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
case BoundKind.Call: | ||
var methodRefKind = ((BoundCall)expression).Method.RefKind; | ||
|
@@ -356,9 +379,9 @@ private bool HasHome(BoundExpression expression, bool needWriteable) | |
|
||
case BoundKind.Dup: | ||
//NB: Dup represents locals that do not need IL slot | ||
// ref locals are currently always writeable, so we do not need to care about "needWriteable" | ||
Debug.Assert(((BoundDup)expression).RefKind != RefKind.RefReadOnly); | ||
return ((BoundDup)expression).RefKind != RefKind.None; | ||
var dupRefKind = ((BoundDup)expression).RefKind; | ||
return dupRefKind == RefKind.Ref || | ||
(!needWriteable && dupRefKind == RefKind.RefReadOnly); | ||
|
||
case BoundKind.FieldAccess: | ||
return HasHome((BoundFieldAccess)expression, needWriteable); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -971,7 +971,23 @@ private static bool IsIndirectAssignment(BoundAssignmentOperator node) | |
{ | ||
var lhs = node.Left; | ||
|
||
Debug.Assert(node.RefKind == RefKind.None || (lhs as BoundLocal)?.LocalSymbol.RefKind == RefKind.Ref, | ||
bool IsAssignable(RefKind lhsKind, RefKind rhsKind) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if this is only for asserting, I'd suggest inlining into the Assert call and simply returning false otherwise. Throwing here will not trigger the Debug window while debugging, and will route it to the VS fault handler. |
||
{ | ||
switch (lhsKind) | ||
{ | ||
case RefKind.None: | ||
case RefKind.Ref: | ||
return lhsKind == rhsKind; | ||
case RefKind.RefReadOnly: | ||
return rhsKind == RefKind.RefReadOnly || rhsKind == RefKind.Ref; | ||
default: | ||
throw ExceptionUtilities.UnexpectedValue(lhsKind); | ||
} | ||
} | ||
|
||
Debug.Assert(node.RefKind == RefKind.None || | ||
lhs is BoundLocal local && | ||
IsAssignable(local.LocalSymbol.RefKind, node.RefKind), | ||
"only ref locals can be a target of a ref assignment"); | ||
|
||
switch (lhs.Kind) | ||
|
@@ -1034,6 +1050,7 @@ private static bool IsIndirectAssignment(BoundAssignmentOperator node) | |
throw ExceptionUtilities.UnexpectedValue(lhs.Kind); | ||
} | ||
} | ||
|
||
private static bool IsIndirectOrInstanceFieldAssignment(BoundAssignmentOperator node) | ||
{ | ||
var lhs = node.Left; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,7 +268,7 @@ internal virtual bool IsWritable | |
case LocalDeclarationKind.UsingVariable: | ||
return false; | ||
default: | ||
return true; | ||
return RefKind != RefKind.RefReadOnly; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The local is still assignable. Is that not what is being tracked here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what do you expect to happen with the following code? int x = 0;
ref readonly int xr = ref x;
xr++; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we explicitly state the true cases, and throw otherwise? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why you would want to ever throw here. |
||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line