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

Initial binding and lowering of collection literals with single-valued elements #67025

Merged
merged 19 commits into from
Mar 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3804,6 +3804,11 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
var switchExpr = (BoundSwitchExpression)expr;
return GetValEscape(switchExpr.SwitchArms.SelectAsArray(a => a.Value), scopeOfTheContainingExpression);

case BoundKind.ArrayOrSpanCollectionLiteralExpression:
case BoundKind.CollectionInitializerCollectionLiteralExpression:
// PROTOTYPE: Revisit if spans may be optimized to avoid heap allocation.
return CallingMethodScope;
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2023

Choose a reason for hiding this comment

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

I would expect a prototype comment here. If the collection literal is converted to span I would expect us to use the local scope depth where the literal appears, or some other reasonable value like that. #Resolved


default:
// in error situations some unexpected nodes could make here
// returning "scopeOfTheContainingExpression" seems safer than throwing.
Expand Down Expand Up @@ -4282,6 +4287,11 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF

return true;

case BoundKind.ArrayOrSpanCollectionLiteralExpression:
case BoundKind.CollectionInitializerCollectionLiteralExpression:
// PROTOTYPE: Revisit if spans may be optimized to avoid heap allocation.
return true;
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2023

Choose a reason for hiding this comment

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

would also expect a prototype comment here. e.g. void M(out Span<int> span) => span = [1, 2, 3] should be a ref safety error. #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.

Added a PROTOTYPE comment to GetValEscape() and CheckValEscape(). As I understand the proposal though, span = [1, 2, 3] should not result an error because the array would be allocated on the heap in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline, I'm basically willing to buy in to this line of thinking. I want LDM to discuss the possibility of a stackalloc [1, 2, 3] expression. This would let users avoid inadvertently changing their code to introduce heap allocations.


default:
// in error situations some unexpected nodes could make here
// returning "false" seems safer than throwing.
Expand Down
193 changes: 193 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,18 @@ BoundExpression createConversion(
return ConvertObjectCreationExpression(syntax, (BoundUnconvertedObjectCreationExpression)source, conversion, isCast, destination, conversionGroupOpt, wasCompilerGenerated, diagnostics);
}

if (conversion.IsCollectionLiteral)
{
return ConvertCollectionLiteralExpression(
(BoundUnconvertedCollectionLiteralExpression)source,
conversion,
isCast,
destination,
conversionGroupOpt,
wasCompilerGenerated,
diagnostics);
}

if (source.Kind == BoundKind.UnconvertedConditionalOperator)
{
Debug.Assert(source.Type is null);
Expand Down Expand Up @@ -404,6 +416,187 @@ static BoundExpression bindObjectCreationExpression(
}
}

