Collection expression arguments: initial binding and lowering by cston · Pull Request #76903 · dotnet/roslyn

cston · 2025-01-24T04:55:47Z

Binding and lowering of collection expression arguments for class and struct target types that implement IEnumerable and do not have a [CollectionBuilder] attribute.

See proposals/collection-expression-arguments.md

Relates to test plan #76310

…s' into collection-args

CyrusNajmabadi · 2025-01-24T06:24:43Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  </data>
+  <data name="IDS_FeatureCollectionExpressionArguments" xml:space="preserve">
+    <value>collection expression arguments</value>
+  </data>


Grrrr #Closed

Yes, these two resource strings are essentially the ones you added and removed from #76003.

CyrusNajmabadi · 2025-01-24T06:25:22Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  </data>
+  <data name="ERR_CollectionArgumentsMustBeFirst" xml:space="preserve">
+    <value>Collection arguments must be the first element.</value>
+  </data>


Grr * 2.

Note i think this should be called "collection argument element" or just "'with' element. Saying the arguments (plural) must be the first element (singular) reads oddly to me. #Closed

CyrusNajmabadi · 2025-01-24T06:26:54Z

src/Compilers/CSharp/Portable/BoundTree/BoundUnconvertedCollectionArguments.cs

+{
+    internal partial class BoundUnconvertedCollectionArguments
+    {
+        internal void GetArguments(AnalyzedArguments analyzedArguments)


fwiw, havin thtis be called GetArguments and not return something reads oddly. Consider .AddArgumentsTo, which makes it much clearer that the receiver is adding the arguments to the passed in value. #ByDesign

I'd like to leave this as is. The intended use is to populate the AnalyzedArguments collection with just the arguments, rather than to add to an existing collection (we assert the collection is empty initially), so renaming to AddArgumentsTo might make the intent less clear.

Would "BuildArguments" or "CollectArguments" solve the problem?

When we support collection builder types, where the factory method has an additional, implicit ReadOnlySpan<T> elements argument, it may make sense to rename this to AddArguments() or something similar. For now though, GetArguments() seems appropriate to me.

This reverts commit 69ca245.

jcouv · 2025-01-30T19:16:19Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

        ERR_ImplicitlyTypedParamsParameter = 9272,
        ERR_VariableDeclarationNamedField = 9273,
        ERR_CollectionExpressionKeyValuePairNotSupported = 9274,
+        ERR_CollectionArgumentsMustBeFirst = 9275,


nit: You may want to leave a gap between errorcodes in main branch and those in dictionary expression branch, so that you don't have to re-number too much (hopefully only once)

jcouv · 2025-01-30T19:20:41Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                        // We only get here if the conversion of the collection expression to the
+                        // target type failed. In this case, simply visit each argument.
+                        VisitArgumentsEvaluate(collectionArguments.Arguments, collectionArguments.ArgumentRefKindsOpt, parameterAnnotationsOpt: default, defaultArguments: default);
+                        break;


Consider leaving a PROTOTYPE note or tracking a follow-up elsewhere to implement nullability analysis on happy cases (where the conversion succeeds) #Closed

Included in the test plan: #76310

jcouv · 2025-01-30T19:26:11Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

    <Field Name="Value" Type="BoundExpression"/>
  </Node>

+  <Node Name="BoundUnconvertedCollectionArguments" Base="BoundNode">


BoundUnconvertedCollectionArguments

nit: consider "BoundWithElement" since we already use the "...Element" naming convention for various collection expression elements (spread, key-value pair) and it would line up with the name of the syntax node ("WithElementSyntax")
Also, there's no "Converted" version of this node, so "Unconverted" feels odd #Closed

jcouv · 2025-01-30T19:45:47Z

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

+            MessageID.IDS_FeatureCollectionExpressionArguments.CheckFeatureAvailability(diagnostics, syntax.WithKeyword);
+
+            var arguments = AnalyzedArguments.GetInstance();
+            BindArgumentsAndNames(syntax.ArgumentList, diagnostics, arguments, allowArglist: true);


allowArglist: true

Consider adding a test to cover this #Closed

Good catch, thanks, I've added tests.

That said, we should consider not supporting __arglist.

I'd be fine not supporting :-) I just assumed you wanted anything that is supported in explicit object creation syntax to be allowed

Added an open question in dotnet/csharplang#9115.

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

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

jcouv

Done with review pass (iteration 23)

src/Compilers/CSharp/Portable/BoundTree/BoundUnconvertedCollectionArguments.cs

jcouv

Done with review pass (iteration 27)

jcouv

LGTM Thanks (iteration 28)

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

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

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

333fred · 2025-02-03T21:48:09Z

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

+                """;
+            var comp = CreateCompilation([sourceA, sourceB]);
+            comp.VerifyEmitDiagnostics(
+                // (7,13): error CS0417: 'U': cannot provide arguments when creating an instance of a variable type


I'll be honest: I've reread this error 5 times and still have no idea what it's tell me. Is the issue that we can't find an appropriate constructor? #Resolved

This is the error reported when constructing an instance of the type parameter with arguments: new U(t). sharplab.io

Good lord that error is awful.

Spolutrean · 2025-03-10T18:46:58Z

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

+            var comp = CreateCompilation([sourceA, sourceB]);
+            comp.VerifyEmitDiagnostics(
+                // (5,5): error CS9223: Creation of params collection 'MyCollection<int>' results in an infinite chain of invocation of constructor 'MyCollection<T>.MyCollection()'.
+                // c = [with(1)];


@cston I may be wrong, but is it really an error? We will pass 1 to MyCollection<T>.MyCollection(params MyCollection<T>), then will create empty instance of MyCollection<T> by parameterless constructor and just add this 1 via .Add method. If we unwrap withElement sharplab then there is no error before

cston added 8 commits January 11, 2025 21:07

Collection expression arguments

3402e2b

Initial binding and lowering

0e8e4d9

with()

0ff7500

Add CollectionExpressionArgumentTests

6346611

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

b70034e

…s' into collection-args

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

41f8519

…s' into collection-args

Update check for List<T> optimization

3059939

Collection arguments must be first

cb99c24

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 24, 2025

Fix build

fa26da0

CyrusNajmabadi reviewed Jan 24, 2025

View reviewed changes

cston added 7 commits January 23, 2025 23:08

Fix tests

a79ab2d

Fix build

ac60002

Update error text

26ee8e2

Not supported diagnostic; additional tests

a9a8efa

More tests

094a719

Add support for CollectionBuilder types

69ca245

Remove arg support for CollectionBuilder types

b41f88f

This reverts commit 69ca245.

cston force-pushed the collection-args branch from 9a04311 to b41f88f Compare January 25, 2025 22:22

cston added 6 commits January 25, 2025 14:34

Remove arg support for dictionary interface types

f545d69

More tests

64782d4

Remove arg support for CollectionBuilder types

e75432b

PROTOTYPE comments

909bd6d

Fix test

e84cfcf

PROTOTYPE comments

19d5fa0

cston force-pushed the collection-args branch from f211702 to 19d5fa0 Compare January 27, 2025 19:10

cston marked this pull request as ready for review January 27, 2025 19:11

jcouv reviewed Jan 30, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs Show resolved Hide resolved

jcouv reviewed Jan 30, 2025

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs Show resolved Hide resolved

jcouv reviewed Jan 30, 2025

View reviewed changes

cston added 4 commits January 30, 2025 13:24

Rename to BoundWithElement

3546bef

Rename to BoundWithElement

92a09f6

Test __arglist

4d2fab1

Update test

cbfcf55

cston requested a review from jcouv January 31, 2025 17:56

jcouv reviewed Jan 31, 2025

View reviewed changes

src/Compilers/CSharp/Portable/BoundTree/BoundUnconvertedCollectionArguments.cs Outdated Show resolved Hide resolved

jcouv reviewed Jan 31, 2025

View reviewed changes

Rename

c492c1d

cston mentioned this pull request Feb 1, 2025

Collection expression arguments: support [CollectionBuilder] factory methods #76998

Merged

cston requested a review from jcouv February 3, 2025 16:34

jcouv approved these changes Feb 3, 2025

View reviewed changes

333fred reviewed Feb 3, 2025

View reviewed changes

cston added 4 commits February 3, 2025 19:00

Rename to BoundCollectionExpressionWithElement

ee5a786

Rename local function

a92872b

Add PROTOTYPE comment

2d4294e

Rename

6cf1511

cston requested a review from 333fred February 4, 2025 14:49

333fred approved these changes Feb 4, 2025

View reviewed changes

cston merged commit f5224d8 into dotnet:features/dictionary-expressions Feb 4, 2025
24 checks passed

cston deleted the collection-args branch February 4, 2025 19:09

Spolutrean reviewed Mar 10, 2025

View reviewed changes

dotnet-maestro bot mentioned this pull request Jan 22, 2026

[main] Source code updates from dotnet/roslyn dotnet/dotnet#4349

Merged

dotnet-bot mentioned this pull request Jan 24, 2026

[Automated] PRs inserted in VS build main-11423.137 #82138

Closed

Conversation

cston commented Jan 24, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Jan 24, 2025 • edited by jcouv 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!

CyrusNajmabadi Jan 24, 2025 • edited by jcouv 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 Jan 24, 2025 • 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!

cston Jan 24, 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!

jcouv Jan 30, 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!

cston Jan 30, 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!

jcouv Jan 30, 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!

jcouv Jan 30, 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!

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred Feb 3, 2025 • 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!

cston Feb 4, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

cston commented Jan 24, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 24, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 24, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 24, 2025 •

edited by cston

Loading

cston Jan 24, 2025 •

edited

Loading

jcouv Jan 30, 2025 •

edited

Loading

cston Jan 30, 2025 •

edited

Loading

jcouv Jan 30, 2025 •

edited

Loading

jcouv Jan 30, 2025 •

edited

Loading

333fred Feb 3, 2025 •

edited by cston

Loading

cston Feb 4, 2025 •

edited

Loading