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

Collection expression arguments: initial binding and lowering #76903

Merged
merged 32 commits into from
Feb 4, 2025
Merged

Collection expression arguments: initial binding and lowering #76903

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
3402e2b
Collection expression arguments
cston Jan 10, 2025
0e8e4d9
Initial binding and lowering
cston Jan 12, 2025
0ff7500
with()
cston Jan 22, 2025
6346611
Add CollectionExpressionArgumentTests
cston Jan 22, 2025
b70034e
Merge remote-tracking branch 'upstream/features/dictionary-expression…
cston Jan 23, 2025
41f8519
Merge remote-tracking branch 'upstream/features/dictionary-expression…
cston Jan 24, 2025
3059939
Update check for List<T> optimization
cston Jan 24, 2025
cb99c24
Collection arguments must be first
cston Jan 24, 2025
fa26da0
Fix build
cston Jan 24, 2025
a79ab2d
Fix tests
cston Jan 24, 2025
ac60002
Fix build
cston Jan 24, 2025
26ee8e2
Update error text
cston Jan 24, 2025
a9a8efa
Not supported diagnostic; additional tests
cston Jan 24, 2025
094a719
More tests
cston Jan 25, 2025
69ca245
Add support for CollectionBuilder types
cston Jan 25, 2025
b41f88f
Remove arg support for CollectionBuilder types
cston Jan 25, 2025
f545d69
Remove arg support for dictionary interface types
cston Jan 25, 2025
64782d4
More tests
cston Jan 25, 2025
e75432b
Remove arg support for CollectionBuilder types
cston Jan 26, 2025
909bd6d
PROTOTYPE comments
cston Jan 26, 2025
e84cfcf
Fix test
cston Jan 27, 2025
19d5fa0
PROTOTYPE comments
cston Jan 27, 2025
16860b2
Params cycles
cston Jan 27, 2025
3546bef
Rename to BoundWithElement
cston Jan 30, 2025
92a09f6
Rename to BoundWithElement
cston Jan 30, 2025
4d2fab1
Test __arglist
cston Jan 30, 2025
cbfcf55
Update test
cston Jan 30, 2025
c492c1d
Rename
cston Jan 31, 2025
ee5a786
Rename to BoundCollectionExpressionWithElement
cston Feb 4, 2025
a92872b
Rename local function
cston Feb 4, 2025
2d4294e
Add PROTOTYPE comment
cston Feb 4, 2025
6cf1511
Rename
cston Feb 4, 2025
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
118 changes: 85 additions & 33 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,13 +895,33 @@ private BoundCollectionExpression ConvertCollectionExpression(
}

implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
collectionCreation = BindCollectionExpressionConstructor(syntax, targetType, constructor, diagnostics);
Debug.Assert((collectionCreation is BoundNewT && !isExpanded && constructor is null) ||
(collectionCreation is BoundObjectCreationExpression creation && creation.Expanded == isExpanded && creation.Constructor == constructor));

if (collectionCreation.HasErrors)
// Bind collection creation with arguments.
foreach (var element in elements)
Copy link
Member

@jcouv jcouv Jan 30, 2025 •
edited by cston
Loading

Choose a reason for hiding this comment

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

It looks like we're converting the arguments in the with-element even when the with-element is not in the right position, or there are multiple. Feels like we should only convert the one that is in the right position, and do some error binding (bind to natural type?) for the error ones (but not updating collectionCreation) #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems useful to me to bind with(...) as arguments to the best candidate method regardless of whether the arguments are in the first position, since presumably that is the intent.

Copy link
Member

@jcouv jcouv Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Binding more than one with element still makes me uncomfortable. I could be convinced to bind a single with in the wrong position.
Two reasons:

  1. Semantically, there's only one object creation in a collection expression. But presumably QuickInfo on each with might show different constructors...
  2. When there are multiple with elements, any diagnostics reported on those in the wrong position are guaranteed to be cascading diagnostics. Do we have a test for that?
    It may be good to get a second opinion and/or discuss after standup.

Copy link
Member Author

Choose a reason for hiding this comment

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

When there are multiple with elements, any diagnostics reported on those in the wrong position are guaranteed to be cascading diagnostics.

Overload resolution for the constructor is independent of the position of the with(), so one does not affect the other.

Do we have a test for that?

Yes, CollectionExpressionArguments.LanguageVersion_03() tests List<int> l = [with(x: 1), with(y: 2)];

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Julien. I'm not comfortable binding multiple constructors for collection expressions. I'm fine with out of position with elements, but multiple constructors will have potential cascading effects, not just in diagnostics, but also in other IDE features, as well as forcing handling of multiple constructors elsewhere in our code. I think we should just turn secondary withs into BoundBadExpressions, bind the expressions for error recovery, and be done with it.

Copy link
Member Author

