Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve detection of receiver cloning in ref safety analysis #67736

Merged
merged 29 commits into from
Aug 18, 2023
Merged

Improve detection of receiver cloning in ref safety analysis #67736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
07ea01a
Add a test
jjonescz Apr 11, 2023
d4be178
Fix spelling
jjonescz Apr 11, 2023
558f0c4
Consider ref property accesses to have home
jjonescz Apr 11, 2023
24569b6
Verify IL
jjonescz Apr 11, 2023
b7f1274
Add indexer test
jjonescz Apr 11, 2023
8d74391
Consider ref indexer accesses to have home
jjonescz Apr 11, 2023
42498e4
Add more tests
jjonescz Apr 11, 2023
7fc0cc8
Add readonly tests
jjonescz Apr 14, 2023
9fcbe7c
Revert `Binder.HasHome` changes
jjonescz Apr 14, 2023
8aaf595
Check value kind
jjonescz Apr 14, 2023
53ca90e
Add flag if receiver needs cloning
jjonescz Apr 17, 2023
19c9451
Rename the flag
jjonescz Apr 19, 2023
9546fb0
Use three state flag
jjonescz Apr 19, 2023
307f02d
Fix callers passing wrong flag
jjonescz Apr 19, 2023
864ca32
Fix nullability warning
jjonescz Apr 19, 2023
bd805b2
Expose flag in synthesized BoundCall factory method
jjonescz Apr 20, 2023
59110a4
Expose flag also in Update method
jjonescz Apr 20, 2023
909497c
Fixup EE
jjonescz Apr 20, 2023
32d35ff
Improve static receiver cloning detection logic
jjonescz Apr 20, 2023
5146f2d
Fix nullability warning
jjonescz Apr 20, 2023
860b2c0
Use constant flags where binder is unavailable
jjonescz Apr 21, 2023
d2798a0
Fix the flag in synthesized entry-point
jjonescz Apr 21, 2023
8c724a4
Assert the flag is known in ref safety visitor
jjonescz Apr 21, 2023
58e5fc3
Merge branch 'main' into 67697-UnscopedRef-Property
jjonescz May 3, 2023
af8974d
Merge branch 'main' into 67697-UnscopedRef-Property
jjonescz Aug 2, 2023
6db323e
Merge branch 'main' into 67697-UnscopedRef-Property
jjonescz Aug 10, 2023
fd34df6
Pass `False` for placeholder receiver
jjonescz Aug 18, 2023
a2fde45
Improve test parameter naming
jjonescz Aug 18, 2023
cd54dda
Merge branch 'main' into 67697-UnscopedRef-Property
jjonescz Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 62 additions & 19 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ private BoundAssignmentOperator BindNamedAttributeArgument(AttributeArgumentSynt
var propertySymbol = namedArgumentNameSymbol as PropertySymbol;
if (propertySymbol is object)
{
lvalue = new BoundPropertyAccess(nameSyntax, null, propertySymbol, resultKind, namedArgumentType);
lvalue = new BoundPropertyAccess(nameSyntax, receiverOpt: null, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, propertySymbol, resultKind, namedArgumentType);
}
else
{
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4547,6 +4547,7 @@ private BoundExpression BindConstructorInitializerCore(
return new BoundCall(
nonNullSyntax,
receiver,
initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(receiver, resultMember),
resultMember,
arguments,
analyzedArguments.GetNames(),
Expand Down Expand Up @@ -7976,7 +7977,7 @@ private BoundExpression BindPropertyAccess(
WarnOnAccessOfOffDefault(node, receiver, diagnostics);
}

return new BoundPropertyAccess(node, receiver, propertySymbol, lookupResult, propertySymbol.Type, hasErrors: (hasErrors || hasError));
return new BoundPropertyAccess(node, receiver, initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(receiver, propertySymbol), propertySymbol, lookupResult, propertySymbol.Type, hasErrors: (hasErrors || hasError));
}
#nullable disable

Expand Down Expand Up @@ -9125,6 +9126,7 @@ private BoundExpression BindIndexerOrIndexedPropertyAccess(
propertyAccess = new BoundIndexerAccess(
syntax,
receiver,
initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(receiver, property),
property,
arguments,
argumentNames,
Expand Down
19 changes: 18 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,13 +1189,30 @@ private BoundCall BindInvocationExpressionContinued(
}
}

return new BoundCall(node, receiver, method, args, argNames, argRefKinds, isDelegateCall: isDelegateCall,
return new BoundCall(node, receiver, initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(receiver, method), method, args, argNames, argRefKinds, isDelegateCall: isDelegateCall,
expanded: expanded, invokedAsExtensionMethod: invokedAsExtensionMethod,
argsToParamsOpt: argsToParams, defaultArguments, resultKind: LookupResultKind.Viable, type: returnType, hasErrors: gotError);
}

#nullable enable

internal ThreeState ReceiverIsSubjectToCloning(BoundExpression? receiver, PropertySymbol property)
=> ReceiverIsSubjectToCloning(receiver, property.GetMethod ?? property.SetMethod);

internal ThreeState ReceiverIsSubjectToCloning(BoundExpression? receiver, MethodSymbol method)
{
if (receiver is BoundValuePlaceholderBase || receiver?.Type?.IsValueType != true)
{
return ThreeState.False;
}

var valueKind = method.IsEffectivelyReadOnly
? BindValueKind.RefersToLocation
: BindValueKind.RefersToLocation | BindValueKind.Assignable;
var result = !CheckValueKind(receiver.Syntax, receiver, valueKind, checkingReceiver: true, BindingDiagnosticBag.Discarded);
return result.ToThreeState();
}

private static SourceLocation GetCallerLocation(SyntaxNode syntax)
{
var token = syntax switch
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ private bool BindLengthAndIndexerForListPattern(SyntaxNode node, TypeSymbol inpu
hasErrors |= !TryGetSpecialTypeMember(Compilation, SpecialMember.System_Array__Length, node, diagnostics, out PropertySymbol lengthProperty);
if (lengthProperty is not null)
{
lengthAccess = new BoundPropertyAccess(node, receiverPlaceholder, lengthProperty, LookupResultKind.Viable, lengthProperty.Type) { WasCompilerGenerated = true };
lengthAccess = new BoundPropertyAccess(node, receiverPlaceholder, initialBindingReceiverIsSubjectToCloning: ThreeState.False, lengthProperty, LookupResultKind.Viable, lengthProperty.Type) { WasCompilerGenerated = true };
}
else
{
Expand Down
7 changes: 4 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Query.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ private static BoundCall ReverseLastTwoParameterOrder(BoundCall result)
(defaultArguments[n - 1], defaultArguments[n - 2]) = (defaultArguments[n - 2], defaultArguments[n - 1]);

return result.Update(
result.ReceiverOpt, result.Method, arguments.ToImmutableAndFree(), argumentNamesOpt: default,
result.ReceiverOpt, result.InitialBindingReceiverIsSubjectToCloning, result.Method, arguments.ToImmutableAndFree(), argumentNamesOpt: default,
argumentRefKindsOpt: default, result.IsDelegateCall, result.Expanded, result.InvokedAsExtensionMethod,
argsToParams.ToImmutableAndFree(), defaultArguments, result.ResultKind, result.OriginalMethodsOpt, result.Type);
}
Expand Down Expand Up @@ -469,7 +469,7 @@ private void ReduceJoin(JoinClauseSyntax join, QueryTranslationState state, Bind
var arguments = invocation.Arguments;
arguments = arguments.SetItem(arguments.Length - 1, MakeQueryClause(join.Into, arguments[arguments.Length - 1], g));

invocation = invocation.Update(invocation.ReceiverOpt, invocation.Method, arguments);
invocation = invocation.Update(invocation.ReceiverOpt, invocation.InitialBindingReceiverIsSubjectToCloning, invocation.Method, arguments);
}

state.Clear(); // this completes the whole query
Expand Down Expand Up @@ -538,7 +538,7 @@ private void ReduceJoin(JoinClauseSyntax join, QueryTranslationState state, Bind
var arguments = invocation.Arguments;
arguments = arguments.SetItem(arguments.Length - 1, MakeQueryClause(join.Into, arguments[arguments.Length - 1], g));

invocation = invocation.Update(invocation.ReceiverOpt, invocation.Method, arguments);
invocation = invocation.Update(invocation.ReceiverOpt, invocation.InitialBindingReceiverIsSubjectToCloning, invocation.Method, arguments);
}

state.fromExpression = MakeQueryClause(join, invocation, x2, invocation, castInvocation);
Expand Down Expand Up @@ -628,6 +628,7 @@ private void ReduceFrom(FromClauseSyntax from, QueryTranslationState state, Bind
var arguments = invocation.Arguments;
invocation = invocation.Update(
invocation.ReceiverOpt,
invocation.InitialBindingReceiverIsSubjectToCloning,
invocation.Method,
arguments.SetItem(arguments.Length - 2, MakeQueryClause(from, arguments[arguments.Length - 2], x2, invocation, castInvocation)));

Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3875,6 +3875,7 @@ private static SyntaxToken GetImplicitConstructorBodyToken(CSharpSyntaxNode cont
return new BoundCall(
syntax: syntax,
receiverOpt: receiver,
initialBindingReceiverIsSubjectToCloning: ThreeState.False,
method: baseConstructor,
arguments: ImmutableArray<BoundExpression>.Empty,
argumentNamesOpt: ImmutableArray<string>.Empty,
Expand Down Expand Up @@ -3921,6 +3922,7 @@ private static SyntaxToken GetImplicitConstructorBodyToken(CSharpSyntaxNode cont
return new BoundCall(
syntax: syntax,
receiverOpt: receiver,
initialBindingReceiverIsSubjectToCloning: ThreeState.False,
method: baseConstructor,
arguments: ImmutableArray.Create(argument),
argumentNamesOpt: default,
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,7 @@ private BoundExpression UnwrapCollectionExpressionIfNullable(BoundExpression col
return BoundCall.Synthesized(
syntax: exprSyntax,
receiverOpt: collectionExpr,
initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(collectionExpr, nullableValueGetter),
method: nullableValueGetter);
}
else
Expand Down
14 changes: 14 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,7 @@ private void VisitArgumentsAndGetArgumentPlaceholders(BoundExpression? receiverO

protected override void VisitArguments(BoundCall node)
{
Debug.Assert(node.InitialBindingReceiverIsSubjectToCloning != ThreeState.Unknown);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's def a part of me that feels like, man, instead of this flag I wish we could just wrap the receiver expression into the BoundCapturedReceiverPlaceholder node at initial binding time. After all, what we are doing in ref safety analysis is simply to introduce this node late into the tree and then analyze that.

I know that change has the tendency for unpleasant ripple effects through other areas of the compiler. Flow analysis does already have mechanisms for "seeing through" placeholders and so on, but they can be unpleasant to use, and I'm sure more than that would be impacted. Also, I think we've solved the problem with the current change and I don't want to belabor that unnecessarily.

Copy link
Member Author

@jjonescz jjonescz Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea, I haven't tried that, but I can imagine it could be an even more invasive change.

VisitArgumentsAndGetArgumentPlaceholders(node.ReceiverOpt, node.Arguments);

if (!node.HasErrors)
Expand All @@ -643,6 +644,7 @@ protected override void VisitArguments(BoundCall node)
node.Syntax,
method,
node.ReceiverOpt,
node.InitialBindingReceiverIsSubjectToCloning,
method.Parameters,
node.Arguments,
node.ArgumentRefKindsOpt,
Expand Down Expand Up @@ -748,6 +750,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
node.Syntax,
constructor,
receiverOpt: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
constructor.Parameters,
node.Arguments,
node.ArgumentRefKindsOpt,
Expand All @@ -758,8 +761,15 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
}
}

public override BoundNode? VisitPropertyAccess(BoundPropertyAccess node)
{
Debug.Assert(node.InitialBindingReceiverIsSubjectToCloning != ThreeState.Unknown);
return base.VisitPropertyAccess(node);
}

public override BoundNode? VisitIndexerAccess(BoundIndexerAccess node)
{
Debug.Assert(node.InitialBindingReceiverIsSubjectToCloning != ThreeState.Unknown);
Visit(node.ReceiverOpt);
VisitArgumentsAndGetArgumentPlaceholders(node.ReceiverOpt, node.Arguments);

Expand All @@ -770,6 +780,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
node.Syntax,
indexer,
node.ReceiverOpt,
node.InitialBindingReceiverIsSubjectToCloning,
indexer.Parameters,
node.Arguments,
node.ArgumentRefKindsOpt,
Expand All @@ -792,6 +803,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
node.Syntax,
method,
receiverOpt: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
method.Parameters,
node.Arguments,
node.ArgumentRefKindsOpt,
Expand Down Expand Up @@ -892,6 +904,7 @@ private void VisitDeconstructionArguments(ArrayBuilder<DeconstructionVariable> v
syntax,
deconstructMethod,
invocation.ReceiverOpt,
invocation.InitialBindingReceiverIsSubjectToCloning,
parameters,
invocation.Arguments,
invocation.ArgumentRefKindsOpt,
Expand Down Expand Up @@ -982,6 +995,7 @@ private static ImmutableArray<BoundExpression> GetDeconstructionRightParts(Bound
collectionEscape = GetInvocationEscapeScope(
equivalentSignatureMethod,
receiver: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
equivalentSignatureMethod.Parameters,
arguments,
refKinds,
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
<ValueType Name="ConversionKind"/>
<ValueType Name="Conversion"/>
<ValueType Name="TextSpan"/>
<ValueType Name="ThreeState"/>
<ValueType Name="UnaryOperatorKind"/>
<ValueType Name="BinaryOperatorKind"/>
<ValueType Name="LookupResultKind"/>
Expand Down Expand Up @@ -1786,6 +1787,8 @@
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>

<Field Name="ReceiverOpt" Type="BoundExpression?"/>
<!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
<Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
<Field Name="Method" Type="MethodSymbol"/>
<Field Name="Arguments" Type="ImmutableArray&lt;BoundExpression&gt;"/>
<Field Name="ArgumentNamesOpt" Type="ImmutableArray&lt;string&gt;" Null="allow"/>
Expand Down Expand Up @@ -2142,6 +2145,8 @@
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>

<Field Name="ReceiverOpt" Type="BoundExpression?"/>
<!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
<Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
<Field Name="PropertySymbol" Type="PropertySymbol"/>
<Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
</Node>
Expand All @@ -2161,6 +2166,8 @@
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>

<Field Name="ReceiverOpt" Type="BoundExpression?"/>
<!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
<Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
<Field Name="Indexer" Type="PropertySymbol"/>
<Field Name="Arguments" Type="ImmutableArray&lt;BoundExpression&gt;"/>
<Field Name="ArgumentNamesOpt" Type="ImmutableArray&lt;string&gt;" Null="allow"/>
Expand Down
Loading