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

Jump to bottom

Collection expressions: use iteration type for convertibility of element types #71397

Merged
merged 6 commits into from
Jan 9, 2024
Merged

Collection expressions: use iteration type for convertibility of element types #71397

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
de50319
Collection expressions: use iteration type for convertibility of elem…
cston Dec 11, 2023
389e1d6
Combine IEnumerable<T> and IEnumerable conversion cases
cston Dec 22, 2023
4802f03
Add test with GetEnumerator() pattern only
cston Dec 22, 2023
92d180f
Add assert; update test
cston Jan 9, 2024
74ed4a7
Merge remote-tracking branch 'upstream/main' into use-iteration-type
cston Jan 9, 2024
257af7b
Handle merge conflict
cston Jan 9, 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
1 change: 0 additions & 1 deletion 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 @@ -4069,7 +4069,6 @@ private bool HasLocalScope(BoundCollectionExpression expr)
}
return true;
case CollectionExpressionTypeKind.ImplementsIEnumerable:
case CollectionExpressionTypeKind.ImplementsIEnumerableT:
// Error cases. Restrict the collection to local scope.
return true;
default:
Expand Down
63 changes: 22 additions & 41 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 @@ -636,7 +636,6 @@ private BoundCollectionExpression ConvertCollectionExpression(
}
break;

