Collection expressions: align ConversionsBase.GetCollectionExpressionTypeKind() with spec #70975

cston · 2023-11-28T00:12:35Z

Update ConversionsBase.GetCollectionExpressionTypeKind() to check collection expressions for the conversions as defined in specification only.

The effect of this change is that recognizing whether to optimize List<T> and ImmutableArray<T> is now handled in lowering rather than initial binding, and the requirements for optimizing those types is tightened:

Optimizations for List<T> are no longer applied if there are dynamic elements, and
Optimizations for ImmutableArray<T> are only applied if the type has a [CollectionBuilder] attribute.

Fixes #70880

…TypeKind() with spec

AlekseyTs · 2023-11-30T18:04:13Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -1638,23 +1638,14 @@ internal static CollectionExpressionTypeKind GetCollectionExpressionTypeKind(CSh
                    return CollectionExpressionTypeKind.Array;
                }
            }


}

Shouldn't we perform more checks for multi-dimensional arrays? #Closed

Does the spec explicitly say that MD arrays are not valid targets?

MD arrays are not valid targets.

Added clarification in dotnet/csharplang#7734.

AlekseyTs · 2023-12-07T15:02:13Z

@333fred, @RikkiGibson Please review

AlekseyTs · 2023-12-11T02:04:00Z

@333fred, @RikkiGibson Please review

AlekseyTs · 2023-12-11T20:21:31Z

@333fred, @RikkiGibson Please review

AlekseyTs · 2023-12-11T22:28:26Z

@RikkiGibson For the second review

RikkiGibson · 2023-12-11T23:27:43Z

I'll try and take a look this afternoon. Thanks

RikkiGibson · 2023-12-11T23:53:40Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

@@ -128,7 +191,7 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr

                Debug.Assert(IsAllocatingRefStructCollectionExpression(node, collectionTypeKind, elementType.Type, _compilation));
                arrayType = ArrayTypeSymbol.CreateSZArray(_compilation.Assembly, elementType);
