Collection expressions: require applicable constructor and Add method for target type implementing IEnumerable #71592

cston · 2024-01-11T23:41:48Z

See corresponding spec changes dotnet/csharplang#7802 and dotnet/csharplang#7873.

Relates to test plan #66418

… for target type implementing IEnumerable

cston · 2024-01-12T07:37:57Z

                public void Add(ref readonly T t) { _list.Add(t); }

Is it expected that an Add(ref readonly T t) method can be used implicitly for collection initializers or collection expressions?

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

cston · 2024-01-12T07:39:51Z

            Diagnostic(ErrorCode.ERR_BadArgTypesForCollectionAdd, "[(IA)null, (IB)null, new AB()]").WithArguments("C.Add(IA)").WithLocation(15, 15),

We should consider reporting a specific error for collection expressions: "no applicable Add method that can be called with argument of type 'T'". #Resolved

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

cston · 2024-01-12T07:44:27Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs

@@ -183,6 +182,19 @@ protected override Conversion GetInterpolatedStringConversion(BoundExpression so

            Debug.Assert(elementType is { });

+            if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerable)


if

Need to update breaking change document with the additional conversion requirements: an accessible .ctor callable with no arguments, and an accessible Add method callable with an argument of the iteration type.

For instance, Dictionary<string, object> d = []; is no longer supported, and collection expressions cannot be converted to string. #Resolved

cston · 2024-01-12T07:50:45Z

    public void ListBase()

Removed test since the diagnostics have changed (ListBase<T> does not have the required Add(T) method), and it's not clear what this is testing other than the required `Add(T) method. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:3756 in 1ad210c. [](commit_id = 1ad210c, deletion_comment = True)

AlekseyTs · 2024-01-12T20:39:20Z

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