@cston cston Feb 4, 2025 •
edited
Loading

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 revisit this later, likely when implementing IOperation support.

{
return BindCollectionExpressionForErrorRecovery(node, targetType, inConversion: true, diagnostics);
if (element is BoundUnconvertedCollectionArguments collectionArguments)
{
var analyzedArguments = AnalyzedArguments.GetInstance();
collectionArguments.GetArguments(analyzedArguments);
var collectionWithArguments = BindCollectionExpressionConstructor(syntax, targetType, constructor, analyzedArguments, diagnostics);
analyzedArguments.Free();
collectionCreation ??= collectionWithArguments;
}
}

// Bind collection creation with no arguments if necessary.
if (collectionCreation is null)
{
var analyzedArguments = AnalyzedArguments.GetInstance();
collectionCreation = BindCollectionExpressionConstructor(syntax, targetType, constructor, analyzedArguments, diagnostics);
analyzedArguments.Free();
Debug.Assert((collectionCreation is BoundNewT && !isExpanded && constructor is null) ||
(collectionCreation is BoundObjectCreationExpression creation && creation.Expanded == isExpanded && creation.Constructor == constructor));

if (collectionCreation.HasErrors)
{
return BindCollectionExpressionForErrorRecovery(node, targetType, inConversion: true, diagnostics);
}
}

