Collection expression arguments: initial binding and lowering #76903

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

@@ -6841,6 +6841,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="IDS_FeatureCollectionExpressions" xml:space="preserve">
    <value>collection expressions</value>
  </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

@@ -6880,6 +6883,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_CollectionExpressionKeyValuePairNotSupported" xml:space="preserve">
    <value>Collection expression type '{0}' does not support key-value pair elements.</value>
  </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:09:59Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6841,6 +6841,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="IDS_FeatureCollectionExpressions" xml:space="preserve">
    <value>collection expressions</value>
  </data>
+  <data name="IDS_FeatureCollectionExpressionArguments" xml:space="preserve">


nit: Why do we want a new feature flag? Seems IDS_FeatureDictionaryExpressions would be sufficient #ByDesign

The motivating scenario for collection arguments is dictionary key comparer, but collection arguments can be used with non-dictionary types. The additional id allows the feature to be reported "collection expression arguments" rather than "dictionary expressions" for the error reported with earlier language versions.

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

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

@@ -2360,6 +2360,9 @@ internal enum ErrorCode
        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

@@ -1958,6 +1958,13 @@
    <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.

jcouv · 2025-01-30T19:48:50Z

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


-                if (collectionCreation.HasErrors)
+                // Bind collection creation with arguments.
+                foreach (var element in elements)


It looks like we're converting the arguments in the with-element even when the with-element is not in the right position, or there are multiple. Feels like we should only convert the one that is in the right position, and do some error binding (bind to natural type?) for the error ones (but not updating collectionCreation) #ByDesign

It seems useful to me to bind with(...) as arguments to the best candidate method regardless of whether the arguments are in the first position, since presumably that is the intent.

Binding more than one with element still makes me uncomfortable. I could be convinced to bind a single with in the wrong position.
Two reasons:

Semantically, there's only one object creation in a collection expression. But presumably QuickInfo on each with might show different constructors...

When there are multiple with elements, any diagnostics reported on those in the wrong position are guaranteed to be cascading diagnostics. Do we have a test for that?
It may be good to get a second opinion and/or discuss after standup.

When there are multiple with elements, any diagnostics reported on those in the wrong position are guaranteed to be cascading diagnostics.

Overload resolution for the constructor is independent of the position of the with(), so one does not affect the other.

Do we have a test for that?

Yes, CollectionExpressionArguments.LanguageVersion_03() tests List<int> l = [with(x: 1), with(y: 2)];

I agree with Julien. I'm not comfortable binding multiple constructors for collection expressions. I'm fine with out of position with elements, but multiple constructors will have potential cascading effects, not just in diagnostics, but also in other IDE features, as well as forcing handling of multiple constructors elsewhere in our code. I think we should just turn secondary withs into BoundBadExpressions, bind the expressions for error recovery, and be done with it.

Added a PROTOTYPE comment to revisit this later, likely when implementing IOperation support.

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

333fred · 2025-02-03T19:21:34Z

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


-                if (collectionCreation.HasErrors)
+                // Bind collection creation with arguments.
+                foreach (var element in elements)


I agree with Julien. I'm not comfortable binding multiple constructors for collection expressions. I'm fine with out of position with elements, but multiple constructors will have potential cascading effects, not just in diagnostics, but also in other IDE features, as well as forcing handling of multiple constructors elsewhere in our code. I think we should just turn secondary withs into BoundBadExpressions, bind the expressions for error recovery, and be done with it.

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.

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

dotnet-issue-labeler bot 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 self-assigned this Jan 30, 2025

jcouv added the Feature - Dictionary Expressions label Jan 30, 2025

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

Draft

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expression arguments: initial binding and lowering #76903

Collection expression arguments: initial binding and lowering #76903

cston commented Jan 24, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 24, 2025 •

edited by jcouv

Loading

cston Jan 24, 2025

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

cston Jan 31, 2025

jcouv Jan 30, 2025 •

edited by cston

Loading

cston Jan 30, 2025

jcouv Jan 30, 2025

jcouv Jan 30, 2025 •

edited

Loading

cston Jan 30, 2025 •

edited

Loading

jcouv Jan 30, 2025 •

edited

Loading

jcouv Jan 30, 2025 •

edited

Loading

cston Jan 30, 2025

jcouv Jan 31, 2025

cston Feb 4, 2025

jcouv Jan 30, 2025 •

edited by cston

Loading

cston Jan 30, 2025

jcouv Jan 31, 2025 •

edited

Loading

cston Jan 31, 2025

333fred Feb 3, 2025

cston Feb 4, 2025 •

edited

Loading

jcouv left a comment

jcouv left a comment

jcouv left a comment

333fred Feb 3, 2025

333fred Feb 3, 2025 •

edited by cston

Loading

cston Feb 4, 2025 •

edited

Loading

333fred Feb 4, 2025

Collection expression arguments: initial binding and lowering #76903

Collection expression arguments: initial binding and lowering #76903

Conversation

cston commented Jan 24, 2025 • edited by jcouv Loading

CyrusNajmabadi Jan 24, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jan 24, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 24, 2025 • edited by cston Loading

Choose a reason for hiding this comment

cston Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 30, 2025 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

cston Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 30, 2025 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Feb 3, 2025 • edited by cston Loading

Choose a reason for hiding this comment

cston Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 by cston

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

jcouv Jan 30, 2025 •

edited by cston

Loading

jcouv Jan 31, 2025 •

edited

Loading

cston Feb 4, 2025 •

edited

Loading

333fred Feb 3, 2025 •

edited by cston

Loading

cston Feb 4, 2025 •

edited

Loading