Collection expressions: IOperation support #70650

cston · 2023-11-01T06:11:05Z

See API proposal #70631.

Relates to test plan #66418

333fred · 2023-11-20T21:09:23Z

src/Features/CSharpTest/SyncNamespace/SyncNamespaceTests_ChangeNamespace.cs

@@ -2343,7 +2343,7 @@ public async Task ChangeNamespace_DoesNotThrowInDuplicateProgramDeclaration()

            // No change namespace action because the folder name is not valid identifier
            var (topLevelProgramFolder, topLevelProgramFilePath) = CreateDocumentFilePath(["3B", "C"], "Program.cs");
-            var (duplicateProgramFolder, duplicateProgramFilePath) = CreateDocumentFilePath([], "Program.cs");
+            var (duplicateProgramFolder, _) = CreateDocumentFilePath([], "Program.cs");


Is this change related to the PR? #Resolved

Yes, with this change, an analyzer reports a diagnostic here for an unnecessary assignment.

AlekseyTs · 2023-11-22T01:25:13Z

Are we testing control flow graph with control flow constructs within a collection expression?

cston · 2023-11-22T16:15:44Z

Are we testing control flow graph with control flow constructs within a collection expression?

CollectionExpressionTests.IOperation_01 and _02.

cston · 2023-11-29T00:04:39Z

@dotnet/roslyn-compiler, please review, thanks.

jcouv · 2023-11-29T18:42:04Z

Looking

jcouv · 2023-11-29T18:47:47Z

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

+      </Comments>
+    </Property>
+  </Node>
+  <Node Name="ISpreadOperation" Base="IOperation" HasType="true">


HasType="true"

That seems unexpected. What's the type of a spread? #Resolved

Good catch, thanks.

jcouv · 2023-11-29T19:08:20Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+                operation.Type,
+                IsImplicit(operation));
+
+            static IOperation unwrapElement(IOperation element)


unwrapElement

static? #Closed

jcouv · 2023-11-29T19:09:21Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+        public override IOperation? VisitCollectionExpression(ICollectionExpressionOperation operation, int? argument)
+        {
+            EvalStackFrame frame = PushStackFrame();
+            var elements = VisitArray(operation.Elements, unwrapElement, wrapElement);


wrapElement

nit: it seems wrapElement can't be made static (it uses instance method IsImplicit). Consider using a lambda around it to cache the allocated delegate. #Resolved

Changed both local functions to lambda expressions.

jcouv · 2023-11-29T19:20:34Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

@@ -6368,6 +6368,46 @@ IArrayInitializerOperation popAndAssembleArrayInitializerValues(IArrayInitialize
            }
        }

+        public override IOperation? VisitCollectionExpression(ICollectionExpressionOperation operation, int? argument)


Looking at VisitInterpolatedStringHandlerCreation below, I would have expected the CFG for collection expression to surface the nitty-gritty details of the collection evaluation. It sounds like this was discussed/approved in API review, but I'm not clear on whether this is the level of abstraction for CFG that we want moving forward? #Closed

Resolved offline. This is the result of a case-by-case discussion in API review considering the needs of the CFG customer. Thanks

jcouv · 2023-11-29T20:21:26Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

            }
        }

-        private IOperation CreateBoundCollectionExpressionSpreadElement(BoundCollectionExpressionSpreadElement boundSpreadExpression)
+        private IOperation CreateBoundCollectionExpressionSpreadElement(BoundCollectionExpressionSpreadElement element, BoundExpression? iteratorItem)


IOperation

nit: consider declaring a more specific return type #Resolved

jcouv · 2023-11-29T20:22:57Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

            }
        }

-        private IOperation CreateBoundCollectionExpressionSpreadElement(BoundCollectionExpressionSpreadElement boundSpreadExpression)
+        private IOperation CreateBoundCollectionExpressionSpreadElement(BoundCollectionExpressionSpreadElement element, BoundExpression? iteratorItem)


Consider doc for iteratorItem #Resolved

Removed parameter.

jcouv · 2023-11-29T20:23:25Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+            }
+            return Create(getUnderlyingExpression((BoundExpression)element));
+
+            [return: NotNullIfNotNull("element")] static BoundExpression? getUnderlyingExpression(BoundExpression? element)


"element"

nameof(element)? #Resolved

jcouv · 2023-11-29T21:52:46Z

nit: consider adding a test that shows more interesting element conversions than identity or reference conversions. For instance, a collection with different Add methods where each element gets its own conversion. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:21265 in 7320176. [](commit_id = 7320176, deletion_comment = False)

jcouv

LGTM Thanks (iteration 9) with some nits to consider

cston · 2023-11-29T22:15:00Z

Added CollectionInitializerType_20 with multiple Add methods and nullable and boxing conversions.

In reply to: 1832759003

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:21265 in 7320176. [](commit_id = 7320176, deletion_comment = False)

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