+                {
+                    if (!HasCollectionExpressionApplicableConstructor(node.Syntax, targetType, diagnostics))
+                    {
+                        reportedErrors = true;


reportedErrors = true;

Are we assuming a general error "No conversion" has been reported elsewhere? #Closed

If so, consider adding a comment pointing to the code doing that

AlekseyTs · 2024-01-12T21:04:00Z

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

+
+                    if (!HasCollectionExpressionApplicableAddMethod(node.Syntax, targetType, elementType, diagnostics))
+                    {
+                        reportedErrors = true;


reportedErrors = true;

It looks like we end up here for ListInterfaces_01 unit-test and report errors that HasCollectionExpressionApplicableAddMethod reports. I think that the errors look confusing. Consider dropping the diagnostics and reporting a dedicated error instead. Something like "conversion from collection expression to type '{targetType}' doesn't exist because there is no accessible method that accepts value of type'{elementType}'" #Closed

Dependencies can be dropped too, since we are reporting an error. Accurate tracking of dependencies is not supported for error scenarios

Perhaps we can report "error CS1061: '{targetType}' does not contain a definition for 'Add' and no accessible extension method 'Add' accepting a first argument of type 'string' could be found (are you missing a using directive or an assembly reference?)" when HasCollectionExpressionApplicableAddMethod doesn't report that itself.

It is quite possible that we only need to worry when HasCollectionExpressionApplicableAddMethod reports "error CS1950: The best overloaded Add method '{bestCandidate}' for the collection initializer has some invalid arguments"

AlekseyTs · 2024-01-12T21:05:45Z

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

+                {
+                    if (!HasCollectionExpressionApplicableConstructor(node.Syntax, targetType, diagnostics))
+                    {
+                        reportedErrors = true;


reportedErrors = true;

Similar suggestion about reporting dedicated error instead. #Closed

On the other hand, if "error CS1729: '{targetType}' does not contain a constructor that takes 0 arguments" and the like are the only errors that we can get from HasCollectionExpressionApplicableConstructor, that is probably fine too.

AlekseyTs · 2024-01-12T21:33:42Z

            // (12,13): error CS7036: There is no argument given that corresponds to the required parameter 'y' of 'S<int>.Add(int, int)'

This error also feels confusing #Closed

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

AlekseyTs · 2024-01-12T21:36:51Z

            // (13,42): error CS1954: The best overloaded method match 'MyCollection<object>.Add(out object)' for the collection initializer element cannot be used. Collection initializer 'Add' methods cannot have ref or out parameters.

This one is probably confusing too #Closed

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

AlekseyTs · 2024-01-12T21:38:32Z

                public void Add(ref readonly T t) { _list.Add(t); }

Is it expected that an Add(ref readonly T t) method can be used implicitly for collection initializers or collection expressions?

Can one call it explicitly without a modifier?

In reply to: 1888571804

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

AlekseyTs · 2024-01-12T21:45:17Z

                public void Add(ref readonly T t) { _list.Add(t); }

It looks like it is working for collection initializers, perhaps we should go along

In reply to: 1889982854

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

AlekseyTs · 2024-01-12T21:46:55Z

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

+                {
+                    if (!HasCollectionExpressionApplicableConstructor(node.Syntax, targetType, diagnostics))
+                    {
+                        reportedErrors = true;


reportedErrors = true;

Are we covering this code path for a type parameter? #Closed

Yes, there are several tests that hit this with a type parameter, including TypeParameter_03.

AlekseyTs · 2024-01-12T21:48:17Z

            // 0.cs(11,13): error CS0012: The type 'A2' is defined in an assembly that is not referenced. You must add a reference to assembly 'a897d975-a839-4fff-828b-deccf9495adc, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.

It feels like this information is useful for a user #Closed

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

AlekseyTs · 2024-01-12T21:54:51Z

            // (4,7): error CS7036: There is no argument given that corresponds to the required parameter 's' of 'C.C(string)'

This is probably confusing #Closed

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

AlekseyTs · 2024-01-12T21:58:31Z

            // (6,90): error CS0304: Cannot create an instance of the variable type 'T1' because it does not have the new() constraint

This is probably confusing too #Closed

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

AlekseyTs · 2024-01-12T22:10:07Z

Done with review pass (commit 3). I think the error reporting should be improved. At the moment, I am not sure about the implementation strategy for that. It is quite possible that the most reliable way is to have overload resolution report errors in a special mode for the two interesting call sites. Basically never report specific confusing errors and report other errors instead.

cston · 2024-01-18T21:29:44Z

            // 1.cs(6,23): error CS1503: Argument 1: cannot convert from 'object' to 'int'

Consider adjusting the error message to clarify that object is the iteration type

Thanks. I'll consider potentially for a subsequent change. Presumably, it would require checking for BinderFlags.CollectionExpressionConversionValidation in OverloadResolutionResult.ReportBadArgumentError().

In reply to: 1899196694

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

AlekseyTs

LGTM (commit 13)

jcouv · 2024-01-23T19:40:09Z

            // 1.cs(6,23): error CS1503: Argument 1: cannot convert from 'object' to 'int'

Sorry, I realize it wouldn't help here, but I was proposing to modify the error message you're adding:
"Collection expression type must have an applicable instance or extension method 'Add' that can be called with an argument of iteration type '{0}'"

In reply to: 1899232311

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

jcouv

LGTM Thanks (iteration 13)

AlekseyTs

LGTM (commit 15), I assume more changes are coming to this PR

jcouv

LGTM Thanks (iteration 15)

AlekseyTs · 2024-01-25T22:58:48Z

@cston It looks like there are legitimate test failures

AlekseyTs · 2024-01-25T23:06:01Z

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

+
+        internal bool HasCollectionExpressionApplicableAddMethod(SyntaxNode syntax, TypeSymbol targetType, TypeSymbol elementType, ImmutableArray<BoundNode> elements, BindingDiagnosticBag diagnostics)
+        {
+            if (elements.Length == 0)


if (elements.Length == 0)

Consider adjusting the two call sites instead. For example, the check for an Add method will be applicable to params parameters, but there won't be any list of Items to pass and to have to fake them feels like an overkill. #Closed

AlekseyTs · 2024-01-25T23:59:22Z

Do we have any success test case for an empty collection expression without an Add method? #Closed

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

Done with review pass (commit 18)

AlekseyTs · 2024-01-26T01:07:09Z

Done with review pass (commit 19)

AlekseyTs

LGTM (commit 20)

jcouv

LGTM Thanks (iteration 20)

- dotnet/roslyn#71592

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 11, 2024

Collection expressions: require applicable constructor and Add method…

ec6908b

… for target type implementing IEnumerable

cston force-pushed the check-add branch from fc0928c to ec6908b Compare January 12, 2024 02:27

More tests

1ad210c

cston commented Jan 12, 2024

View reviewed changes

cston marked this pull request as ready for review January 12, 2024 07:59

cston requested review from a team as code owners January 12, 2024 07:59

Add breaking change description

b6491c6

AlekseyTs reviewed Jan 12, 2024

View reviewed changes

cston added 2 commits January 16, 2024 16:07

Add IDE tests

6d4f78b

Merge remote-tracking branch 'upstream/main' into check-add

008a562

jaredpar added this to the 17.10 milestone Jan 17, 2024

cston added 2 commits January 17, 2024 14:42

Add specific error for Add method

3dabaf1

Merge remote-tracking branch 'upstream/main' into check-add

0550695

Merge remote-tracking branch 'upstream/main' into check-add

86eb437

AlekseyTs approved these changes Jan 23, 2024

View reviewed changes

jcouv approved these changes Jan 23, 2024

View reviewed changes

cston added 2 commits January 24, 2024 13:47

Update error message

a59cfe2

Merge remote-tracking branch 'upstream/main' into check-add

548d410

AlekseyTs previously approved these changes Jan 25, 2024

View reviewed changes

jcouv approved these changes Jan 25, 2024

View reviewed changes

cston added 3 commits January 25, 2024 11:52

Require Add method for non-empty collections only

46bd28d

Merge remote-tracking branch 'upstream/main' into check-add

4faea26

Update breaking change note

7bb2741

AlekseyTs reviewed Jan 25, 2024

View reviewed changes

Update tests

2168070

Address feedback

e15911c

AlekseyTs approved these changes Jan 26, 2024

View reviewed changes

jcouv approved these changes Jan 26, 2024

View reviewed changes

cston merged commit cd90f6f into dotnet:main Jan 26, 2024
31 checks passed

cston deleted the check-add branch January 26, 2024 18:36

ghost modified the milestones: 17.10, Next Jan 26, 2024

Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024

Tanya-Solyanik mentioned this pull request Feb 5, 2024

Add extension function for DesignerActionItemCollection dotnet/winforms#10819

Merged

halter73 added a commit to dotnet/aspnetcore that referenced this pull request Feb 12, 2024

Work around breaking change to collection exrpressions

7728089

- dotnet/roslyn#71592

cston mentioned this pull request Apr 5, 2024

string.Concat behavior difference in recent preview #72883

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expressions: require applicable constructor and Add method for target type implementing IEnumerable #71592

Collection expressions: require applicable constructor and Add method for target type implementing IEnumerable #71592

cston commented Jan 11, 2024 •

edited

Loading

cston commented Jan 12, 2024

cston commented Jan 12, 2024 •

edited

Loading

cston Jan 12, 2024 •

edited

Loading

cston commented Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024

AlekseyTs Jan 12, 2024

AlekseyTs Jan 12, 2024

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024

AlekseyTs commented Jan 12, 2024

AlekseyTs Jan 12, 2024 •

edited

Loading

cston Jan 17, 2024

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024

cston commented Jan 18, 2024

AlekseyTs left a comment

jcouv commented Jan 23, 2024

jcouv left a comment

AlekseyTs left a comment

jcouv left a comment

AlekseyTs commented Jan 25, 2024

AlekseyTs Jan 25, 2024 •

edited

Loading

AlekseyTs commented Jan 25, 2024 •

edited

Loading

AlekseyTs commented Jan 26, 2024

AlekseyTs left a comment

jcouv left a comment

		@@ -183,6 +182,19 @@ protected override Conversion GetInterpolatedStringConversion(BoundExpression so

		Debug.Assert(elementType is { });

		if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerable)

Collection expressions: require applicable constructor and Add method for target type implementing IEnumerable #71592

Collection expressions: require applicable constructor and Add method for target type implementing IEnumerable #71592

Conversation

cston commented Jan 11, 2024 • edited Loading

cston commented Jan 12, 2024

cston commented Jan 12, 2024 • edited Loading

cston Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

cston commented Jan 12, 2024 • edited Loading

AlekseyTs Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 12, 2024 • edited Loading

AlekseyTs commented Jan 12, 2024 • edited Loading

AlekseyTs commented Jan 12, 2024

AlekseyTs commented Jan 12, 2024

AlekseyTs Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

cston Jan 17, 2024

Choose a reason for hiding this comment

AlekseyTs commented Jan 12, 2024 • edited Loading

AlekseyTs commented Jan 12, 2024 • edited Loading

AlekseyTs commented Jan 12, 2024 • edited Loading

AlekseyTs commented Jan 12, 2024

cston commented Jan 18, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Jan 23, 2024

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 25, 2024

AlekseyTs Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 25, 2024 • edited Loading

AlekseyTs commented Jan 26, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

cston commented Jan 11, 2024 •

edited

Loading

cston commented Jan 12, 2024 •

edited

Loading

cston Jan 12, 2024 •

edited

Loading

cston commented Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs commented Jan 12, 2024 •

edited

Loading

AlekseyTs Jan 25, 2024 •

edited

Loading

AlekseyTs commented Jan 25, 2024 •

edited

Loading