private BoundExpression ConvertCollectionLiteralExpression(
BoundUnconvertedCollectionLiteralExpression node,
Conversion conversion,
bool isCast,
TypeSymbol targetType,
ConversionGroup? conversionGroupOpt,
bool wasCompilerGenerated,
BindingDiagnosticBag diagnostics)
{
var syntax = (CSharpSyntaxNode)node.Syntax;
var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2023

Choose a reason for hiding this comment

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

I wasn't sure what the effect of WasCompilerGenerated = true is here. #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.

This matches BindInitializerExpression, and seems reasonable to me since the placeholder is not in the source.

TypeSymbol? elementType;
BoundCollectionLiteralExpression collectionLiteral;

if (targetType is ArrayTypeSymbol arrayType)
{
if (!arrayType.IsSZArray)
{
Error(diagnostics, ErrorCode.ERR_CollectionLiteralTargetTypeNotConstructible, syntax, targetType);
}
collectionLiteral = bindArrayOrSpan(syntax, spanConstructor: null, implicitReceiver, node.Initializers, arrayType.ElementType, diagnostics);
}
else if (isSpanType(targetType, WellKnownType.System_Span_T, out elementType))
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2023

Choose a reason for hiding this comment

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

Should there be a prototype comment to track stack-allocating collection literals converted to span? I'm not opposed to us punting on that in the initial implementation pass, though I'd probably want the ref safety rules to work as if the collection literal is stack-allocated.

My expectations around this area are based on reading https://github.com/dotnet/csharplang/blob/main/proposals/collection-literals.md#span-types. Let me know if that's not up to date. #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.

Added a comment to GetValEscape() and CheckValEscape() for now.

{
var spanConstructor = getSpanConstructor(syntax, targetType, WellKnownMember.System_Span_T__ctor_Array, diagnostics);
collectionLiteral = bindArrayOrSpan(syntax, spanConstructor, implicitReceiver, node.Initializers, elementType, diagnostics);
}
else if (isSpanType(targetType, WellKnownType.System_ReadOnlySpan_T, out elementType))
{
var spanConstructor = getSpanConstructor(syntax, targetType, WellKnownMember.System_ReadOnlySpan_T__ctor_Array, diagnostics);
collectionLiteral = bindArrayOrSpan(syntax, spanConstructor, implicitReceiver, node.Initializers, elementType, diagnostics);
}
else
{
collectionLiteral = bindCollectionInitializer(syntax, implicitReceiver, node.Initializers, diagnostics);
}

return new BoundConversion(
syntax,
collectionLiteral,
conversion,
node.Binder.CheckOverflowAtRuntime,
explicitCastInCode: isCast && !wasCompilerGenerated,
conversionGroupOpt,
collectionLiteral.ConstantValueOpt,
targetType);

bool isSpanType(TypeSymbol targetType, WellKnownType spanType, [NotNullWhen(true)] out TypeSymbol? elementType)
{
if (targetType is NamedTypeSymbol { Arity: 1 } namedType
&& TypeSymbol.Equals(namedType.OriginalDefinition, Compilation.GetWellKnownType(spanType), TypeCompareKind.AllIgnoreOptions))
{
elementType = namedType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type;
return true;
}
elementType = null;
return false;
}

BoundArrayOrSpanCollectionLiteralExpression bindArrayOrSpan(
CSharpSyntaxNode syntax,
MethodSymbol? spanConstructor,
BoundObjectOrCollectionValuePlaceholder implicitReceiver,
ImmutableArray<BoundExpression> elements,
TypeSymbol elementType,
BindingDiagnosticBag diagnostics)
{
var builder = ArrayBuilder<BoundExpression>.GetInstance(elements.Length);
foreach (var element in elements)
{
builder.Add(convertArrayElement(element, elementType, diagnostics));
}
return new BoundArrayOrSpanCollectionLiteralExpression(
syntax,
spanConstructor,
naturalTypeOpt: null,
wasTargetTyped: true,
implicitReceiver,
builder.ToImmutableAndFree(),
targetType)
{ WasCompilerGenerated = wasCompilerGenerated };
}

BoundCollectionInitializerCollectionLiteralExpression bindCollectionInitializer(
CSharpSyntaxNode syntax,
BoundObjectOrCollectionValuePlaceholder implicitReceiver,
ImmutableArray<BoundExpression> elements,
BindingDiagnosticBag diagnostics)
{
BoundExpression collectionCreation;
if (targetType is NamedTypeSymbol namedType)
{
var analyzedArguments = AnalyzedArguments.GetInstance();
collectionCreation = BindClassCreationExpression(syntax, namedType.Name, syntax, namedType, analyzedArguments, diagnostics);
collectionCreation.WasCompilerGenerated = true;
analyzedArguments.Free();
}
else if (targetType is TypeParameterSymbol typeParameter)
{
var arguments = AnalyzedArguments.GetInstance();
collectionCreation = BindTypeParameterCreationExpression(syntax, typeParameter, arguments, initializerOpt: null, typeSyntax: syntax, wasTargetTyped: true, diagnostics);
arguments.Free();
}
else
{
collectionCreation = new BoundBadExpression(syntax, LookupResultKind.NotCreatable, ImmutableArray<Symbol?>.Empty, ImmutableArray<BoundExpression>.Empty, targetType);
}

bool hasEnumerableInitializerType = collectionTypeImplementsIEnumerable(targetType, syntax, diagnostics);
if (!hasEnumerableInitializerType && !targetType.IsErrorType())
{
Error(diagnostics, ErrorCode.ERR_CollectionLiteralTargetTypeNotConstructible, syntax, targetType);
}

var collectionInitializerAddMethodBinder = this.WithAdditionalFlags(BinderFlags.CollectionInitializerAddMethod);
var builder = ArrayBuilder<BoundExpression>.GetInstance(node.Initializers.Length);
foreach (var element in node.Initializers)
{
var result = BindCollectionInitializerElementAddMethod(
(ExpressionSyntax)element.Syntax,
ImmutableArray.Create(element),
hasEnumerableInitializerType,
collectionInitializerAddMethodBinder,
diagnostics,
implicitReceiver);
result.WasCompilerGenerated = true;
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2023

Choose a reason for hiding this comment

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

Is this consistent with the bound tree for item in new Collection() { item }? #Resolved

Copy link
Member Author

@cston cston Mar 11, 2023

Choose a reason for hiding this comment

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

builder.Add(result);
}
return new BoundCollectionInitializerCollectionLiteralExpression(
syntax,
collectionCreation,
naturalTypeOpt: null, // PROTOTYPE: Support natural type.
wasTargetTyped: true, // PROTOTYPE: Support natural type.
implicitReceiver,
builder.ToImmutableAndFree(),
targetType)
{ WasCompilerGenerated = wasCompilerGenerated };
}

MethodSymbol? getSpanConstructor(CSharpSyntaxNode syntax, TypeSymbol spanType, WellKnownMember spanMember, BindingDiagnosticBag diagnostics)
{
return (MethodSymbol?)GetWellKnownTypeMember(spanMember, diagnostics, syntax: syntax)?.SymbolAsMember((NamedTypeSymbol)spanType);
}

bool collectionTypeImplementsIEnumerable(TypeSymbol targetType, CSharpSyntaxNode syntax, BindingDiagnosticBag diagnostics)
{
// This implementation differs from CollectionInitializerTypeImplementsIEnumerable().
// That method checks for an implicit conversion from IEnumerable to the collection type,
// but that would allow: Nullable<StructCollection> s = [];
var ienumerableType = GetSpecialType(SpecialType.System_Collections_IEnumerable, diagnostics, syntax);
var allInterfaces = targetType is TypeParameterSymbol typeParameter
? typeParameter.AllEffectiveInterfacesNoUseSiteDiagnostics
: targetType.AllInterfacesNoUseSiteDiagnostics;
return allInterfaces.Any(static (a, b) => a.Equals(b, TypeCompareKind.AllIgnoreOptions), ienumerableType);
}

BoundExpression convertArrayElement(BoundExpression element, TypeSymbol elementType, BindingDiagnosticBag diagnostics)
{
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 9, 2023

Choose a reason for hiding this comment

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

There's no particular reason to try and share this between Classify, etc. calls, right? #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 don't think we need to share this between calls to convertArrayElement(). This is a struct type and since we're already passing in a BindingDiagnosticBag, it seems cleaner to have this method update that diagnostic bag than adding a second diagnostic parameter.

var conversion = Conversions.ClassifyImplicitConversionFromExpression(element, elementType, ref useSiteInfo);
diagnostics.Add(element.Syntax, useSiteInfo);
bool hasErrors = !conversion.IsValid;
if (hasErrors)
{
GenerateImplicitConversionError(diagnostics, element.Syntax, conversion, element, elementType);
}
var result = CreateConversion(
element.Syntax,
element,
conversion,
isCast: false,
conversionGroupOpt: null,
wasCompilerGenerated: true,
destination: elementType,
diagnostics: diagnostics,
hasErrors: hasErrors);
result.WasCompilerGenerated = true;
return result;
}
}

