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

Check ref safety of arg mixing in collection initializers #76237

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Check ref safety of arg mixing in collection initializers #76237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e9d7a35
Check ref safety of arg mixing in collection initializers
jjonescz Dec 3, 2024
333529c
Simplify the implementation
jjonescz Dec 4, 2024
55fb093
Extend tests
jjonescz Dec 10, 2024
a80efd9
Fix missed warning
jjonescz Dec 11, 2024
461ab15
Extend tests
jjonescz Dec 11, 2024
f350350
Clarify a condition
jjonescz Dec 11, 2024
7dbbe97
Simplify implementation
jjonescz Dec 12, 2024
601b822
Allow the receiver to affect the scope
jjonescz Dec 12, 2024
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
137 changes: 109 additions & 28 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4454,12 +4454,52 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext scopeOfTheCo
return GetValEscapeOfObjectInitializer(initExpr, scopeOfTheContainingExpression);

case BoundKind.CollectionInitializerExpression:
var colExpr = (BoundCollectionInitializerExpression)expr;
return GetValEscape(colExpr.Initializers, scopeOfTheContainingExpression);
{
var colExpr = (BoundCollectionInitializerExpression)expr;

// var c = new C() { element };
//
// is equivalent to
//
// var c = new C();
// c.Add(element);
//
// So we check arg mixing of `(receiverPlaceholder).Add(element)` calls,
// and make the result "scoped" if any call could cause the elements to escape.

using var _ = new PlaceholderRegion(this, [(colExpr.Placeholder, SafeContext.CallingMethod)]) { ForceRemoveOnDispose = true };
Copy link
Member Author

Choose a reason for hiding this comment

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

Added ForceRemoveOnDispose = true because otherwise it can happen that the placeholder is added twice causing an assert to fail.

Copy link
Member Author

@jjonescz jjonescz Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This might need to be extended to also handle the assert in the Add part, like I'm doing in a similar PR (#76263): b64ee87

But the bigger question is whether this looks like a good approach in general - using PlaceholderRegion temporarily during the check, etc.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like adding a PlaceholderRegion temporarily here works, and it is unfortunate that PlaceholderRegions are typically added during the Visit methods and not removed. I'm curious though: was there a reason it was necessary to add the PlaceholderRegion temporarily in this particular case rather than adding it once (and not removing), in the calling VisitObjectCreationExpressionBase()?

Copy link
Member Author

Choose a reason for hiding this comment

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

was there a reason it was necessary to add the PlaceholderRegion temporarily in this particular case rather than adding it once (and not removing), in the calling VisitObjectCreationExpressionBase()?

Yes. When GetValEscape is called on a collection initializer, I need to decide whether it can be returnable or must be scoped to current block. So I temporarily mark the receiver with CallingMethod scope to see if that is ref safe - but it might not be ref safe (and depending on that, I determine the ValEscape of the collection initializer). Put another way, the receiver scope is just speculative and hence I don't think it should be set permanently in the visitor.

foreach (var initializer in colExpr.Initializers)
{
switch (initializer)
{
case BoundCollectionElementInitializer init:
if (!CheckInvocationArgMixing(
init.Syntax,
MethodInfo.Create(init.AddMethod),
receiverOpt: colExpr.Placeholder,
receiverIsSubjectToCloning: init.InitialBindingReceiverIsSubjectToCloning,
parameters: init.AddMethod.Parameters,
argsOpt: init.Arguments,
argRefKindsOpt: default,
argsToParamsOpt: init.ArgsToParamsOpt,
scopeOfTheContainingExpression: scopeOfTheContainingExpression,
BindingDiagnosticBag.Discarded))
{
return scopeOfTheContainingExpression;
}
break;

case BoundDynamicCollectionElementInitializer dynamicInit:
break;

default:
Debug.Fail($"{initializer.Kind} expression of {initializer.Type} type");
break;
}
}

case BoundKind.CollectionElementInitializer:
var colElement = (BoundCollectionElementInitializer)expr;
return GetValEscape(colElement.Arguments, scopeOfTheContainingExpression);
return SafeContext.CallingMethod;
}