case CollectionExpressionTypeKind.ImplementsIEnumerableT:
case CollectionExpressionTypeKind.ImplementsIEnumerable:
if (targetType.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_Collections_Immutable_ImmutableArray_T), TypeCompareKind.ConsiderEverything))
{
Expand All @@ -651,7 +650,7 @@ private BoundCollectionExpression ConvertCollectionExpression(
BoundExpression? collectionCreation = null;
BoundObjectOrCollectionValuePlaceholder? implicitReceiver = null;

if (collectionTypeKind is CollectionExpressionTypeKind.ImplementsIEnumerableT or CollectionExpressionTypeKind.ImplementsIEnumerable)
if (collectionTypeKind is CollectionExpressionTypeKind.ImplementsIEnumerable)
{
implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
if (targetType is NamedTypeSymbol namedType)
Expand Down Expand Up @@ -824,30 +823,33 @@ private void GenerateImplicitConversionErrorForCollectionExpression(
BindingDiagnosticBag diagnostics)
{
var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(Compilation, targetType, out TypeWithAnnotations elementTypeWithAnnotations);
if (collectionTypeKind == CollectionExpressionTypeKind.CollectionBuilder)
{
Debug.Assert(elementTypeWithAnnotations.Type is null); // GetCollectionExpressionTypeKind() does not set elementType for CollectionBuilder cases.
if (!TryGetCollectionIterationType((ExpressionSyntax)node.Syntax, targetType, out elementTypeWithAnnotations))
{
Error(diagnostics, ErrorCode.ERR_CollectionBuilderNoElementType, node.Syntax, targetType);
return;
}
Debug.Assert(elementTypeWithAnnotations.HasType);
}

var elementType = elementTypeWithAnnotations.Type;
if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerableT
&& findSingleIEnumerableTImplementation(targetType, Compilation) is { } implementation)
switch (collectionTypeKind)
{
// If we have a single IEnumerable<T> implementation, we can report conversion errors against that element type below
elementType = implementation.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type;
case CollectionExpressionTypeKind.ImplementsIEnumerable:
case CollectionExpressionTypeKind.CollectionBuilder:
Debug.Assert(elementTypeWithAnnotations.Type is null); // GetCollectionExpressionTypeKind() does not set elementType for these cases.
if (!TryGetCollectionIterationType((ExpressionSyntax)node.Syntax, targetType, out elementTypeWithAnnotations))
{
Error(
diagnostics,
collectionTypeKind == CollectionExpressionTypeKind.CollectionBuilder ?
ErrorCode.ERR_CollectionBuilderNoElementType :
ErrorCode.ERR_CollectionExpressionTargetNoElementType,
node.Syntax,
targetType);
return;
}
Debug.Assert(elementTypeWithAnnotations.HasType);
break;
}

bool reportedErrors = false;

if (collectionTypeKind != CollectionExpressionTypeKind.None &&
elementType is { })
if (collectionTypeKind != CollectionExpressionTypeKind.None)
{
var elementType = elementTypeWithAnnotations.Type;
Debug.Assert(elementType is { });

var elements = node.Elements;
var useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
foreach (var element in elements)
Expand Down Expand Up @@ -889,27 +891,6 @@ private void GenerateImplicitConversionErrorForCollectionExpression(
}

return;

static NamedTypeSymbol? findSingleIEnumerableTImplementation(TypeSymbol targetType, CSharpCompilation compilation)
{
var allInterfaces = targetType.GetAllInterfacesOrEffectiveInterfaces();
var ienumerableType = compilation.GetSpecialType(SpecialType.System_Collections_Generic_IEnumerable_T);
NamedTypeSymbol? singleIEnumerableImplementation = null;
foreach (var @interface in allInterfaces)
{
if (ReferenceEquals(@interface.OriginalDefinition, ienumerableType))
{
if (singleIEnumerableImplementation is not null)
{
return null;
}

singleIEnumerableImplementation = @interface;
}
}

return singleIEnumerableImplementation;
}
}

private MethodSymbol? GetCollectionBuilderMethod(
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/CollectionExpressionTypeKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ internal enum CollectionExpressionTypeKind
Span,
ReadOnlySpan,
CollectionBuilder,
ImplementsIEnumerableT,
ImplementsIEnumerable,
ArrayInterface,
}
Expand Down
7 changes: 4 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,18 +106,19 @@ internal DeconstructionUncommonData(DeconstructMethodInfo deconstructMethodInfoO

private sealed class CollectionExpressionUncommonData : NestedUncommonData
{
internal CollectionExpressionUncommonData(CollectionExpressionTypeKind collectionExpressionTypeKind, TypeSymbol? elementType, ImmutableArray<Conversion> elementConversions) :
internal CollectionExpressionUncommonData(CollectionExpressionTypeKind collectionExpressionTypeKind, TypeSymbol elementType, ImmutableArray<Conversion> elementConversions) :
base(elementConversions)
{
Debug.Assert(collectionExpressionTypeKind != CollectionExpressionTypeKind.None);
Copy link
Member

@jcouv jcouv Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

consider adding assertion that elementType is not null (we already have such assertion in caller, but I think it would make sense here) #Closed

CollectionExpressionTypeKind = collectionExpressionTypeKind;
ElementType = elementType;
}

internal readonly CollectionExpressionTypeKind CollectionExpressionTypeKind;
internal readonly TypeSymbol? ElementType;
internal readonly TypeSymbol ElementType;
}

internal static Conversion CreateCollectionExpressionConversion(CollectionExpressionTypeKind collectionExpressionTypeKind, TypeSymbol? elementType, ImmutableArray<Conversion> elementConversions)
internal static Conversion CreateCollectionExpressionConversion(CollectionExpressionTypeKind collectionExpressionTypeKind, TypeSymbol elementType, ImmutableArray<Conversion> elementConversions)
{
return new Conversion(
ConversionKind.CollectionExpression,
Expand Down
54 changes: 3 additions & 51 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ protected override Conversion GetCollectionExpressionConversion(
case CollectionExpressionTypeKind.None:
return Conversion.NoConversion;

case CollectionExpressionTypeKind.ImplementsIEnumerable:
case CollectionExpressionTypeKind.CollectionBuilder:
{
_binder.TryGetCollectionIterationType((Syntax.ExpressionSyntax)syntax, targetType, out elementTypeWithAnnotations);
Expand All @@ -180,31 +181,9 @@ protected override Conversion GetCollectionExpressionConversion(
break;
}

var elements = node.Elements;
if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerable)
{
return Conversion.CreateCollectionExpressionConversion(collectionTypeKind, elementType: null, default);
}
else if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerableT)
{
var allInterfaces = targetType.GetAllInterfacesOrEffectiveInterfaces();
var ienumerableType = this.Compilation.GetSpecialType(SpecialType.System_Collections_Generic_IEnumerable_T);
bool isCompatible = false;
foreach (var @interface in allInterfaces)
{
if (isCompatibleIEnumerableT(@interface, ienumerableType, elements, ref useSiteInfo))
{
isCompatible = true;
// Don't break so we collect all remaining use-site information
}
}

return isCompatible
? Conversion.CreateCollectionExpressionConversion(collectionTypeKind, elementType: null, default)
: Conversion.NoConversion;
}

Debug.Assert(elementType is { });

var elements = node.Elements;
var builder = ArrayBuilder<Conversion>.GetInstance(elements.Length);
foreach (var element in elements)
{
Expand All @@ -220,33 +199,6 @@ protected override Conversion GetCollectionExpressionConversion(

return Conversion.CreateCollectionExpressionConversion(collectionTypeKind, elementType, builder.ToImmutableAndFree());

bool isCompatibleIEnumerableT(NamedTypeSymbol targetInterface, NamedTypeSymbol ienumerableType,
ImmutableArray<BoundNode> elements, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
Debug.Assert(ienumerableType.SpecialType == SpecialType.System_Collections_Generic_IEnumerable_T);
if (!ReferenceEquals(targetInterface.OriginalDefinition, ienumerableType))
{
return false;
}

var targetElementType = targetInterface.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
return elementsCanAllConvert(elements, targetElementType.Type, ref useSiteInfo);
}

bool elementsCanAllConvert(ImmutableArray<BoundNode> elements, TypeSymbol elementType, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
foreach (var element in elements)
{
Conversion elementConversion = convertElement(element, elementType, ref useSiteInfo);
if (!elementConversion.Exists)
{
return false;
}
}

return true;
}

Conversion convertElement(BoundNode element, TypeSymbol elementType, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
return element switch
Expand Down
5 changes: 0 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1650,11 +1650,6 @@ internal static CollectionExpressionTypeKind GetCollectionExpressionTypeKind(CSh
{
return CollectionExpressionTypeKind.CollectionBuilder;
}
else if (implementsSpecialInterface(compilation, destination, SpecialType.System_Collections_Generic_IEnumerable_T))
{
elementType = default;
return CollectionExpressionTypeKind.ImplementsIEnumerableT;
}
Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline:
It feels strange for GetCollectionExpressionTypeKind to introduce a concept of "element type" in some cases.
Could we align on the concept of iteration type for all kinds of collections and add the special cases to TryGetCollectionIterationType instead, or call TryGetCollectionIterationType here and rename elementType->iterationType?
That would simplify concepts and align with the spec better.

Copy link
Member Author

@cston cston Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the suggestion. I agree, we should refactor these two methods, possibly moving calculation of iteration type to TryGetCollectionIterationType exclusively or having one method call the other. Let's refactor as necessary after completing other expected changes here.

else if (implementsSpecialInterface(compilation, destination, SpecialType.System_Collections_IEnumerable))
{
// ^ This implementation differs from Binder.CollectionInitializerTypeImplementsIEnumerable().
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6856,6 +6856,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CollectionBuilderNoElementType" xml:space="preserve">
<value>'{0}' has a CollectionBuilderAttribute but no element type.</value>
</data>
<data name="ERR_CollectionExpressionTargetNoElementType" xml:space="preserve">
<value>Collection expression target '{0}' has no element type.</value>
</data>
<data name="ERR_CollectionBuilderAttributeInvalidType" xml:space="preserve">
<value>The CollectionBuilderAttribute builder type must be a non-generic class or struct.</value>
</data>
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2281,6 +2281,7 @@ internal enum ErrorCode

ERR_InvalidExperimentalDiagID = 9211,
ERR_SpreadMissingMember = 9212,
ERR_CollectionExpressionTargetNoElementType = 9213,

#endregion

Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2410,6 +2410,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.ERR_CollectionExpressionImmutableArray:
case ErrorCode.ERR_InvalidExperimentalDiagID:
case ErrorCode.ERR_SpreadMissingMember:
case ErrorCode.ERR_CollectionExpressionTargetNoElementType:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
4 changes: 0 additions & 4 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers
switch (collectionTypeKind)
{
case CollectionExpressionTypeKind.ImplementsIEnumerable:
return VisitCollectionInitializerCollectionExpression(node, node.Type);
case CollectionExpressionTypeKind.ImplementsIEnumerableT:
if (useListOptimization(_compilation, node, out var listElementType))
{
return CreateAndPopulateList(node, listElementType, node.Elements.SelectAsArray(unwrapListElement));
Expand Down Expand Up @@ -1076,8 +1074,6 @@ private BoundExpression GetKnownLengthExpression(ImmutableArray<BoundNode> eleme
for (int i = 0; i < numberIncludingLastSpread; i++)
{
var element = elements[i];
var rewrittenExpression = rewrittenExpressions[i];

if (element is BoundCollectionExpressionSpreadElement spreadElement)
{
var collectionPlaceholder = spreadElement.ExpressionPlaceholder;
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,7 +1246,6 @@ private ICollectionExpressionOperation CreateBoundCollectionExpression(BoundColl
case CollectionExpressionTypeKind.Span:
return null;
case CollectionExpressionTypeKind.ImplementsIEnumerable:
case CollectionExpressionTypeKind.ImplementsIEnumerableT:
return (expr.CollectionCreation as BoundObjectCreationExpression)?.Constructor;
case CollectionExpressionTypeKind.CollectionBuilder:
return expr.CollectionBuilderMethod;
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
View file
Open in desktop

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
View file
Open in desktop

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
View file
Open in desktop

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
View file
Open in desktop

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf
View file
Open in desktop

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

Loading
Loading