cston force-pushed the collections-ioperation branch from ce50d29 to f5c7696 Compare November 1, 2023 20:57

cston added the Feature - Collection Expressions label Nov 2, 2023

Collection expressions: IOperation support

146078f

cston force-pushed the collections-ioperation branch 2 times, most recently from d1f454a to 86b01ce Compare November 2, 2023 17:38

Update API

55084d3

cston force-pushed the collections-ioperation branch 3 times, most recently from 35b30f3 to ad8cdd6 Compare November 2, 2023 21:08

Fix tests

398b930

cston force-pushed the collections-ioperation branch from ad8cdd6 to 398b930 Compare November 3, 2023 03:39

Use ArrayCreationOperation when possible

9c7794c

cston force-pushed the collections-ioperation branch 9 times, most recently from 8871fda to 65a4ae9 Compare November 15, 2023 06:28

Simplify API

5f2379c

cston force-pushed the collections-ioperation branch from 65a4ae9 to 5f2379c Compare November 15, 2023 17:35

Use BoundCollectionInitializer for spread iterator body

5ae2a11

cston force-pushed the collections-ioperation branch 3 times, most recently from 2c1626f to f3bb8d1 Compare November 19, 2023 20:08

Update API following review

bfb8667

333fred reviewed Nov 20, 2023

View reviewed changes

333fred approved these changes Nov 20, 2023

View reviewed changes

cston requested a review from a team November 20, 2023 21:45

cston mentioned this pull request Nov 28, 2023

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

Merged

jcouv self-assigned this Nov 29, 2023

jcouv reviewed Nov 29, 2023

View reviewed changes

cston added 2 commits November 29, 2023 12:27

Merge remote-tracking branch 'upstream/main' into collections-ioperation

7320176

Address feedback

f17d439

jcouv approved these changes Nov 29, 2023

View reviewed changes

Add test

4dd8285

cston merged commit 772aefd into dotnet:main Nov 30, 2023
26 of 27 checks passed

cston deleted the collections-ioperation branch November 30, 2023 00:03

ghost added this to the Next milestone Nov 30, 2023

cston mentioned this pull request Nov 30, 2023

[API proposal] Collection expressions: IOperation support #70631

Closed

allisonchou mentioned this pull request Dec 5, 2023

[Automated] PRs inserted in VS build main-34404.193 #71093

Closed

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

cston mentioned this pull request Jan 26, 2024

Initializing a ConcurrentBag with a dynamic variable using a collection initializer results in MSB6006 "csc.exe" exited with code -2146232797 #71703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expressions: IOperation support #70650

Collection expressions: IOperation support #70650

cston commented Nov 1, 2023 •

edited by jcouv

Loading

333fred Nov 20, 2023 •

edited by cston

Loading

cston Nov 20, 2023

AlekseyTs commented Nov 22, 2023

cston commented Nov 22, 2023

cston commented Nov 29, 2023

jcouv commented Nov 29, 2023

jcouv Nov 29, 2023 •

edited by cston

Loading

cston Nov 29, 2023

jcouv Nov 29, 2023 •

edited

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

cston Nov 29, 2023

jcouv Nov 29, 2023 •

edited

Loading

jcouv Nov 29, 2023

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

cston Nov 29, 2023

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv commented Nov 29, 2023 •

edited by cston

Loading

jcouv left a comment

cston commented Nov 29, 2023

Collection expressions: IOperation support #70650

Collection expressions: IOperation support #70650

Conversation

cston commented Nov 1, 2023 • edited by jcouv Loading

333fred Nov 20, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Nov 20, 2023

Choose a reason for hiding this comment

AlekseyTs commented Nov 22, 2023

cston commented Nov 22, 2023

cston commented Nov 29, 2023

jcouv commented Nov 29, 2023

jcouv Nov 29, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Nov 29, 2023

Choose a reason for hiding this comment

jcouv Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 29, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Nov 29, 2023

Choose a reason for hiding this comment

jcouv Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 29, 2023

Choose a reason for hiding this comment

jcouv Nov 29, 2023 • edited by cston Loading

Choose a reason for hiding this comment

jcouv Nov 29, 2023 • edited by cston Loading

Choose a reason for hiding this comment

cston Nov 29, 2023

Choose a reason for hiding this comment

jcouv Nov 29, 2023 • edited by cston Loading

Choose a reason for hiding this comment

jcouv commented Nov 29, 2023 • edited by cston Loading

jcouv left a comment

Choose a reason for hiding this comment

cston commented Nov 29, 2023

cston commented Nov 1, 2023 •

edited by jcouv

Loading

333fred Nov 20, 2023 •

edited by cston

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv Nov 29, 2023 •

edited

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv Nov 29, 2023 •

edited

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv Nov 29, 2023 •

edited by cston

Loading

jcouv commented Nov 29, 2023 •

edited by cston

Loading