case BoundKind.ObjectInitializerMember:
// this node generally makes no sense outside of the context of containing initializer
Expand All @@ -4468,11 +4508,18 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext scopeOfTheCo
return scopeOfTheContainingExpression;

case BoundKind.ImplicitReceiver:
case BoundKind.ObjectOrCollectionValuePlaceholder:
// binder uses this as a placeholder when binding members inside an object initializer
// just say it does not escape anywhere, so that we do not get false errors.
return scopeOfTheContainingExpression;

case BoundKind.ObjectOrCollectionValuePlaceholder:
if (_placeholderScopes?.TryGetValue((BoundObjectOrCollectionValuePlaceholder)expr, out var scope) == true)
{
return scope;
}

return scopeOfTheContainingExpression;

case BoundKind.InterpolatedStringHandlerPlaceholder:
// The handler placeholder cannot escape out of the current expression, as it's a compiler-synthesized
// location.
Expand Down Expand Up @@ -4774,6 +4821,18 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, SafeContext
}
return true;

case BoundKind.ObjectOrCollectionValuePlaceholder:
if (_placeholderScopes?.TryGetValue((BoundObjectOrCollectionValuePlaceholder)expr, out var scope) == true)
{
if (!scope.IsConvertibleTo(escapeTo))
{
Error(diagnostics, inUnsafeRegion ? ErrorCode.WRN_EscapeVariable : ErrorCode.ERR_EscapeVariable, node, expr.Syntax);
Copy link
Member

Choose a reason for hiding this comment

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

inUnsafeRegion ? ErrorCode.WRN_EscapeVariable

Are we testing the unsafe { } region case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added unsafe test. I'm not sure the WRN_EscapeVariable here is reachable (at least in the tested scenarios, this is used with discarded diagnostics just for the return value), but at least it's consistent with other similar places (and I imagine it might be reachable with more complex nested scenarios).

return inUnsafeRegion;
}
return true;
}
goto default;

case BoundKind.Local:
var localSymbol = ((BoundLocal)expr).LocalSymbol;
if (!GetLocalScopes(localSymbol).ValEscapeScope.IsConvertibleTo(escapeTo))
Expand Down Expand Up @@ -5257,17 +5316,52 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, SafeContext
var initExpr = (BoundObjectInitializerExpression)expr;
return CheckValEscapeOfObjectInitializer(initExpr, escapeFrom, escapeTo, diagnostics);

// this would be correct implementation for CollectionInitializerExpression
// however it is unclear if it is reachable since the initialized type must implement IEnumerable
case BoundKind.CollectionInitializerExpression:
var colExpr = (BoundCollectionInitializerExpression)expr;
return CheckValEscape(colExpr.Initializers, escapeFrom, escapeTo, diagnostics);
{
var colExpr = (BoundCollectionInitializerExpression)expr;

// return new C() { element };
//
// is equivalent to
//
// var c = new C();
// c.Add(element);
// return c;
//
// So we check arg mixing of `(receiverPlaceholder).Add(element)` calls
// where the placeholder has `escapeTo` scope and the call has `escapeFrom` scope.

bool result = true;
using var _ = new PlaceholderRegion(this, [(colExpr.Placeholder, escapeTo)]) { ForceRemoveOnDispose = true };
foreach (var initializer in colExpr.Initializers)
{
switch (initializer)
{
case BoundCollectionElementInitializer init:
result |= CheckInvocationArgMixing(
init.Syntax,
MethodInfo.Create(init.AddMethod),
receiverOpt: colExpr.Placeholder,
receiverIsSubjectToCloning: init.InitialBindingReceiverIsSubjectToCloning,
parameters: init.AddMethod.Parameters,
argsOpt: init.Arguments,
argRefKindsOpt: default,
argsToParamsOpt: init.ArgsToParamsOpt,
scopeOfTheContainingExpression: escapeFrom,
diagnostics);
break;

case BoundDynamicCollectionElementInitializer dynamicInit:
break;

default:
Debug.Fail($"{initializer.Kind} expression of {initializer.Type} type");
break;
}
}

// this would be correct implementation for CollectionElementInitializer
// however it is unclear if it is reachable since the initialized type must implement IEnumerable
case BoundKind.CollectionElementInitializer:
var colElement = (BoundCollectionElementInitializer)expr;
return CheckValEscape(colElement.Arguments, escapeFrom, escapeTo, diagnostics);
return result;
}