/// <summary>
/// Rewrite the subexpressions in a conditional expression to convert the whole thing to the destination type.
/// </summary>
Expand Down
44 changes: 40 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -755,8 +755,7 @@ private BoundExpression BindExpressionInternal(ExpressionSyntax node, BindingDia
return BadExpression(node);

case SyntaxKind.CollectionCreationExpression:
// PROTOTYPE: Implement binding for this.
return BadExpression(node);
return BindCollectionLiteralExpression((CollectionCreationExpressionSyntax)node, diagnostics);

case SyntaxKind.NullableType:
// Not reachable during method body binding, but
Expand Down Expand Up @@ -4482,6 +4481,41 @@ BoundExpression bindObjectCreationExpression(ObjectCreationExpressionSyntax node
}
}

private BoundExpression BindCollectionLiteralExpression(CollectionCreationExpressionSyntax syntax, BindingDiagnosticBag diagnostics)
{
MessageID.IDS_FeatureCollectionLiterals.CheckFeatureAvailability(diagnostics, syntax, syntax.OpenBracketToken.GetLocation());

var builder = ArrayBuilder<BoundExpression>.GetInstance(syntax.Elements.Count);
foreach (var element in syntax.Elements)
{
builder.Add(bindElement(element, diagnostics));
}
return new BoundUnconvertedCollectionLiteralExpression(syntax, builder.ToImmutableAndFree(), this);

BoundExpression bindElement(CollectionElementSyntax syntax, BindingDiagnosticBag diagnostics)
{
switch (syntax)
{
case ExpressionElementSyntax expressionElementSyntax:
return BindValue(expressionElementSyntax.Expression, diagnostics, BindValueKind.RValue);

case DictionaryElementSyntax dictionaryElementSyntax:
_ = BindValue(dictionaryElementSyntax.KeyExpression, diagnostics, BindValueKind.RValue);
_ = BindValue(dictionaryElementSyntax.ValueExpression, diagnostics, BindValueKind.RValue);
Error(diagnostics, ErrorCode.ERR_CollectionLiteralElementNotImplemented, syntax);
return BadExpression(syntax);

case SpreadElementSyntax spreadElementSyntax:
_ = BindValue(spreadElementSyntax.Expression, diagnostics, BindValueKind.RValue);
Error(diagnostics, ErrorCode.ERR_CollectionLiteralElementNotImplemented, syntax);
return BadExpression(syntax);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 27, 2023

Choose a reason for hiding this comment

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

nit: consider specific errors here to make it clear that these just aren't supported currently. saying that it's an invalid-expr seems like it could be confusing for anyone trying this out. #Resolved


default:
throw ExceptionUtilities.UnexpectedValue(syntax.Kind());
}
}
}

