Collection expressions: emit Array.Empty<T>() for [] with target type T[] or IEnumerable<T> by cston · Pull Request #69355 · dotnet/roslyn

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_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

ghost 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 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

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

Conversation

cston commented Aug 3, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sergio0694 commented Aug 3, 2023

Uh oh!

CyrusNajmabadi commented Aug 3, 2023

Uh oh!

CyrusNajmabadi Aug 3, 2023 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 3, 2023 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 3, 2023 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

cston Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

cston commented Aug 3, 2023

Uh oh!

stephentoub commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Aug 4, 2023

Uh oh!

azure-pipelines bot commented Aug 4, 2023

Uh oh!

alrz commented Aug 4, 2023

Uh oh!

CyrusNajmabadi commented Aug 4, 2023

Uh oh!

RikkiGibson Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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