case BoundKind.PointerElementAccess:
var accessedExpression = ((BoundPointerElementAccess)expr).Expression;
Expand Down Expand Up @@ -5563,19 +5657,6 @@ private bool CheckValEscapeOfObjectInitializer(BoundObjectInitializerExpression

#nullable disable

private bool CheckValEscape(ImmutableArray<BoundExpression> expressions, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
{
foreach (var expression in expressions)
{
if (!CheckValEscape(expression.Syntax, expression, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics))
{
return false;
}
}

return true;
}

private bool CheckInterpolatedStringHandlerConversionEscape(BoundExpression expression, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
{
var data = expression.GetInterpolatedStringHandlerData();
Expand Down
1 change: 1 addition & 0 deletions 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 @@ -6473,6 +6473,7 @@ BoundExpression bindCollectionInitializerElementAddMethod(
boundCall.Method,
boundCall.Arguments,
boundCall.ReceiverOpt,
boundCall.InitialBindingReceiverIsSubjectToCloning,
boundCall.Expanded,
boundCall.ArgsToParamsOpt,
boundCall.DefaultArguments,
Expand Down
13 changes: 8 additions & 5 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 @@ -153,6 +153,8 @@ private ref struct PlaceholderRegion
private readonly RefSafetyAnalysis _analysis;
private readonly ArrayBuilder<(BoundValuePlaceholderBase, SafeContext)> _placeholders;

public bool ForceRemoveOnDispose { get; init; }

public PlaceholderRegion(RefSafetyAnalysis analysis, ArrayBuilder<(BoundValuePlaceholderBase, SafeContext)> placeholders)
{
_analysis = analysis;
Expand All @@ -167,7 +169,7 @@ public void Dispose()
{
foreach (var (placeholder, _) in _placeholders)
{
_analysis.RemovePlaceholderScope(placeholder);
_analysis.RemovePlaceholderScope(placeholder, forceRemove: ForceRemoveOnDispose);
}
_placeholders.Free();
}
Expand Down Expand Up @@ -200,16 +202,17 @@ private void AddPlaceholderScope(BoundValuePlaceholderBase placeholder, SafeCont
_placeholderScopes[placeholder] = valEscapeScope;
}

#pragma warning disable IDE0060
private void RemovePlaceholderScope(BoundValuePlaceholderBase placeholder)
private void RemovePlaceholderScope(BoundValuePlaceholderBase placeholder, bool forceRemove)
{
Debug.Assert(_placeholderScopes?.ContainsKey(placeholder) == true);

// https://github.com/dotnet/roslyn/issues/65961: Currently, analysis may require subsequent calls
// to GetRefEscape(), etc. for the same expression so we cannot remove placeholders eagerly.
//_placeholderScopes.Remove(placeholder);
if (forceRemove)
{
_placeholderScopes?.Remove(placeholder);
}
}
#pragma warning restore IDE0060

private SafeContext GetPlaceholderScope(BoundValuePlaceholderBase placeholder)
{
Expand Down
2 changes: 2 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 @@ -2057,6 +2057,8 @@
<Field Name="Arguments" Type="ImmutableArray&lt;BoundExpression&gt;"/>
<!-- Used for IOperation to enable translating the initializer to a IDynamicInvocationOperation -->
<Field Name="ImplicitReceiverOpt" Type="BoundExpression?" />
<!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
<Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
<Field Name="Expanded" Type="bool"/>
<Field Name="ArgsToParamsOpt" Type="ImmutableArray&lt;int&gt;" Null="allow"/>
<Field Name="DefaultArguments" Type="BitVector" />
Expand Down
Loading