Collection expressions: IOperation and Add method with params array #72469

cston · 2024-03-08T23:39:49Z

For a collection with an Add(params T[]) method, Binder.GetUnderlyingCollectionExpressionElement was returning the array rather than the element in the array.

Fixes #72461

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

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

RikkiGibson · 2024-03-09T01:25:46Z

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

+
+            comp = CreateCompilation([sourceB2, sourceA, s_collectionExtensions], options: TestOptions.ReleaseExe);
+            comp.VerifyEmitDiagnostics();
+            CompileAndVerify(comp, expectedOutput: "[], ");


I expected the output here to be similar to the output for sourceB1. Am I missing something about the semantics of the respective expressions?

In sourceB1, the Add(params MyCollection[]) call for x in [x, ..y] is bound in expanded form, that is we call the method with Add(new MyCollection[] { x }).

In sourceB2, the call for [] in [[]] is bound in normal form, that is the [] is treated as an empty array rather than a MyCollection instance and we call Add(new MyCollection[0]).

I've added Console.WriteLine() calls to hopefully indicate this.

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

meant to comment not approve yet

AlekseyTs · 2024-03-09T03:59:10Z

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

+            {
+                int argIndex = collectionInitializer.InvokedAsExtensionMethod ? 1 : 0;
+                var arg = collectionInitializer.Arguments[argIndex];
+                if (collectionInitializer.Expanded && collectionInitializer.AddMethod.Parameters[argIndex].IsParams)


&& collectionInitializer.AddMethod.Parameters[argIndex].IsParams

I think we should remove this check. At best it is redundant. because overload resolution already checked for that. At worse, it will lead to the same bug when the collectionInitializer.AddMethod is an override that doesn't have ParamArray attribute (overload resolution checks for it on the least derived declaration). The collectionInitializer.Expanded condition is sufficient in my opinion. #Closed

It's necessary to check that the argIndex refers to the params parameter even if the collectionInitializer.Expanded to allow for the case where there is a parameter before the params parameter (see Add_ParamsArray_04):

public void Add(T x, params T[] y) { ... }

It looks like the ParamArray attribute is added to overrides, at least for overrides declared in C#. But I've changed the check here to avoid using IsParams.

It looks like the ParamArray attribute is added to overrides, at least for overrides declared in C#.

What is important is the fact that the presence of the attribute on overrides is not required for the method to be used in expanded form. And metadata like that can easily be created.

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

AlekseyTs · 2024-03-09T04:07:59Z

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

+                    public static void Add<T>(this ICollection<T> collection, params T[] elements)
+                    {
+                        foreach (T element in elements)
+                            collection.Add(element);


Add

Just curious, what method is called here? #Closed

The implementation of ICollection<T>.Add(T) is called on the collection instance.

The implementation of ICollection<T>.Add(T) is called on the collection instance.

If that method is available and it is preferred over params version here, how do we end up using params method for collection expression?

The ICollection<KeyValuePair<K, V>>.Add() method on Dictionary<K, V> is ignored in the collection expression because it is an explicit implementation.

The ICollection<KeyValuePair<K, V>>.Add() method on Dictionary<K, V> is ignored in the collection expression because it is an explicit implementation.

In other words, there is no suitable Add method on Dictionary<K, V>, we start looking for an extension on Dictionary<K, V> and find one that extends ICollection<KeyValuePair<K, V>>. We never look for Add on ICollection<KeyValuePair<K, V>>. Is this an accurate description of what is going on?

Yes, that's correct.

AlekseyTs · 2024-03-09T04:16:21Z

Done with review pass (commit 2)

AlekseyTs · 2024-03-09T04:17:52Z

@cston Consider leaving the bug active and assigning it to me when this PR is merged. I will make sure similar scenarios with param collections are working properly.

AlekseyTs · 2024-03-11T16:00:54Z

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

+                class MyCollection : MyCollectionBase, IEnumerable<object>
+                {
+                    private List<object> _list = new();
+                    public override void Add(object[] x) => _list.AddRange(x);


public override void Add(object[] x) => _list.AddRange(x);

It doesn't look like this test targets an interesting scenario when the override doesn't have ParamArray attribute. Consider adding a test for this scenario as well. The situation can be achieved by placing abstract and override methods in different assemblies and adding params modifier to the abstract method after the override method is compiled. #Closed

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

AlekseyTs · 2024-03-11T16:13:10Z

Done with review pass (commit 4)

AlekseyTs · 2024-03-11T21:08:59Z

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

+                }
+                """;
+            string assemblyName = GetUniqueName();
+            var comp = CreateCompilation(new AssemblyIdentity(assemblyName, new Version(1, 0, 0, 0)),  sourceA1, references: TargetFrameworkUtil.StandardReferences);


var comp = CreateCompilation(new AssemblyIdentity(assemblyName, new Version(1, 0, 0, 0)), sourceA1, references: TargetFrameworkUtil.StandardReferences);

It looks like correctness leg complains about formatting issue around this line.

AlekseyTs

LGTM (commit 6)

Collection expressions: IOperation and Add method with params array

8d8779a

cston requested a review from a team as a code owner March 8, 2024 23:39

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

cston marked this pull request as draft March 8, 2024 23:50

Add test

d37a755

cston marked this pull request as ready for review March 9, 2024 00:02

RikkiGibson self-assigned this Mar 9, 2024

RikkiGibson previously approved these changes Mar 9, 2024

View reviewed changes

AlekseyTs reviewed Mar 9, 2024

View reviewed changes

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

AlekseyTs reviewed Mar 9, 2024

View reviewed changes

cston added 2 commits March 11, 2024 00:50

PR feedback

5abf704

Replace check for IsParams

67ebbeb

AlekseyTs reviewed Mar 11, 2024

View reviewed changes

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

cston added 2 commits March 11, 2024 13:10

Update override test

362866b

Update tests

4c5b402

AlekseyTs reviewed Mar 11, 2024

View reviewed changes

Formatting

720a7b4

AlekseyTs approved these changes Mar 11, 2024

View reviewed changes

Update tests

2f2fe7b

333fred approved these changes Mar 11, 2024

View reviewed changes

cston merged commit 94c65cd into dotnet:main Mar 12, 2024
24 checks passed

dotnet-policy-service bot added this to the Next milestone Mar 12, 2024

RikkiGibson removed this from the Next milestone Mar 25, 2024

RikkiGibson added this to the 17.10 P3 milestone Mar 25, 2024

AlekseyTs mentioned this pull request Mar 26, 2024

Collection expressions: require Add method callable with a single argument for class and struct types that do not use [CollectionBuilder] #72654

Merged

cston mentioned this pull request Apr 6, 2024

CSC Compiler Crash in VS 17.9.5 (likely related to collection expressions) #72903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection expressions: IOperation and Add method with params array #72469

Collection expressions: IOperation and Add method with params array #72469

cston commented Mar 8, 2024

RikkiGibson Mar 9, 2024

cston Mar 11, 2024 •

edited

Loading

AlekseyTs Mar 9, 2024 •

edited

Loading

cston Mar 11, 2024

cston Mar 11, 2024

AlekseyTs Mar 11, 2024

AlekseyTs Mar 9, 2024 •

edited

Loading

cston Mar 11, 2024

AlekseyTs Mar 11, 2024 •

edited

Loading

cston Mar 11, 2024

AlekseyTs Mar 11, 2024

cston Mar 11, 2024

AlekseyTs commented Mar 9, 2024

AlekseyTs commented Mar 9, 2024

AlekseyTs Mar 11, 2024 •

edited

Loading

AlekseyTs commented Mar 11, 2024

AlekseyTs Mar 11, 2024

AlekseyTs left a comment

Collection expressions: IOperation and Add method with params array #72469

Collection expressions: IOperation and Add method with params array #72469

Conversation

cston commented Mar 8, 2024

Choose a reason for hiding this comment

cston Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 9, 2024 • 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

AlekseyTs Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 11, 2024 • 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

AlekseyTs commented Mar 9, 2024

AlekseyTs commented Mar 9, 2024

AlekseyTs Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 11, 2024

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston Mar 11, 2024 •

edited

Loading

AlekseyTs Mar 9, 2024 •

edited

Loading

AlekseyTs Mar 9, 2024 •

edited

Loading

AlekseyTs Mar 11, 2024 •

edited

Loading

AlekseyTs Mar 11, 2024 •

edited

Loading