-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Do not copy nullable value type receiver of a constrained call. #66311
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -316,6 +316,10 @@ private void EmitExpressionCore(BoundExpression expression, bool used) | |
| EmitComplexConditionalReceiver((BoundComplexConditionalReceiver)expression, used); | ||
| break; | ||
|
|
||
| case BoundKind.ComplexReceiver: | ||
| EmitComplexReceiver((BoundComplexReceiver)expression, used); | ||
| break; | ||
|
|
||
| case BoundKind.PseudoVariable: | ||
| EmitPseudoVariableValue((BoundPseudoVariable)expression, used); | ||
| break; | ||
|
|
@@ -365,7 +369,11 @@ private void EmitComplexConditionalReceiver(BoundComplexConditionalReceiver expr | |
|
|
||
| EmitExpression(expression.ReferenceTypeReceiver, used); | ||
| _builder.EmitBranch(ILOpCode.Br, doneLabel); | ||
| _builder.AdjustStack(-1); | ||
|
|
||
| if (used) | ||
| { | ||
| _builder.AdjustStack(-1); | ||
| } | ||
|
|
||
| _builder.MarkLabel(whenValueTypeLabel); | ||
| EmitExpression(expression.ValueTypeReceiver, used); | ||
|
|
@@ -412,7 +420,8 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces | |
| ((TypeParameterSymbol)receiverType).EffectiveInterfacesNoUseSiteDiagnostics.IsEmpty) || // This could be a nullable value type, which must be copied in order to not mutate the original value | ||
| LocalRewriter.CanChangeValueBetweenReads(receiver, localsMayBeAssignedOrCaptured: false) || | ||
| (receiverType.IsReferenceType && receiverType.TypeKind == TypeKind.TypeParameter) || | ||
| (receiver.Kind == BoundKind.Local && IsStackLocal(((BoundLocal)receiver).LocalSymbol)); | ||
| (receiver.Kind == BoundKind.Local && IsStackLocal(((BoundLocal)receiver).LocalSymbol)) || | ||
| (notConstrained && IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker.Analyze(expression)); | ||
|
|
||
| // ===== RECEIVER | ||
| if (nullCheckOnCopy) | ||
|
|
@@ -564,6 +573,60 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces | |
| } | ||
| } | ||
|
|
||
| private sealed class IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator | ||
| { | ||
| private readonly BoundLoweredConditionalAccess _conditionalAccess; | ||
| private bool? _result; | ||
|
|
||
| private IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker(BoundLoweredConditionalAccess conditionalAccess) | ||
| : base() | ||
| { | ||
| _conditionalAccess = conditionalAccess; | ||
| } | ||
|
|
||
| public static bool Analyze(BoundLoweredConditionalAccess conditionalAccess) | ||
| { | ||
| var walker = new IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker(conditionalAccess); | ||
| walker.Visit(conditionalAccess.WhenNotNull); | ||
| Debug.Assert(walker._result.HasValue); | ||
| return walker._result.GetValueOrDefault(); | ||
| } | ||
|
|
||
| public override BoundNode Visit(BoundNode node) | ||
| { | ||
| if (_result.HasValue) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return base.Visit(node); | ||
| } | ||
|
|
||
| public override BoundNode VisitCall(BoundCall node) | ||
| { | ||
| if (node.ReceiverOpt is BoundConditionalReceiver { Id: var id } && id == _conditionalAccess.Id) | ||
| { | ||
| Debug.Assert(!_result.HasValue); | ||
| _result = !IsSafeToDereferenceReceiverRefAfterEvaluatingArguments(node.Arguments); | ||
| return null; | ||
| } | ||
|
|
||
| return base.VisitCall(node); | ||
| } | ||
|
|
||
| public override BoundNode VisitConditionalReceiver(BoundConditionalReceiver node) | ||
| { | ||
| if (node.Id == _conditionalAccess.Id) | ||
| { | ||
| Debug.Assert(!_result.HasValue); | ||
| _result = false; | ||
| return null; | ||
| } | ||
|
|
||
| return base.VisitConditionalReceiver(node); | ||
| } | ||
| } | ||
|
|
||
| private void EmitConditionalReceiver(BoundConditionalReceiver expression, bool used) | ||
| { | ||
| Debug.Assert(!expression.Type.IsValueType); | ||
|
|
@@ -1752,25 +1815,21 @@ LocalDefinition emitGenericReceiver(BoundCall call, out CallKind callKind) | |
| // reference to the stack. So, for a class we need to emit a reference to a temporary | ||
| // location, rather than to the original location | ||
|
|
||
| // Struct values are never nulls. | ||
| // We will emit a check for such case, but the check is really a JIT-time | ||
| // We will emit a runtime check for a class, but the check is really a JIT-time | ||
| // constant since JIT will know if T is a struct or not. | ||
|
|
||
| // if ((object)default(T) == null) | ||
| // if (!typeof(T).IsValueType) | ||
| // { | ||
| // temp = receiverRef | ||
| // receiverRef = ref temp | ||
| // } | ||
|
|
||
| object whenNotNullLabel = null; | ||
| object whenValueTypeLabel = null; | ||
|
|
||
| if (!receiverType.IsReferenceType) | ||
| { | ||
| // if ((object)default(T) == null) | ||
| EmitDefaultValue(receiverType, true, receiver.Syntax); | ||
| EmitBox(receiverType, receiver.Syntax); | ||
| whenNotNullLabel = new object(); | ||
| _builder.EmitBranch(ILOpCode.Brtrue, whenNotNullLabel); | ||
| // if (!typeof(T).IsValueType) | ||
| whenValueTypeLabel = TryEmitIsValueTypeBranch(receiverType, receiver.Syntax); | ||
| } | ||
|
|
||
| // temp = receiverRef | ||
|
|
@@ -1780,16 +1839,91 @@ LocalDefinition emitGenericReceiver(BoundCall call, out CallKind callKind) | |
| _builder.EmitLocalStore(tempOpt); | ||
| _builder.EmitLocalAddress(tempOpt); | ||
|
|
||
| if (whenNotNullLabel is not null) | ||
| if (whenValueTypeLabel is not null) | ||
| { | ||
| _builder.MarkLabel(whenNotNullLabel); | ||
| _builder.MarkLabel(whenValueTypeLabel); | ||
| } | ||
| } | ||
|
|
||
| return tempOpt; | ||
| } | ||
| } | ||
|
|
||
| private object TryEmitIsValueTypeBranch(TypeSymbol type, SyntaxNode syntax) | ||
| { | ||
| if (Binder.GetWellKnownTypeMember(_module.Compilation, WellKnownMember.System_Type__GetTypeFromHandle, _diagnostics, syntax: syntax, isOptional: false) is MethodSymbol getTypeFromHandle && | ||
| Binder.GetWellKnownTypeMember(_module.Compilation, WellKnownMember.System_Type__get_IsValueType, _diagnostics, syntax: syntax, isOptional: false) is MethodSymbol getIsValueType) | ||
| { | ||
| _builder.EmitOpCode(ILOpCode.Ldtoken); | ||
| EmitSymbolToken(type, syntax); | ||
| _builder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); // argument off, return value on | ||
| EmitSymbolToken(getTypeFromHandle, syntax, null); | ||
| _builder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); // instance off, return value on | ||
| EmitSymbolToken(getIsValueType, syntax, null); | ||
| var whenValueTypeLabel = new object(); | ||
| _builder.EmitBranch(ILOpCode.Brtrue, whenValueTypeLabel); | ||
|
|
||
| return whenValueTypeLabel; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private void EmitComplexReceiver(BoundComplexReceiver expression, bool used) | ||
| { | ||
| Debug.Assert(!used); | ||
|
Member
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. nit: What's the benefit of only asserting? Throwing seems better.
Contributor
Author
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 implementation is doing the right thing. I do not see any point in throwing. The assert will help us identify the test scenario. |
||
| Debug.Assert(!expression.Type.IsReferenceType); | ||
| Debug.Assert(!expression.Type.IsValueType); | ||
|
|
||
| var receiverType = expression.Type; | ||
|
|
||
| // if (!typeof(T).IsValueType) | ||
| object whenValueTypeLabel = TryEmitIsValueTypeBranch(receiverType, expression.Syntax); | ||
|
|
||
| EmitExpression(expression.ReferenceTypeReceiver, used); | ||
| var doneLabel = new object(); | ||
| _builder.EmitBranch(ILOpCode.Br, doneLabel); | ||
|
|
||
| if (whenValueTypeLabel is not null) | ||
| { | ||
| if (used) | ||
| { | ||
| _builder.AdjustStack(-1); | ||
| } | ||
|
|
||
| _builder.MarkLabel(whenValueTypeLabel); | ||
| EmitExpression(expression.ValueTypeReceiver, used); | ||
| } | ||
|
|
||
| _builder.MarkLabel(doneLabel); | ||
| } | ||
|
|
||
| private void EmitComplexReceiverAddress(BoundComplexReceiver expression) | ||
| { | ||
| Debug.Assert(!expression.Type.IsReferenceType); | ||
| Debug.Assert(!expression.Type.IsValueType); | ||
|
|
||
| var receiverType = expression.Type; | ||
|
|
||
| // if (!typeof(T).IsValueType) | ||
| object whenValueTypeLabel = TryEmitIsValueTypeBranch(receiverType, expression.Syntax); | ||
|
|
||
| var receiverTemp = EmitAddress(expression.ReferenceTypeReceiver, AddressKind.ReadOnly); | ||
| Debug.Assert(receiverTemp == null); | ||
| var doneLabel = new Object(); | ||
| _builder.EmitBranch(ILOpCode.Br, doneLabel); | ||
|
|
||
| if (whenValueTypeLabel is not null) | ||
| { | ||
| _builder.AdjustStack(-1); | ||
| _builder.MarkLabel(whenValueTypeLabel); | ||
| // we will not write through this receiver, but it could be a target of mutating calls | ||
| EmitReceiverRef(expression.ValueTypeReceiver, AddressKind.Constrained); | ||
| } | ||
|
|
||
| _builder.MarkLabel(doneLabel); | ||
| } | ||
|
|
||
| internal static bool IsPossibleReferenceTypeReceiverOfConstrainedCall(BoundExpression receiver) | ||
| { | ||
| var receiverType = receiver.Type; | ||
|
|
@@ -1811,7 +1945,9 @@ internal static bool ReceiverIsKnownToReferToTempIfReferenceType(BoundExpression | |
|
|
||
| if (receiver is | ||
| BoundLocal { LocalSymbol.IsKnownToReferToTempIfReferenceType: true } or | ||
| BoundComplexConditionalReceiver) | ||
| BoundComplexConditionalReceiver or | ||
| BoundComplexReceiver or | ||
| BoundConditionalReceiver { Type: { IsReferenceType: false, IsValueType: false } }) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2940,6 +2940,20 @@ public override BoundNode VisitComplexConditionalReceiver(BoundComplexConditiona | |
| return null; | ||
| } | ||
|
|
||
| public override BoundNode VisitComplexReceiver(BoundComplexReceiver node) | ||
|
Member
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. Are any flow passes actually going to see one of these? It looks like they're only created during local lowering, so I wouldn't expected them to be seen. #Resolved
Contributor
Author
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.
This code path is reachable. Flow analysis is done during async rewrite, etc. |
||
| { | ||
| var savedState = this.State.Clone(); | ||
|
|
||
| VisitRvalue(node.ValueTypeReceiver); | ||
| Join(ref this.State, ref savedState); | ||
|
|
||
| savedState = this.State.Clone(); | ||
| VisitRvalue(node.ReferenceTypeReceiver); | ||
| Join(ref this.State, ref savedState); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| public override BoundNode VisitSequence(BoundSequence node) | ||
| { | ||
| var sideEffects = node.SideEffects; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Can we reach this? I thought this node would only be introduced in a later phase (but not during initial binding). #Resolved
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.
The same goes for
BoundKind.ComplexConditionalReceiver. Just follow the pattern.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.
Why not throw unreachable exception since this is unreachable? I'd do that for
ComplexConditionalReceivertooThere 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.
I am comfortable going as is