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.

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

Collection expressions: use inline arrays for collection expressions with span target types #69412

Merged
merged 13 commits into from
Aug 24, 2023
Merged
68 changes: 62 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3852,6 +3852,13 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
return GetInterpolatedStringHandlerConversionEscapeScope(conversion.Operand, scopeOfTheContainingExpression);
}

if (conversion.ConversionKind == ConversionKind.CollectionExpression)
{
return HasLocalScope((BoundCollectionExpression)conversion.Operand)
? scopeOfTheContainingExpression
: CallingMethodScope;
}

if (conversion.Conversion.IsInlineArray)
{
ImmutableArray<BoundExpression> arguments;
Expand Down Expand Up @@ -3959,9 +3966,6 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
var switchExpr = (BoundSwitchExpression)expr;
return GetValEscape(switchExpr.SwitchArms.SelectAsArray(a => a.Value), scopeOfTheContainingExpression);

case BoundKind.CollectionExpression:
return CallingMethodScope;

default:
// in error situations some unexpected nodes could make here
// returning "scopeOfTheContainingExpression" seems safer than throwing.
Expand All @@ -3971,6 +3975,51 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
}
}

#nullable enable
private bool HasLocalScope(BoundCollectionExpression expr)
{
// A non-empty collection expression with span type may be stored
// on the stack. In those cases the expression may have local scope.
if (expr.Type?.IsRefLikeType == true && expr.Elements.Length > 0)
Copy link
Member

@jcouv jcouv Aug 23, 2023

Choose a reason for hiding this comment

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

nit: consider inverting if to flatten method #Resolved

{
var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(_compilation, expr.Type, out var elementType);

switch (collectionTypeKind)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this handle spreads correctly? Maybe we never expect to get any of the below TypeKinds when spreads are present? Might be worth asserting.

Copy link
Member Author

Choose a reason for hiding this comment

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

A collection expression targeting a span is considered local scope, even if the collection includes spreads. See CollectionExpressionTests.RefSafety_Return_02 for example.

{
case CollectionExpressionTypeKind.ReadOnlySpan:
Debug.Assert(elementType is { });
return !LocalRewriter.ShouldUseRuntimeHelpersCreateSpan(expr, elementType);
case CollectionExpressionTypeKind.Span:
return true;
case CollectionExpressionTypeKind.CollectionBuilder:
// For a ref struct type with a builder method, the scope of the collection
// expression is the scope of an invocation of the builder method with the
// collection expression as the span argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

pseudocode might be more clear than prose here. e.g. RS rs = [x, y, z] is equivalent to RS rs = RSFactory.Create((ReadOnlySpan<...>)[x, y, z])

var constructMethod = expr.CollectionBuilderMethod;
if (constructMethod is not { Parameters: [{ RefKind: RefKind.None } parameter] })
{
// Unexpected construct method. Restrict the collection to local scope.
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

we expect an error to be reported elsewhere for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should have filtered out the unexpected collection builder method in Binder.GetCollectionBuilderMethod(). The case here should not be hit.

}
Debug.Assert(constructMethod.ReturnType.Equals(expr.Type, TypeCompareKind.AllIgnoreOptions));
Debug.Assert(parameter.Type.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions));
if (parameter.EffectiveScope == ScopedKind.ScopedValue)
{
return false;
}
if (LocalRewriter.ShouldUseRuntimeHelpersCreateSpan(expr, ((NamedTypeSymbol)parameter.Type).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type))
{
return false;
}
return true;
default:
throw ExceptionUtilities.UnexpectedValue(collectionTypeKind); // ref struct collection type with unexpected type kind
}
}
return false;
}
#nullable disable

private uint GetTupleValEscape(ImmutableArray<BoundExpression> elements, uint scopeOfTheContainingExpression)
{
uint narrowestScope = scopeOfTheContainingExpression;
Expand Down Expand Up @@ -4374,6 +4423,16 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF
return CheckInterpolatedStringHandlerConversionEscape(conversion.Operand, escapeFrom, escapeTo, diagnostics);
}