-                spanConstructor = ((MethodSymbol)_compilation.GetWellKnownTypeMember(
+                spanConstructor = ((MethodSymbol)_factory.WellKnownMember(


nit: _factory.WellKnownMethod eliminates the need to cast #ByDesign

Actually, we're using _factory.WellKnownMethod() throughout this file, and in a number of other callsites in LocalRewriter where we could cast the result to MethodSymbol. Given that, I'll leave as-is, to reduce the churn.

RikkiGibson · 2023-12-11T23:55:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -674,7 +674,7 @@ void checkConstraintLanguageVersionAndRuntimeSupportForConversion(SyntaxNode syn
            }
            else
            {
-                if ((collectionTypeKind is CollectionExpressionTypeKind.ArrayInterface or CollectionExpressionTypeKind.List) ||
+                if ((collectionTypeKind is CollectionExpressionTypeKind.ArrayInterface) ||


Do we need to check presence of these members when collectionTypeKind is CollectionInitializer? #ByDesign

We should only be using these methods for collection initializers when the target is List<T>, and in those cases, we'll bind to the methods in initial binding.

RikkiGibson · 2023-12-12T00:04:06Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                        IL_0017:  ldsfld     "System.Runtime.CompilerServices.CallSite<System.Action<System.Runtime.CompilerServices.CallSite, System.Collections.Generic.List<object>, dynamic>> Program.<>o__0.<>p__0"
+                        IL_001c:  brtrue.s   IL_005c
+                        IL_001e:  ldc.i4     0x100
+                        IL_0023:  ldstr      "Add"


Is this dynamically invoking Add? I'm not sure I understand why codegen is changing in this test. #Resolved

Yes, this is dynamically invoking Add(). (This implementation corresponds to the collection initializer approach - see sharplab.io.) We don't optimize if the elements include dynamic values.

The reason I was surprised is that the codegen before didn't appear to be optimized. Is that codegen from before actually optimized and that's why it's changing now?

Yes, the dynamic case was optimized previously.

RikkiGibson · 2023-12-12T00:06:32Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                    IL_0015:  stloc.2
+                    IL_0016:  ldloc.0
+                    IL_0017:  ldloc.2
+                    IL_0018:  callvirt   "void System.Collections.Generic.List<object>.Add(string)"


uh..which one? :) #Resolved

This uses the specialized List<object>.Add(string) rather than List<object>.Add(object).

RikkiGibson

Done review pass. Had a few minor comments and questions.

cston added 9 commits November 1, 2023 22:40

Collection expressions: IOperation support

146078f

Update API

55084d3

Fix tests

398b930

Use ArrayCreationOperation when possible

9c7794c

Simplify API

5f2379c

Use BoundCollectionInitializer for spread iterator body

5ae2a11

Update API following review

bfb8667

Collection expressions: align ConversionsBase.GetCollectionExpression…

a48b51f

…TypeKind() with spec

Merge remote-tracking branch 'origin/collections-ioperation' into 70880

debbb18

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 28, 2023

Updates

62f53e0

cston force-pushed the 70880 branch from 13a7d13 to 62f53e0 Compare November 28, 2023 03:43

Merge remote-tracking branch 'upstream/main' into 70880

e1937b3

cston marked this pull request as ready for review November 30, 2023 05:57

cston requested a review from a team as a code owner November 30, 2023 05:57

AlekseyTs reviewed Nov 30, 2023

View reviewed changes

cston requested review from 333fred, RikkiGibson and a team December 5, 2023 18:18

RikkiGibson self-assigned this Dec 8, 2023

333fred approved these changes Dec 11, 2023

View reviewed changes

RikkiGibson reviewed Dec 11, 2023

View reviewed changes

RikkiGibson reviewed Dec 12, 2023

View reviewed changes

RikkiGibson approved these changes Dec 12, 2023

View reviewed changes

cston merged commit cfba8aa into dotnet:main Dec 12, 2023

cston deleted the 70880 branch December 12, 2023 06:08

ghost added this to the Next milestone Dec 12, 2023

allisonchou mentioned this pull request Dec 12, 2023

[Automated] PRs inserted in VS build main-34412.76 #71239

Closed

Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expressions: align ConversionsBase.GetCollectionExpressionTypeKind() with spec #70975

Collection expressions: align ConversionsBase.GetCollectionExpressionTypeKind() with spec #70975

cston commented Nov 28, 2023 •

edited

Loading

AlekseyTs Nov 30, 2023 •

edited

Loading

AlekseyTs Nov 30, 2023

cston Nov 30, 2023

cston Nov 30, 2023

AlekseyTs commented Dec 7, 2023

AlekseyTs commented Dec 11, 2023

AlekseyTs commented Dec 11, 2023

AlekseyTs commented Dec 11, 2023

RikkiGibson commented Dec 11, 2023

RikkiGibson Dec 11, 2023 •

edited by cston

Loading

cston Dec 12, 2023

RikkiGibson Dec 11, 2023 •

edited by cston

Loading

cston Dec 12, 2023

RikkiGibson Dec 12, 2023 •

edited by cston

Loading

cston Dec 12, 2023 •

edited

Loading

RikkiGibson Dec 12, 2023

cston Dec 12, 2023

RikkiGibson Dec 12, 2023 •

edited by cston

Loading

cston Dec 12, 2023 •

edited

Loading

RikkiGibson left a comment

Collection expressions: align ConversionsBase.GetCollectionExpressionTypeKind() with spec #70975

Collection expressions: align ConversionsBase.GetCollectionExpressionTypeKind() with spec #70975

Conversation

cston commented Nov 28, 2023 • edited Loading

AlekseyTs Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 30, 2023

Choose a reason for hiding this comment

cston Nov 30, 2023

Choose a reason for hiding this comment

cston Nov 30, 2023

Choose a reason for hiding this comment

AlekseyTs commented Dec 7, 2023

AlekseyTs commented Dec 11, 2023

AlekseyTs commented Dec 11, 2023

AlekseyTs commented Dec 11, 2023

RikkiGibson commented Dec 11, 2023

RikkiGibson Dec 11, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Dec 12, 2023

Choose a reason for hiding this comment

RikkiGibson Dec 11, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Dec 12, 2023

Choose a reason for hiding this comment

RikkiGibson Dec 12, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Dec 12, 2023

Choose a reason for hiding this comment

cston Dec 12, 2023

Choose a reason for hiding this comment

RikkiGibson Dec 12, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

cston commented Nov 28, 2023 •

edited

Loading

AlekseyTs Nov 30, 2023 •

edited

Loading

RikkiGibson Dec 11, 2023 •

edited by cston

Loading

RikkiGibson Dec 11, 2023 •

edited by cston

Loading

RikkiGibson Dec 12, 2023 •

edited by cston

Loading

cston Dec 12, 2023 •

edited

Loading

RikkiGibson Dec 12, 2023 •

edited by cston

Loading

cston Dec 12, 2023 •

edited

Loading