Add IOp support for `with(...)` elements. #81058

CyrusNajmabadi · 2025-11-05T19:53:36Z

Relates to test plan #80613
Closes #80998

…-arguments' into creationArguments

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

CyrusNajmabadi · 2025-11-05T20:31:31Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_Constructor.cs


        var operation = semanticModel.GetOperation(root.DescendantNodes().OfType<CollectionExpressionSyntax>().ToArray()[1]);
        VerifyOperationTree(compilation, operation, """
            ICollectionExpressionOperation (1 elements, ConstructMethod: MyList<System.Int32>..ctor([System.Int32 capacity = 0], [System.String name = "default"])) (OperationKind.CollectionExpression, Type: MyList<System.Int32>) (Syntax: '[with(capacity: 10), 2]')


review with whitespace off.

CyrusNajmabadi · 2025-11-05T20:32:10Z

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

        {
            EvalStackFrame frame = PushStackFrame();
+            var creationArguments = VisitArguments(operation.CreationArguments, instancePushed: false);
+            PopStackFrame(frame);


it seemed sensible to me to push/pop a frame here independent of the frame for visiting the elements. But i wasn't 100% sure. LMK if this is correct.

I'm not sure what the decision to keep the frames for the arguments and elements "independent" versus nested like in VisitObjectCreation could impact.

Maybe Aleksey can advise. Otherwise this does need some more research
I suspect we can do await with the split

note: my intent was to match VisitObjectCreation.

note: my intent was to match VisitObjectCreation.

Thanks for the clarification
In that case, would it be an outer frame with an inner frame (push, push, pop, pop) instead of two separate frames (push, pop, push, pop)?

Maybe Aleksey can advise.

See my other comments

CyrusNajmabadi · 2025-11-05T20:33:10Z

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

        </summary>
      </Comments>
    </Property>
+    <Property Name="CreationArguments" Type="ImmutableArray&lt;IArgumentOperation&gt;">


i went with CreationArguments as i think it's nicely 'generic'. They are the arguments that control the 'creation' of the collection. I didn't really want to be very explicit that they were the 'WithElement' arguments. But i don't feel strongly on this. If people think it should align more closely with the syntax, that's ok with me.

It feels like ConstructArguments to emphasize the connection with ConstructMethod would be reasonable here. However, I also don't feel strongly. :)

CyrusNajmabadi · 2025-11-05T20:33:43Z

@RikkiGibson @jjonescz @333fred ptal.

CyrusNajmabadi · 2025-11-06T18:59:03Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_Extra.cs

-            // (12,74): warning CS8620: Argument of type 'T' cannot be used for parameter 'items' of type 'ReadOnlySpan<T?>' in 'MyCollection<T?> MyCollectionBuilder.Create<T?>(ReadOnlySpan<T?> items)' due to differences in the nullability of reference types.
-            //     static IMyCollection<T?> F<T>(ReadOnlySpan<T> items, T arg) => [with(arg), ..items];
-            Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "arg").WithArguments("T", "System.ReadOnlySpan<T?>", "items", "MyCollection<T?> MyCollectionBuilder.Create<T?>(ReadOnlySpan<T?> items)").WithLocation(12, 74));
+            Diagnostic(ErrorCode.ERR_BadCollectionArgumentsArgCount, "with(arg)").WithArguments("Create", "1").WithLocation(12, 69));


note: this now matches CollectionBuilder_SpreadElement_BoxingConversion_A, which also has no nullable warning here. the only error we should report is the bad-with, since we only bind to (ReadOnlySpan<T> items) not (ReadOnlySpan<T> items, T arg) (since arguments can't go at builder end, they must be at the start).

jcouv · 2025-11-06T21:04:16Z

Closes #80998 ?

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

CyrusNajmabadi · 2025-11-06T21:54:42Z

Assert added in d4da134

AlekseyTs · 2025-11-11T18:34:21Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_Extra.cs

+                                ICollectionExpressionOperation (1 elements, ConstructMethod: System.Collections.Generic.List<System.Int32>..ctor(System.Int32 capacity)) (OperationKind.CollectionExpression, Type: System.Collections.Generic.IList<System.Int32>) (Syntax: '[with(Compu ... seBranch()]')
+                                  ConstructArguments(1):
+                                      IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: capacity) (OperationKind.Argument, Type: null) (Syntax: 'ComputeCapacity()')
+                                        IInvocationOperation (System.Int32 Program.ComputeCapacity()) (OperationKind.Invocation, Type: System.Int32) (Syntax: 'ComputeCapacity()')


ComputeCapacity

Note that this is evaluated after TrueBranch and FalseBranch. Should be the other way around #Closed

…Arguments

src/Compilers/Test/Core/Compilation/CompilationExtensions.cs

AlekseyTs · 2025-11-13T15:05:33Z

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

+            if (operation.ConstructArguments.Any(a => a is IArgumentOperation) && !operation.ConstructArguments.All(a => a is IArgumentOperation))
+                throw ExceptionUtilities.UnexpectedValue("Mixed argument operations and non-argument operations in ConstructArguments");
+
+            // Ugly, but necessary.  If we bound successfully, we'll have an array of IArgumentOperation.  We want to


Ugly, but necessary.

I do not find this statement useful in general, and, frankly, I do not find the code ugly. We are always trying to do what is necessary to have correct behavior for all possible scenarios.

AlekseyTs · 2025-11-13T15:09:05Z

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

+                ? PopArray(operation.ConstructArguments)
+                : ImmutableArray<IOperation>.CastUp(PopArray(arguments, RewriteArgumentFromArray));
+
+            return PopStackFrame(frame, new CollectionExpressionOperation(


return PopStackFrame(frame, new CollectionExpressionOperation(

A change for this line is unnecessary, but creates a "noise" in the diff and history. Please revert since there is no intent to change behavior here.

To clarify, this refers to the change around the following original lines:

PopStackFrame(frame); return new CollectionExpressionOperation(

I find this easier to read and reason about as it prevents anything from sneaking in between the popping of the stack frame and the returning of the final collection expr op. I would prefer to keep it in this form as i think it better represents the semantics wanted here.

AlekseyTs · 2025-11-13T15:27:18Z

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

+                    // creation methods when producing the .ConstructArguments for the ICollectionExpressionOperation.
+                    // Note: the caller will end up stripping this off when producing the ConstructArguments, so it will
+                    // not actually leak to the user.  But this ends up keeping the logic simple between that callsite
+                    // and this code which actually hits all the arguments passed along. Because we never actually


I find this approach bug-prone and, at the moment, I am not convinced that it is the right "price" for the simplification

Do you have a recommendation on an alternative approach?

Do you have a recommendation on an alternative approach?

I am assuming this is about ReadOnlySpan placeholder and leaning towards the following:

It will be cleaner (and probably simpler) to leave the node in IOperation tree

Bound tree should use a placeholder node that cannot be confused with a placeholder used for any other purpose. For example, by adding a placeholder kind to ValuePlaceholder, or by using distinct bound node type.

For that node, we probably should create IPlaceholderOperation with new PlaceholderKind, or add a whole new IOperation node

Stack spilling in CFG builder should be adjusted to to leave the placeholder on the stack unchanged

AlekseyTs · 2025-11-13T15:39:32Z

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

+                    if (boundCall.IsErroneousNode)
+                    {
+                        var array = @this.CreateFromArray<BoundNode, IOperation>(((IBoundInvalidNode)boundCall).InvalidNodeChildren);
+                        return array.WhereAsArray(o => o is not IPlaceholderOperation);


o is not IPlaceholderOperation

This looks bug-prone, theoretically we could have meaningful placeholders among the arguments. If not now, then in the future.

AlekseyTs · 2025-11-13T15:48:34Z

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

+                if (collectionCreation is null)
+                    return [];
+
+                Debug.Fail("Unhandled case: " + collectionCreation.GetType());


Debug.Fail("Unhandled case: " + collectionCreation.GetType());

Please add Validate method to the BoundCollectionExpression node by adding HasValidate="true" attribute in BoundNodes.xml, and assert possible values that could be in CollectionCreation according to this method.

Nice. 7bdad0d

AlekseyTs · 2025-11-13T15:59:36Z

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

+
+                    // With a CollectionBuilder, the last argument will be a placeholder where the .Elements will go.
+                    // We do *not* want to include that information in the Arguments we return.
+                    Debug.Assert(arguments is [.., IArgumentOperation { Value: IPlaceholderOperation { PlaceholderKind: PlaceholderKind.Unspecified } }], "We should always have at least one argument (the placeholder elements).");


IPlaceholderOperation

I assume this is to account for presence of ```CollectionBuilderElementsPlaceholder``. Consider asserting in Validate when and where do we expect it to be found

SGTM: 9727a59

AlekseyTs · 2025-11-13T16:28:06Z

                Span<int> span = [with(in x), 1, 2, 3];

It will be interesting to check the shape of IOperation tree for this collection expression and see how x is reflected in it.

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_ArraysAndSpans.cs:578 in b21f43c. [](commit_id = b21f43c, deletion_comment = False)

CyrusNajmabadi · 2025-11-13T16:34:23Z

7ce3cef

…

On Thu, Nov 13, 2025 at 5:28 PM AlekseyTs ***@***.***> wrote: *AlekseyTs* left a comment (dotnet/roslyn#81058) <#81058 (comment)> Span<int> span = [with(in x), 1, 2, 3]; It will be interesting to check the shape of IOperation tree for this collection expression and see how x is reflected in it. ------------------------------ Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_ArraysAndSpans.cs:578 in b21f43c <b21f43c>. [](commit_id = b21f43c <b21f43c>, deletion_comment = False) — Reply to this email directly, view it on GitHub <#81058 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MY5QIPZ3VGG5TKPJWWT34SWS3AVCNFSM6AAAAACLIBKJ32VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMRYGYZTANRRGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

AlekseyTs · 2025-11-13T16:38:20Z

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_IOperation.cs

+namespace Microsoft.CodeAnalysis.CSharp.UnitTests;
+
+[CompilerTrait(CompilerFeature.CollectionExpressions)]
+public sealed class CollectionExpressionTests_WithElement_IOperation : CSharpTestBase


CollectionExpressionTests_WithElement_IOperation

Please move this file to Microsoft.CodeAnalysis.CSharp.IOperation.UnitTests project. It is fine to add IOperation/CFG verification to some of the existing tests to assert more for those scenarios. However, we prefer all tests with the sole purpose to verify IOperation/CFG to be placed to the dedicated test project.

AlekseyTs · 2025-11-13T16:42:45Z

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

                        ErrorCode.ERR_CollectionArgumentsNotSupportedForType,
                        _node.WithElement.Syntax.GetFirstToken().GetLocation(),
                        _targetType);
+                    return null;


return null;

We should have a test verifying what happens with WithElement in IOperation tree when we hit this code path.

AlekseyTs · 2025-11-13T16:43:09Z

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

+                    // For the read-only array interfaces (IEnumerable<E>, IReadOnlyCollection<E>, IReadOnlyList<E>), only
+                    // the parameterless `with()` is allowed.
+                    _diagnostics.Add(ErrorCode.ERR_CollectionArgumentsMustBeEmpty, _node.WithElement.Syntax.GetFirstToken().GetLocation());
+                    return null;


return null;

We should have a test verifying what happens with WithElement in IOperation tree when we hit this code path.

CyrusNajmabadi · 2025-11-13T16:43:14Z

7a2b9fc

…

On Thu, Nov 13, 2025 at 5:38 PM AlekseyTs ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_IOperation.cs <#81058 (comment)>: > @@ -0,0 +1,1017 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using System.Linq; +using Microsoft.CodeAnalysis.CSharp.Test.Utilities; +using Microsoft.CodeAnalysis.Test.Utilities; +using Roslyn.Test.Utilities; +using Xunit; + +namespace Microsoft.CodeAnalysis.CSharp.UnitTests; + +[CompilerTrait(CompilerFeature.CollectionExpressions)] +public sealed class CollectionExpressionTests_WithElement_IOperation : CSharpTestBase CollectionExpressionTests_WithElement_IOperation <http://example.com/codeflow?start=20&length=48> Please move this file to Microsoft.CodeAnalysis.CSharp.IOperation.UnitTests project. It is fine to add IOperation/CFG verification to some of the existing tests to assert more for those scenarios. However, we prefer all tests with the sole purpose to verify IOperation/CFG to be placed to the dedicated test project. — Reply to this email directly, view it on GitHub <#81058 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MY6UA3HPROGDGK3MZVD34SXZRAVCNFSM6AAAAACLIBKJ32VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTINRQGY4DSMBWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

AlekseyTs · 2025-11-13T16:44:09Z

Done with review pass (commit 54), only glanced over the test changes

CyrusNajmabadi added 7 commits November 5, 2025 07:22

Add CFG and IOp

ab6f95f

In progress

47ffa73

Fixes

e26f05b

Fix

f781408

Cleanup

f489a02

Cleanup

4fb9888

Fixup test

c5f3662

dotnet-policy-service bot added VSCode Needs UX Triage Needs API Review Needs to be reviewed by the API review council labels Nov 5, 2025

Merge remote-tracking branch 'upstream/features/collection-expression…

13baeda

…-arguments' into creationArguments

CyrusNajmabadi commented Nov 5, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs Outdated Show resolved Hide resolved

CyrusNajmabadi commented Nov 5, 2025

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review November 5, 2025 20:33

CyrusNajmabadi requested review from a team as code owners November 5, 2025 20:33

CyrusNajmabadi added the Area-Compilers label Nov 5, 2025

CyrusNajmabadi added 3 commits November 5, 2025 12:34

Remove prototype

e431b33

Pass along error bit

d6078c5

Update test

d3946ca

CyrusNajmabadi commented Nov 6, 2025

View reviewed changes

jcouv added the Feature - Collection Arguments label Nov 6, 2025

333fred reviewed Nov 6, 2025

View reviewed changes

CyrusNajmabadi added 2 commits November 6, 2025 13:52

Assert syntax

51b41e2

Add assert

d4da134

CyrusNajmabadi added 2 commits November 11, 2025 17:22

Throw if we mix arguments and non-arguments

9f7cce5

add test

1db3441

AlekseyTs reviewed Nov 11, 2025

View reviewed changes

CyrusNajmabadi added 2 commits November 11, 2025 19:50

Push all values first before popping

dacba39

Merge branch 'features/collection-expression-arguments' into creation…

4772ed6

…Arguments

CyrusNajmabadi commented Nov 13, 2025

View reviewed changes

src/Compilers/Test/Core/Compilation/CompilationExtensions.cs Outdated Show resolved Hide resolved

Apply suggestion from @CyrusNajmabadi

b21f43c

AlekseyTs reviewed Nov 13, 2025

View reviewed changes

CyrusNajmabadi added 4 commits November 13, 2025 17:05

Update comment

a0b96a4

Make check more precise

83c1141

Add validation

7bdad0d

Validate more

9727a59

Add iop test

7ce3cef

AlekseyTs reviewed Nov 13, 2025

View reviewed changes

Move tests

7a2b9fc

AlekseyTs reviewed Nov 13, 2025

View reviewed changes

CyrusNajmabadi added 2 commits November 13, 2025 17:59

Add iop tests

511ca83

Add iop tests

1505fdd

This was referenced Nov 13, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

[android-arm64] The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#6408

Open

Add IOp support for with(...) elements. #81058

Are you sure you want to change the base?

Add IOp support for with(...) elements. #81058

Conversation

CyrusNajmabadi commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 10, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 6, 2025

Uh oh!

AlekseyTs Nov 11, 2025 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 13, 2025

Uh oh!

CyrusNajmabadi commented Nov 13, 2025 via email

Uh oh!

Choose a reason for hiding this comment

Add IOp support for `with(...)` elements. #81058

Add IOp support for `with(...)` elements. #81058

CyrusNajmabadi commented Nov 5, 2025 •

edited

Loading

jcouv Nov 10, 2025 •

edited

Loading

AlekseyTs Nov 11, 2025 •

edited

Loading