private BoundExpression BindDelegateCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol type, BindingDiagnosticBag diagnostics)
{
AnalyzedArguments analyzedArguments = AnalyzedArguments.GetInstance();
Expand Down Expand Up @@ -4756,7 +4790,6 @@ private BoundExpression MakeBadExpressionForObjectCreation(SyntaxNode node, Type

return new BoundBadExpression(node, LookupResultKind.NotCreatable, ImmutableArray.Create<Symbol?>(type), children.ToImmutableAndFree(), type) { WasCompilerGenerated = wasCompilerGenerated };
}
#nullable disable

private BoundObjectInitializerExpressionBase BindInitializerExpression(
InitializerExpressionSyntax syntax,
Expand Down Expand Up @@ -4788,6 +4821,7 @@ private BoundObjectInitializerExpressionBase BindInitializerExpression(
throw ExceptionUtilities.Unreachable();
}
}
#nullable disable

private BoundExpression BindInitializerExpressionOrValue(
ExpressionSyntax syntax,
Expand Down Expand Up @@ -5971,8 +6005,9 @@ private BoundExpression BindTypeParameterCreationExpression(ObjectCreationExpres
return result;
}

#nullable enable
private BoundExpression BindTypeParameterCreationExpression(
SyntaxNode node, TypeParameterSymbol typeParameter, AnalyzedArguments analyzedArguments, InitializerExpressionSyntax initializerOpt,
SyntaxNode node, TypeParameterSymbol typeParameter, AnalyzedArguments analyzedArguments, InitializerExpressionSyntax? initializerOpt,
SyntaxNode typeSyntax, bool wasTargetTyped, BindingDiagnosticBag diagnostics)
{
if (!typeParameter.HasConstructorConstraint && !typeParameter.IsValueType)
Expand All @@ -5998,6 +6033,7 @@ private BoundExpression BindTypeParameterCreationExpression(

return MakeBadExpressionForObjectCreation(node, typeParameter, analyzedArguments, initializerOpt, typeSyntax, diagnostics);
}
#nullable disable

/// <summary>
/// Given the type containing constructors, gets the list of candidate instance constructors and uses overload resolution to determine which one should be called.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ internal static Conversion GetTrivialConversion(ConversionKind kind)
internal static Conversion ImplicitEnumeration => new Conversion(ConversionKind.ImplicitEnumeration);
internal static Conversion ImplicitThrow => new Conversion(ConversionKind.ImplicitThrow);
internal static Conversion ObjectCreation => new Conversion(ConversionKind.ObjectCreation);
internal static Conversion CollectionLiteral => new Conversion(ConversionKind.CollectionLiteral);
internal static Conversion AnonymousFunction => new Conversion(ConversionKind.AnonymousFunction);
internal static Conversion Boxing => new Conversion(ConversionKind.Boxing);
internal static Conversion NullLiteral => new Conversion(ConversionKind.NullLiteral);
Expand Down Expand Up @@ -631,6 +632,11 @@ public bool IsObjectCreation
}
}

/// <summary>
/// Returns true if the conversion is an implicit collection literal expression conversion.
/// </summary>
public bool IsCollectionLiteral => Kind == ConversionKind.CollectionLiteral;

/// <summary>
/// Returns true if the conversion is an implicit switch expression conversion.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ internal enum ConversionKind : byte

DefaultLiteral, // a conversion from a `default` literal to any type
ObjectCreation, // a conversion from a `new()` expression to any type
CollectionLiteral, // a conversion from a collection literal to any type

InterpolatedStringHandler, // A conversion from an interpolated string literal to a type attributed with InterpolatedStringBuilderAttribute
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public static bool IsImplicitConversion(this ConversionKind conversionKind)
case StackAllocToSpanType:
case ImplicitPointer:
case ObjectCreation:
case CollectionLiteral:
return true;

case ExplicitNumeric:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,7 @@ public Conversion ClassifyStandardConversion(BoundExpression sourceExpression, T
return Conversion.NoConversion;
}

// PROTOTYPE: Ensure collection literal conversions are not considered standard implicit conversions.
private static bool IsStandardImplicitConversionFromExpression(ConversionKind kind)
{
if (IsStandardImplicitConversionFromType(kind))
Expand Down Expand Up @@ -1094,6 +1095,9 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi

case BoundKind.UnconvertedObjectCreationExpression:
return Conversion.ObjectCreation;

case BoundKind.UnconvertedCollectionLiteralExpression:
Copy link
Member

@333fred 333fred Mar 1, 2023

Choose a reason for hiding this comment

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

I would have expected to see changes to ClassifyStandardImplicitConversion, or IsStandardImplicitConversion. We probably have a test hole around use of a collection literal as an input to a user-defined conversion. #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.

Will this become an issue once collection literals have natural types, or even within this change? I've added a test with a user-defined conversion to catch this case once natural types are supported. Let me know if I'm overlooking something.

Copy link
Member

Choose a reason for hiding this comment

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

Likely will require the natural type.

return Conversion.CollectionLiteral;
}

return Conversion.NoConversion;
Expand Down
Loading