Collection expressions: emit Array.Empty<T>() for [] with target type T[] or IEnumerable<T> #69355

cston · 2023-08-03T06:42:58Z

Emit Array.Empty<T>() for an empty collection expression when the target type is:

T[]
IEnumerable<T>
IReadOnlyCollection<T>
IReadOnlyList<T>

Relates to test plan #66418

… T[] or IEnumerable<T>

Sergio0694 · 2023-08-03T12:19:47Z

Question: would it make sense to emit Enumerable.Empty<T>() instead when the target is IEnumerable<T>? Wouldn't that be more efficient, in that it would avoid the allocation of the enumerator (as the returned enumerable is also an enumerator)?

CyrusNajmabadi · 2023-08-03T14:45:13Z

Does array.empty not optimize the enumerator it returns either?

CyrusNajmabadi · 2023-08-03T14:46:40Z

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

+            var collectionType = (NamedTypeSymbol)node.Type;
+            BoundExpression? arrayOrList;
+
+            // For [] with target type IEnumerable<T>, use Array.Empty<T>() rather than List<T>.


could also do for IReadOnlyList/IReadOnlyCollection. #Resolved

CyrusNajmabadi · 2023-08-03T14:47:36Z

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

+                  IL_0000:  call       "string[] System.Array.Empty<string>()"
+                  IL_0005:  newobj     "System.ReadOnlySpan<string>..ctor(string[])"
+                  IL_000a:  call       "MyCollection<string> MyCollectionBuilder.Create<string>(System.ReadOnlySpan<string>)"
+                  IL_000f:  ret
                }
                """);
            verifier.VerifyIL("Program.F1",


add test where the destination is an int*[][] this should be able to use Array.Empty<int*[]> (inner type is pointer, outer is array). #Resolved

CyrusNajmabadi · 2023-08-03T14:51:40Z

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

+                    static ICollection<string> EmptyICollection() => [];
+                    static IList<string> EmptyIList() => [];
+                    static IReadOnlyCollection<string> EmptyIReadOnlyCollection() => [];
+                    static IReadOnlyList<string> EmptyIReadOnlyList() => [];


pity about these last two. #Resolved

It looks like we are calling Array.Empty for these. Did I miss something?

Yes, these cases were added in commit 2 response to #69355 (comment).

cston · 2023-08-03T16:20:41Z

Does array.empty not optimize the enumerator it returns either?

It looks like Array.Empty<T>().GetEnumerator() returns a unique instance each time (see sharplab.io).

stephentoub · 2023-08-03T16:28:47Z

Does array.empty not optimize the enumerator it returns either?

It looks like Array.Empty<T>().GetEnumerator() returns a unique instance each time (see sharplab.io).

Array.Empty<T>()'s IEnumerable<T>.GetEnumerator returns a singleton. It does not allocate.
SharpLab

cston · 2023-08-04T15:37:42Z

/azp run roslyn-CI

azure-pipelines · 2023-08-04T15:37:53Z

Azure Pipelines successfully started running 1 pipeline(s).

alrz · 2023-08-04T22:27:57Z

Is it recommended to use [] for immutable array as well?

CyrusNajmabadi · 2023-08-04T22:46:43Z

Is it recommended to use [] for immutable array as well?

Yes.

RikkiGibson · 2023-08-08T02:41:39Z

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

+            var collectionType = (NamedTypeSymbol)node.Type;
+            BoundExpression arrayOrList;
+
+            // Use Array.Empty<T>() rather than List<T> for an empty collection expression when


I know things are in flux here but the implementation seems fine.

RikkiGibson · 2023-08-08T02:44:21Z

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

+                    static ICollection<string> EmptyICollection() => [];
+                    static IList<string> EmptyIList() => [];
+                    static IReadOnlyCollection<string> EmptyIReadOnlyCollection() => [];
+                    static IReadOnlyList<string> EmptyIReadOnlyList() => [];


It looks like we are calling Array.Empty for these. Did I miss something?

jcouv · 2023-08-09T03:27:42Z

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

+            {
+                return arrayEmpty;
+            }
+            return new BoundArrayCreation(


nit: consider adding equivalent pseudo-code // new ArrayType[0] #Closed

jcouv · 2023-08-09T03:28:43Z

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

+                    type: arrayEmpty.ReturnType);
+            }
+
+            return null;


nit: consider inverting above if #Closed

jcouv · 2023-08-09T03:31:15Z

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

-                  IL_0000:  ldc.i4.0
-                  IL_0001:  newarr     "int"
-                  IL_0006:  ret
+                  IL_0000:  call       "int[] System.Array.Empty<int>()"


Do we have a test with missing Array.Empty API? #Closed

jcouv

Done with review pass (iteration 3). Minor nits only

jcouv

LGTM Thanks (iteration 4)

…get type T[] or IEnumerable<T> (dotnet#69355)" This reverts commit d838fac.

Collection expressions: emit Array.Empty<T>() for [] with target type…

0d9c59d

… T[] or IEnumerable<T>

cston requested a review from a team as a code owner August 3, 2023 06:42

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

cston requested review from 333fred and RikkiGibson August 3, 2023 06:43

cston mentioned this pull request Aug 1, 2023

Implement optimizations for constructing collections from collection literals #68785

Open

17 tasks

CyrusNajmabadi reviewed Aug 3, 2023

View reviewed changes

cston added 2 commits August 3, 2023 09:04

Use Array.Empty<T>() for IReadOnlyCollection<T> and IReadOnlyList<T>

9444f58

Test nested array of pointers

5d2e7a1

jcouv self-assigned this Aug 4, 2023

CyrusNajmabadi mentioned this pull request Aug 5, 2023

[Proposal]: Collection expressions (VS 17.7, .NET 8) dotnet/csharplang#5354

Closed

4 tasks

RikkiGibson approved these changes Aug 8, 2023

View reviewed changes

cston requested a review from a team August 8, 2023 17:23

jcouv reviewed Aug 9, 2023

View reviewed changes

Address feedback

87cb4d6

jaredpar added the New Feature - Collection Expressions label Aug 9, 2023

jcouv approved these changes Aug 9, 2023

View reviewed changes

cston merged commit d838fac into dotnet:main Aug 9, 2023
25 checks passed

ghost added this to the Next milestone Aug 9, 2023

cston deleted the array-empty branch August 9, 2023 17:46

dibarbet added a commit to dibarbet/roslyn that referenced this pull request Aug 15, 2023

Revert "Collection expressions: emit Array.Empty<T>() for [] with tar…

f361593

…get type T[] or IEnumerable<T> (dotnet#69355)" This reverts commit d838fac.

dibarbet mentioned this pull request Aug 15, 2023

Revert "Collection expressions: emit Array.Empty<T>() for [] with tar… #69532

Closed

allisonchou mentioned this pull request Aug 16, 2023

[Automated] PRs inserted in VS build main-34016.170 #69571

Closed

RikkiGibson mentioned this pull request Aug 17, 2023

Update compiler toolset to latest #69544

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expressions: emit Array.Empty<T>() for [] with target type T[] or IEnumerable<T> #69355

Collection expressions: emit Array.Empty<T>() for [] with target type T[] or IEnumerable<T> #69355

cston commented Aug 3, 2023 •

edited by jcouv

Loading

Sergio0694 commented Aug 3, 2023

CyrusNajmabadi commented Aug 3, 2023

CyrusNajmabadi Aug 3, 2023 •

edited by cston

Loading

CyrusNajmabadi Aug 3, 2023 •

edited by cston

Loading

CyrusNajmabadi Aug 3, 2023 •

edited by cston

Loading

RikkiGibson Aug 8, 2023

cston Aug 8, 2023

cston commented Aug 3, 2023

stephentoub commented Aug 3, 2023 •

edited

Loading

cston commented Aug 4, 2023

azure-pipelines bot commented Aug 4, 2023

alrz commented Aug 4, 2023

CyrusNajmabadi commented Aug 4, 2023

RikkiGibson Aug 8, 2023 •

edited

Loading

RikkiGibson Aug 8, 2023

jcouv Aug 9, 2023 •

edited

Loading

jcouv Aug 9, 2023 •

edited

Loading

jcouv Aug 9, 2023 •

edited

Loading

jcouv left a comment

jcouv left a comment

Collection expressions: emit Array.Empty<T>() for [] with target type T[] or IEnumerable<T> #69355

Collection expressions: emit Array.Empty<T>() for [] with target type T[] or IEnumerable<T> #69355

Conversation

cston commented Aug 3, 2023 • edited by jcouv Loading

Sergio0694 commented Aug 3, 2023

CyrusNajmabadi commented Aug 3, 2023

CyrusNajmabadi Aug 3, 2023 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Aug 3, 2023 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Aug 3, 2023 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Aug 8, 2023

Choose a reason for hiding this comment

cston Aug 8, 2023

Choose a reason for hiding this comment

cston commented Aug 3, 2023

stephentoub commented Aug 3, 2023 • edited Loading

cston commented Aug 4, 2023

azure-pipelines bot commented Aug 4, 2023

alrz commented Aug 4, 2023

CyrusNajmabadi commented Aug 4, 2023

RikkiGibson Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Aug 8, 2023

Choose a reason for hiding this comment

jcouv Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

cston commented Aug 3, 2023 •

edited by jcouv

Loading

CyrusNajmabadi Aug 3, 2023 •

edited by cston

Loading

CyrusNajmabadi Aug 3, 2023 •

edited by cston

Loading

CyrusNajmabadi Aug 3, 2023 •

edited by cston

Loading

stephentoub commented Aug 3, 2023 •

edited

Loading

RikkiGibson Aug 8, 2023 •

edited

Loading

jcouv Aug 9, 2023 •

edited

Loading

jcouv Aug 9, 2023 •

edited

Loading

jcouv Aug 9, 2023 •

edited

Loading