if (!elements.IsDefaultOrEmpty && HasCollectionInitializerTypeInProgress(syntax, targetType))
Expand All @@ -913,20 +933,32 @@ private BoundCollectionExpression ConvertCollectionExpression(
var collectionInitializerAddMethodBinder = new CollectionInitializerAddMethodBinder(syntax, targetType, this);
foreach (var element in elements)
{
BoundNode convertedElement = element is BoundCollectionExpressionSpreadElement spreadElement ?
(BoundNode)BindCollectionExpressionSpreadElementAddMethod(
(SpreadElementSyntax)spreadElement.Syntax,
spreadElement,
collectionInitializerAddMethodBinder,
implicitReceiver,
diagnostics) :
BindCollectionInitializerElementAddMethod(
element.Syntax,
ImmutableArray.Create((BoundExpression)element),
hasEnumerableInitializerType: true,
collectionInitializerAddMethodBinder,
diagnostics,
implicitReceiver);
BoundNode convertedElement;
switch (element)
{
case BoundUnconvertedCollectionArguments collectionArguments:
// Handled above.
continue;
case BoundCollectionExpressionSpreadElement spreadElement:
convertedElement = BindCollectionExpressionSpreadElementAddMethod(
(SpreadElementSyntax)spreadElement.Syntax,
spreadElement,
collectionInitializerAddMethodBinder,
implicitReceiver,
diagnostics);
break;
case BoundKeyValuePairElement:
throw ExceptionUtilities.UnexpectedValue(element);
default:
convertedElement = BindCollectionInitializerElementAddMethod(
element.Syntax,
ImmutableArray.Create((BoundExpression)element),
hasEnumerableInitializerType: true,
collectionInitializerAddMethodBinder,
diagnostics,
implicitReceiver);
break;
}
builder.Add(convertedElement);
}
}
Expand Down Expand Up @@ -964,7 +996,6 @@ private BoundCollectionExpression ConvertCollectionExpression(
var elementConversions = conversion.UnderlyingConversions;

Debug.Assert(elementType is { });
Debug.Assert(elements.Length <= elementConversions.Length);
Debug.Assert(elementConversions.All(c => c.Exists));

int conversionIndex = 0;
Expand All @@ -973,6 +1004,12 @@ private BoundCollectionExpression ConvertCollectionExpression(
BoundNode convertedElement;
switch (element)
{
case BoundUnconvertedCollectionArguments collectionArguments:
if (collectionArguments.Arguments.Length > 0)
{
diagnostics.Add(ErrorCode.ERR_CollectionArgumentsNotSupportedForType, ((WithElementSyntax)collectionArguments.Syntax).WithKeyword, targetType);
}
continue;
case BoundCollectionExpressionSpreadElement spreadElement:
convertedElement = bindSpreadElement(
spreadElement,
Expand Down Expand Up @@ -1105,8 +1142,7 @@ private bool HasCollectionInitializerTypeInProgress(SyntaxNode syntax, TypeSymbo
Debug.Assert(result);

var useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
Conversion collectionBuilderReturnTypeConversion;
collectionBuilderMethod = GetCollectionBuilderMethod(namedType, elementTypeOriginalDefinition.Type, builderType, methodName, ref useSiteInfo, out collectionBuilderReturnTypeConversion);
collectionBuilderMethod = GetCollectionBuilderMethod(namedType, elementTypeOriginalDefinition.Type, builderType, methodName, ref useSiteInfo);
diagnostics.Add(syntax, useSiteInfo);
if (collectionBuilderMethod is null)
{
Expand All @@ -1115,8 +1151,6 @@ private bool HasCollectionInitializerTypeInProgress(SyntaxNode syntax, TypeSymbo
return null;
}

Debug.Assert(collectionBuilderReturnTypeConversion.Exists);

ReportUseSite(collectionBuilderMethod, diagnostics, syntax.Location);

var parameterType = (NamedTypeSymbol)collectionBuilderMethod.Parameters[0].Type;
Expand All @@ -1134,7 +1168,12 @@ private bool HasCollectionInitializerTypeInProgress(SyntaxNode syntax, TypeSymbo
return collectionBuilderMethod;
}

internal BoundExpression BindCollectionExpressionConstructor(SyntaxNode syntax, TypeSymbol targetType, MethodSymbol? constructor, BindingDiagnosticBag diagnostics)
internal BoundExpression BindCollectionExpressionConstructor(
SyntaxNode syntax,
TypeSymbol targetType,
MethodSymbol? constructorNoArguments,
AnalyzedArguments analyzedArguments,
BindingDiagnosticBag diagnostics)
{
//
// !!! ATTENTION !!!
Expand All @@ -1144,10 +1183,9 @@ internal BoundExpression BindCollectionExpressionConstructor(SyntaxNode syntax,
//

BoundExpression collectionCreation;
var analyzedArguments = AnalyzedArguments.GetInstance();
if (targetType is NamedTypeSymbol namedType)
{
var binder = new ParamsCollectionTypeInProgressBinder(namedType, this, constructor);
var binder = new ParamsCollectionTypeInProgressBinder(namedType, this, constructorNoArguments);
collectionCreation = binder.BindClassCreationExpression(syntax, namedType.Name, syntax, namedType, analyzedArguments, diagnostics);
collectionCreation.WasCompilerGenerated = true;
}
Expand All @@ -1160,7 +1198,6 @@ internal BoundExpression BindCollectionExpressionConstructor(SyntaxNode syntax,
{
throw ExceptionUtilities.UnexpectedValue(targetType);
}
analyzedArguments.Free();
return collectionCreation;
}

Expand Down Expand Up @@ -1787,6 +1824,7 @@ private BoundCollectionExpression BindCollectionExpressionForErrorRecovery(
keyValuePairElement.Update(
BindToNaturalType(keyValuePairElement.Key, diagnostics, reportNoTargetType),
BindToNaturalType(keyValuePairElement.Value, diagnostics, reportNoTargetType)),
BoundUnconvertedCollectionArguments collectionArguments => bindToNaturalType(collectionArguments, diagnostics, reportNoTargetType),
_ => BindToNaturalType((BoundExpression)element, diagnostics, reportNoTargetType)
};
builder.Add(result);
Expand All @@ -1805,6 +1843,21 @@ private BoundCollectionExpression BindCollectionExpressionForErrorRecovery(
targetType,
hasErrors: true)
{ WasCompilerGenerated = node.IsParamsArrayOrCollection, IsParamsArrayOrCollection = node.IsParamsArrayOrCollection };

BoundUnconvertedCollectionArguments bindToNaturalType(BoundUnconvertedCollectionArguments collectionArguments, BindingDiagnosticBag diagnostics, bool reportNoTargetType)
{
var arguments = collectionArguments.Arguments;
var builder = ArrayBuilder<BoundExpression>.GetInstance(arguments.Length);
foreach (var argument in arguments)
{
builder.Add(BindToNaturalType(argument, diagnostics, reportNoTargetType));
}
return collectionArguments.Update(
builder.ToImmutableAndFree(),
collectionArguments.ArgumentNamesOpt,
collectionArguments.ArgumentRefKindsOpt,
collectionArguments.Binder);
}
}

internal void GenerateImplicitConversionErrorForCollectionExpression(
Expand Down Expand Up @@ -1861,6 +1914,9 @@ internal void GenerateImplicitConversionErrorForCollectionExpression(
{
switch (element)
{
case BoundUnconvertedCollectionArguments:
// Collection arguments do not affect convertibility.
break;
case BoundCollectionExpressionSpreadElement spreadElement:
{
var enumeratorInfo = spreadElement.EnumeratorInfoOpt;
Expand Down Expand Up @@ -1928,11 +1984,8 @@ void generateImplicitConversionError(
TypeSymbol elementTypeOriginalDefinition,
TypeSymbol? builderType,
string? methodName,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo,
out Conversion returnTypeConversion)
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
returnTypeConversion = default;

if (!SourceNamedTypeSymbol.IsValidCollectionBuilderType(builderType))
{
return null;
Expand Down Expand Up @@ -2004,7 +2057,6 @@ void generateImplicitConversionError(
}

useSiteInfo.AddDiagnostics(candidateUseSiteInfo.Diagnostics);
returnTypeConversion = conversion;
return method;
}

Expand Down
Loading