Skip to content

Check ref safety of arg mixing in collection initializers #76237

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

Closed
jjonescz wants to merge 8 commits into from
Closed

Check ref safety of arg mixing in collection initializers #76237

Show file tree
Hide file tree
Changes from 2 commits
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
88 changes: 66 additions & 22 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 @@ -4455,11 +4455,12 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext scopeOfTheCo

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

case BoundKind.CollectionElementInitializer:
var colElement = (BoundCollectionElementInitializer)expr;
return GetValEscape(colElement.Arguments, scopeOfTheContainingExpression);
// If arg mixing fails when the receiver has calling-method scope (i.e., some arguments could escape into the receiver), make the value scoped.
return
!scopeOfTheContainingExpression.IsConvertibleTo(SafeContext.CallingMethod) &&
Copy link
Member

@RikkiGibson RikkiGibson Dec 11, 2024

Choose a reason for hiding this comment

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

Why should we predicate this only on convertibility to CallingMethod scope? Consider the following:

RS M(ref int param)
{
    int local = 1;
    return new RS(ref param) { local }; // error expected
}

// ref struct RS, Add([UnscopedRef] in int item), etc.

In this case, scopeOfTheContainingExpression is ReturnOnly (right?), and not convertible to CallingMethod. But don't we still need to do a mixing check here?

Copy link
Member

@RikkiGibson RikkiGibson Dec 11, 2024

Choose a reason for hiding this comment

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

I could also imagine scenarios where new RS(...) has some local lifetime, but a collection initializer argument has an even narrower lifetime.

Copy link
Member Author

@jjonescz jjonescz Dec 11, 2024

Choose a reason for hiding this comment

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

The check !scopeOfTheContainingExpression.IsConvertibleTo(SafeContext.CallingMethod) is just an optimization - if it's false, it actually means scopeOfTheContainingExpression == SafeContext., and we can return SafeContext.CallingMethod immediately.

I will refactor the code to make that clearer.

In this case, scopeOfTheContainingExpression is ReturnOnly (right?), and not convertible to CallingMethod. But don't we still need to do a mixing check here?

We do a mixing check there. The !scopeOfTheContainingExpression.IsConvertibleTo(SafeContext.CallingMethod) condition is true in that case.

However, I'm not sure how that example is different from currently tested scenarios - you added a ref param to the ref struct's constructor, but that already goes through different code paths, in this PR I've only changed handling of the collection initializer, i.e., the part after the constructor - { local }.

I could also imagine scenarios where new RS(...) has some local lifetime, but a collection initializer argument has an even narrower lifetime.

I don't follow. Here we are computing ValEscape for the expression new R() { ... }, so it does not yet have any lifetime. And we either determine the expression to have ValEscape of scopeOfTheContainingExpression or CallingMethod. I don't think the collection initializer argument can have narrower lifetime than scopeOfTheContainingExpression (that's the narrowest lifetime we are currently at).

!CheckValEscapeOfCollectionInitializer(colExpr, escapeFrom: scopeOfTheContainingExpression, escapeTo: SafeContext.CallingMethod, BindingDiagnosticBag.Discarded)
? scopeOfTheContainingExpression
: SafeContext.CallingMethod;

case BoundKind.ObjectInitializerMember:
// this node generally makes no sense outside of the context of containing initializer
Expand All @@ -4468,11 +4469,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 +4782,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
Contributor

@cston cston Dec 10, 2024

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

@jjonescz jjonescz Dec 10, 2024

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 +5277,9 @@ 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);

// 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 CheckValEscapeOfCollectionInitializer(colExpr, escapeFrom, escapeTo, diagnostics);

case BoundKind.PointerElementAccess:
var accessedExpression = ((BoundPointerElementAccess)expr).Expression;
Expand Down Expand Up @@ -5561,21 +5573,53 @@ private bool CheckValEscapeOfObjectInitializer(BoundObjectInitializerExpression
return true;
}

#nullable disable

private bool CheckValEscape(ImmutableArray<BoundExpression> expressions, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
private bool CheckValEscapeOfCollectionInitializer(BoundCollectionInitializerExpression colExpr, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
{
foreach (var expression in expressions)
// 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.
Copy link
Member

@RikkiGibson RikkiGibson Dec 12, 2024

Choose a reason for hiding this comment

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

Suggested change
// where the placeholder has `escapeTo` scope and the call has `escapeFrom` scope.
// where the receiverPlaceholder has `escapeTo` scope and the call has `escapeFrom` scope.

suggest using exactly the same name here to make it just a little bit easier to grasp what this comment is conveying.


bool result = true;
using var _ = new PlaceholderRegion(this, [(colExpr.Placeholder, escapeTo)]) { ForceRemoveOnDispose = true };
foreach (var initializer in colExpr.Initializers)
{
if (!CheckValEscape(expression.Syntax, expression, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics))
switch (initializer)
{
return false;
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;
}
}

return true;
return result;
}

#nullable disable

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
17 changes: 10 additions & 7 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .../Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ private BoundExpression MakeDynamicCollectionInitializer(BoundExpression rewritt
{
Debug.Assert(temps.Count == 0);
temps.Free();
return initializer.Update(addMethod, rewrittenArguments, rewrittenReceiver, expanded: false, argsToParamsOpt: default, defaultArguments: default, invokedAsExtensionMethod: false, initializer.ResultKind, rewrittenType);
return initializer.Update(addMethod, rewrittenArguments, rewrittenReceiver, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, expanded: false, argsToParamsOpt: default, defaultArguments: default, invokedAsExtensionMethod: false, initializer.ResultKind, rewrittenType);
}

if (Instrument)
Expand Down
Loading