Collection expression arguments: support [CollectionBuilder] factory methods by cston · Pull Request #76998 · dotnet/roslyn

cston · 2025-02-01T18:23:34Z

Binding and lowering of collection expression arguments for target types that are constructed using [CollectionBuilder] factory methods.

See proposals/collection-expression-arguments.md

Relates to test plan #76310

cston · 2025-02-02T06:00:55Z

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

-                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterNotNullConstraint, "[null]").WithArguments("MyCollectionBuilder.Create<T>(System.ReadOnlySpan<T>)", "T", "string?").WithLocation(7, 28),
+                //// (7,28): warning CS8714: The type 'string?' cannot be used as type parameter 'T' in the generic type or method 'MyCollectionBuilder.Create<T>(ReadOnlySpan<T>)'. Nullability of type argument 'string?' doesn't match 'notnull' constraint.
+                //// MyCollection<string?> x1 = [null]; // 1
+                //Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterNotNullConstraint, "[null]").WithArguments("MyCollectionBuilder.Create<T>(System.ReadOnlySpan<T>)", "T", "string?").WithLocation(7, 28),


WRN_NullabilityMismatchInTypeParameterNotNullConstraint

This case was previously handled in Binder.GetAndValidateCollectionBuilderMethod() in the call to collectionBuilderMethod.CheckConstraints(). That was unintentional, because initial binding should check type constraints but not nullability constraints. Nullability constraints should be checked in NullableWalker, and the existing work item in #68786 covers that.

…lder methods

jcouv · 2025-02-18T21:53:55Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            static bool usesSingleParameterBuilderMethod(CSharpCompilation compilation, BoundCollectionExpression node, TypeWithAnnotations elementType)
+            {
+                return Binder.GetCollectionBuilderMethod(node) is { Parameters: [var parameter] } &&
+                    parameter.Type.Equals(compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T).Construct([elementType]));


parameter.Type.Equals(compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T).Construct([elementType]))

Do we have a test for the second part of the condition? I"m not sure when it would come into play #Resolved

The second check should not fail. Changed to an assert.

Also, can this either be a ReferenceEquals, or be passed some TypeCompareKind?

cston · 2025-02-18T21:53:59Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

                    case CollectionExpressionTypeKind.ImplementsIEnumerable:
-                        if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Generic_List_T, out var listElementType))
+                        if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Generic_List_T, out var listElementType) &&
+                            usesParameterlessListConstructor(_compilation, node))


usesParameterlessListConstructor(_compilation, node))

The local function usesParameterlessListConstructor() was extracted from useListOptimization() and used here, to avoid calling TryRewriteSingleElementSpreadToList() when arguments are included with a List<T> target type. See List_SingleSpread() unit test which would fail to set capacity: in [with(capacity: capacity), ..e] otherwise.

This change affects List<T> rather than collection builder types.