if (conversion.ConversionKind == ConversionKind.CollectionExpression)
{
if (HasLocalScope((BoundCollectionExpression)conversion.Operand) && escapeTo < _localScopeDepth)
{
Error(diagnostics, ErrorCode.ERR_CollectionExpressionEscape, node, expr.Type);
return false;
}
return true;
}

if (conversion.Conversion.IsInlineArray)
{
ImmutableArray<BoundExpression> arguments;
Expand Down Expand Up @@ -4488,9 +4547,6 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF

return true;

case BoundKind.CollectionExpression:
return true;

default:
// in error situations some unexpected nodes could make here
// returning "false" seems safer than throwing.
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6852,6 +6852,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CollectionBuilderAttributeInvalidMethodName" xml:space="preserve">
<value>The CollectionBuilderAttribute method name is invalid.</value>
</data>
<data name="ERR_CollectionExpressionEscape" xml:space="preserve">
<value>A collection expression of type '{0}' cannot be used in this context because it may be exposed outside of the current scope.</value>
</data>
<data name="ERR_EqualityContractRequiresGetter" xml:space="preserve">
<value>Record equality contract property '{0}' must have a get accessor.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ private bool TryEmitReadonlySpanAsBlobWrapper(NamedTypeSymbol spanType, BoundExp
}

/// <summary>Gets whether the element type of an array is appropriate for storing in a blob.</summary>
private static bool IsTypeAllowedInBlobWrapper(SpecialType type) => type is
internal static bool IsTypeAllowedInBlobWrapper(SpecialType type) => type is
// 1 byte
// For primitives that are a single byte in size, a span can point directly to a blob
// containing the constant data.
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2268,6 +2268,7 @@ internal enum ErrorCode
WRN_UseDefViolationRefField = 9201,

ERR_FeatureNotAvailableInVersion12 = 9202,
ERR_CollectionExpressionEscape = 9203,

#endregion

Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2399,6 +2399,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.WRN_ByValArraySizeConstRequired:
case ErrorCode.WRN_UseDefViolationRefField:
case ErrorCode.ERR_FeatureNotAvailableInVersion12:
case ErrorCode.ERR_CollectionExpressionEscape:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ internal sealed partial class LocalRewriter : BoundTreeRewriterWithStackGuard
private readonly int _topLevelMethodOrdinal;
private DelegateCacheRewriter? _lazyDelegateCacheRewriter;
private bool _inExpressionLambda;
private ArrayBuilder<LocalSymbol>? _additionalLocals;

/// <summary>
/// The original body of the current lambda or local function body, or null if not currently lowering a lambda.
Expand Down Expand Up @@ -618,6 +619,41 @@ public override BoundNode VisitRefTypeOperator(BoundRefTypeOperator node)
return node.Update(operand, getTypeFromHandle, type);
}

private BoundStatement? RewriteFieldOrPropertyInitializer(BoundStatement initializer)
{
var previousLocals = _additionalLocals;
Copy link
Member

@jcouv jcouv Aug 11, 2023

Choose a reason for hiding this comment

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

I didn't understand, why introduce this new _additionalLocals machinery, which affects every single block, when one can easily insert a new block during lowering of specific nodes when additional locals are needed? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

The inline array temporary needs to be added in the enclosing scope (so the lifetime of the temporary matches that scope) rather than being created in a new nested block.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks much for the explanation. Consider adding a comment on the declaration of _additionalLocals (allows adding locals without inserting new blocks/scopes)

_additionalLocals = ArrayBuilder<LocalSymbol>.GetInstance();

try
{
if (initializer.Kind == BoundKind.Block)
{
var block = (BoundBlock)initializer;

var statement = RewriteExpressionStatement((BoundExpressionStatement)block.Statements.Single(), suppressInstrumentation: true);
Debug.Assert(statement is { });
return block.Update(block.Locals.AddRange(_additionalLocals), block.LocalFunctions, block.HasUnsafeModifier, block.Instrumentation, ImmutableArray.Create(statement));
}
else
{
var statement = RewriteExpressionStatement((BoundExpressionStatement)initializer, suppressInstrumentation: true);
if (statement is null || _additionalLocals.Count == 0)
{
return statement;
}
return new BoundBlock(
Copy link
Member

@jcouv jcouv Aug 15, 2023

Choose a reason for hiding this comment

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

I'm a little worried about the fact that a Visit method besides VisitBlock has to handle additional locals.
How do we know whether any other Visit methods (current or future) also needs to handle that? Is there anything we could do to help remembering to handle? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't thought of an easy way to assert that _additionalLocals is always at the expected scope. If there are Visit methods that represent blocks but are not setting _additionalLocals, then there two possibilities. If the block is a top-level block, we should fail the assert in VisitArrayOrSpanCollectionExpression() that _additionalLocals is { }. Otherwise, _additionalLocals will represent the containing block of the actual block. And for the latter case, that should just mean that the inline array temporaries will have longer scope than is strictly necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding assert to LocalRewriter.VisitCollectionExpression() that _additionalLocals is { } to increase the chance of catching other cases.

statement.Syntax,
_additionalLocals.ToImmutable(),
ImmutableArray.Create(statement));
}
}
finally
{
_additionalLocals.Free();
_additionalLocals = previousLocals;
}
}