jcouv · 2025-02-18T21:57:55Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

        {
            Debug.Assert(!_inExpressionLambda);
            Debug.Assert(_additionalLocals is { });
-            Debug.Assert(node.CollectionCreation is null); // shouldn't have generated a constructor call


Why was the assertion removed? Looking at conversion logic, I don't see any changes affecting collectionCreation outside of collection builder scenario. #Closed

VisitArrayOrSpanCollectionExpression() is called from VisitCollectionBuilderCollectionExpression(), when generating the ReadOnlySpan<T> argument for the builder method, even though node represents the collection created from the builder in that case, rather than the argument.

jcouv

Done with review pass (iteration 20). Tests not looked at yet

333fred

Done review pass. Didn't go into the tests yet.

333fred · 2025-02-18T23:30:09Z

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

    internal partial class BoundCollectionExpressionWithElement
    {
-        internal void GetArguments(AnalyzedArguments analyzedArguments)
+        internal void AddArguments(AnalyzedArguments analyzedArguments)


Perhaps AddSelfToArguments? The current wording implies that it's modifying this, which is not happening. #Closed

Renamed to AddToArguments.

333fred · 2025-02-18T23:42:42Z

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

+                    {
+                        if (element is BoundCollectionExpressionWithElement withElement)
+                        {
+                            // PROTOTYPE: If there are multiple with() elements, should with() elements after


Eh, we have the information. Feels to me like we can be useful to users and bind it, even if it's the second thing. For duplicate withs, I'm of less strong opinion, we could possibly just make it a bad node and leave it.

333fred · 2025-02-19T02:14:37Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

-                return elements.All(canOptimizeListElement, addMethod);
+                return ctor is { } &&
+                    node.CollectionCreation is BoundObjectCreationExpression { Constructor: var objectCreate } &&
+                    ctor.Equals(objectCreate.OriginalDefinition);


Can this be a ReferenceEquals? #Closed

333fred · 2025-02-19T02:15:21Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            static bool usesSingleParameterBuilderMethod(CSharpCompilation compilation, BoundCollectionExpression node, TypeWithAnnotations elementType)
+            {
+                return Binder.GetCollectionBuilderMethod(node) is { Parameters: [var parameter] } &&
+                    parameter.Type.Equals(compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T).Construct([elementType]));


Also, can this either be a ReferenceEquals, or be passed some TypeCompareKind?

jcouv · 2025-02-20T19:01:34Z

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

-                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterNotNullConstraint, "[null]").WithArguments("MyCollectionBuilder.Create<T>(System.ReadOnlySpan<T?>)", "T", "string?").WithLocation(7, 28),
+                //// (7,28): warning CS8714: The type 'string?' cannot be used as type parameter 'T' in the generic type or method 'MyCollectionBuilder.Create<T>(ReadOnlySpan<T?>)'. Nullability of type argument 'string?' doesn't match 'notnull' constraint.
+                //// MyCollection<string?> x1 = [null];
+                //Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterNotNullConstraint, "[null]").WithArguments("MyCollectionBuilder.Create<T>(System.ReadOnlySpan<T?>)", "T", "string?").WithLocation(7, 28),


What's caused this to get worse in this PR?

See #76998 (comment).

jcouv · 2025-02-20T19:30:47Z

    public void CollectionBuilder_ParamsParameter()

Consider testing a scenario where the instance Create is not applicable or missing, but we have an extension method which is. If I recall, for collection builders, we only want instance Create methods, right? #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs:1127 in 9965c1b. [](commit_id = 9965c1b, deletion_comment = False)

cston · 2025-02-20T19:48:45Z

    public void CollectionBuilder_ParamsParameter()

Builder methods are static methods that are found from the builder type. We don't consider methods outside of the builder type.

In reply to: 2672483630

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs:1127 in 9965c1b. [](commit_id = 9965c1b, deletion_comment = False)

jcouv · 2025-02-20T19:51:57Z

Do we have any test targeting the RefSafetyAnalysis change?

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs:4114 in 9965c1b. [](commit_id = 9965c1b, deletion_comment = False)

jcouv

LGTM Thanks (iteration 27)

cston · 2025-02-20T20:21:56Z

Existing tests in CollectionExpressionTests hit the affected code paths in RefSafetyAnalysis.VisitCollectionExpression() and RefSafetyAnalysis.HasLocalScope(). But we don't have any ref safety tests for arguments though. I will add tests in a follow up PR. Thanks.

In reply to: 2672531426

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionArgumentTests.cs:4114 in 9965c1b. [](commit_id = 9965c1b, deletion_comment = False)

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

dotnet-policy-service bot added the VSCode label Feb 2, 2025

cston force-pushed the collection-builder-args branch from da3c3e3 to 6c4fc7e Compare February 2, 2025 01:13

cston commented Feb 2, 2025

View reviewed changes

cston force-pushed the collection-builder-args branch from 242f3ea to 399140a Compare February 4, 2025 17:41

Collection expression arguments: target types using [CollectionBuilder]

f826cdb

cston force-pushed the collection-builder-args branch from 399140a to f826cdb Compare February 4, 2025 19:41

build-analysis bot mentioned this pull request Feb 7, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

3 tasks

cston force-pushed the collection-builder-args branch 3 times, most recently from d8277bf to fb0b649 Compare February 7, 2025 18:59

Remove BoundCollectionExpression.CollectionBuilderMethod

c45c03a

cston force-pushed the collection-builder-args branch from fb0b649 to c45c03a Compare February 7, 2025 19:08

More tests

e6301bb

cston force-pushed the collection-builder-args branch from fd9db56 to e6301bb Compare February 8, 2025 00:49

cston added 2 commits February 8, 2025 21:07

Misc.

876d213

PROTOTYPE comments

b054ccf

cston force-pushed the collection-builder-args branch from e8f7fdd to 9c80498 Compare February 11, 2025 06:53

IsCollectionBuilderMethodCallableWithoutAdditionalArguments()

3e4b545

cston force-pushed the collection-builder-args branch from 9c80498 to 3e4b545 Compare February 11, 2025 08:09

cston added 3 commits February 11, 2025 10:35

Move methods; update tests

05960dd

Add use-site diagnostic test

196927c

(608d4f1) Collection expressions: add params to tests for invalid bui…

9525299

…lder methods

cston mentioned this pull request Feb 12, 2025

Collection expressions: add params cases to tests for invalid builder methods #77171

Closed

cston added 3 commits February 11, 2025 18:44

Update params tests

f60930c

More constraint tests

178f421

Rename parameter; update comments; remove unused local

50db4a3

cston force-pushed the collection-builder-args branch from 2030034 to 50db4a3 Compare February 12, 2025 21:47

Consider all candidates at once

cf08253

jcouv reviewed Feb 18, 2025

View reviewed changes

cston commented Feb 18, 2025

View reviewed changes

jcouv reviewed Feb 18, 2025

View reviewed changes

jcouv self-assigned this Feb 18, 2025

cston added 2 commits February 18, 2025 14:51

Address feedback

217739c

Add evaluation order test

d427eae

333fred reviewed Feb 19, 2025

View reviewed changes

cston added 5 commits February 19, 2025 10:23

Fix test

bc6f443

Interceptor test

ee36efa

Address feedback

7271bad

Rename

3ade63c

Constraints test

9965c1b

cston requested review from a team, 333fred and jcouv February 19, 2025 20:11

jcouv reviewed Feb 20, 2025

View reviewed changes

333fred approved these changes Feb 20, 2025

View reviewed changes

jcouv added the Feature - Dictionary Expressions label Feb 20, 2025

jcouv approved these changes Feb 20, 2025

View reviewed changes

cston merged commit 11a1c39 into dotnet:features/dictionary-expressions Feb 20, 2025
28 checks passed

cston deleted the collection-builder-args branch February 20, 2025 20:33

This was referenced Feb 26, 2025

Collection expression arguments: evaluate arguments before elements when calling [CollectionBuilder] factory method #77344

Draft

Collection expression arguments: ref safety tests #77381

Merged

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 Feb 1, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Feb 18, 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!

Choose a reason for hiding this comment

Uh oh!

333fred Feb 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

cston commented Feb 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cston commented Feb 1, 2025 •

edited by jcouv

Loading

cston Feb 2, 2025 •

edited

Loading

jcouv Feb 18, 2025 •

edited

Loading

cston Feb 18, 2025 •

edited

Loading

jcouv Feb 18, 2025 •

edited

Loading

333fred Feb 18, 2025 •

edited by jcouv

Loading

333fred Feb 19, 2025 •

edited by jcouv

Loading

jcouv commented Feb 20, 2025 •

edited by cston

Loading

jcouv commented Feb 20, 2025 •

edited by cston

Loading

cston commented Feb 20, 2025 •

edited

Loading