public override BoundNode VisitTypeOrInstanceInitializers(BoundTypeOrInstanceInitializers node)
{
ImmutableArray<BoundStatement> originalStatements = node.Statements;
Expand All @@ -626,18 +662,7 @@ public override BoundNode VisitTypeOrInstanceInitializers(BoundTypeOrInstanceIni
{
if (IsFieldOrPropertyInitializer(initializer))
{
if (initializer.Kind == BoundKind.Block)
{
var block = (BoundBlock)initializer;

var statement = RewriteExpressionStatement((BoundExpressionStatement)block.Statements.Single(), suppressInstrumentation: true);
Debug.Assert(statement is { });
statements.Add(block.Update(block.Locals, block.LocalFunctions, block.HasUnsafeModifier, block.Instrumentation, ImmutableArray.Create(statement)));
}
else
{
statements.Add(RewriteExpressionStatement((BoundExpressionStatement)initializer, suppressInstrumentation: true));
}
statements.Add(RewriteFieldOrPropertyInitializer(initializer));
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.PooledObjects;
Expand All @@ -20,26 +19,37 @@ public override BoundNode VisitBlock(BoundBlock node)
}

var builder = ArrayBuilder<BoundStatement>.GetInstance();
VisitStatementSubList(builder, node.Statements);
var previousLocals = _additionalLocals;
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment about what we are trying to do might be appropriate, e.g. when _additionalLocals is null here, it means we are entering the outermost block of the current function. Symbols inserted to _additionalLocals by child statements should be put in this block.

_additionalLocals = ArrayBuilder<LocalSymbol>.GetInstance();

var additionalLocals = TemporaryArray<LocalSymbol>.Empty;

BoundBlockInstrumentation? instrumentation = null;
if (Instrument)
try
{
Instrumenter.InstrumentBlock(node, this, ref additionalLocals, out var prologue, out var epilogue, out instrumentation);
if (prologue != null)
{
builder.Insert(0, prologue);
}
VisitStatementSubList(builder, node.Statements);

var additionalLocals = TemporaryArray<LocalSymbol>.Empty;

if (epilogue != null)
BoundBlockInstrumentation? instrumentation = null;
if (Instrument)
{
builder.Add(epilogue);
Instrumenter.InstrumentBlock(node, this, ref additionalLocals, out var prologue, out var epilogue, out instrumentation);
if (prologue != null)
{
builder.Insert(0, prologue);
}

if (epilogue != null)
{
builder.Add(epilogue);
}
}
}

return new BoundBlock(node.Syntax, node.Locals.AddRange(additionalLocals), node.LocalFunctions, node.HasUnsafeModifier, instrumentation, builder.ToImmutableAndFree(), node.HasErrors);
return new BoundBlock(node.Syntax, node.Locals.AddRange(_additionalLocals).AddRange(additionalLocals), node.LocalFunctions, node.HasUnsafeModifier, instrumentation, builder.ToImmutableAndFree(), node.HasErrors);
}
finally
{
_additionalLocals.Free();
_additionalLocals = previousLocals;
}
}

/// <summary>
Expand Down
Loading