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

cston · 2024-03-21T21:47:10Z

Fixes #72098

…ent for class and struct types that do not use [CollectionBuilder]

AlekseyTs · 2024-03-22T14:19:16Z

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

+                // Any trailing parameters must be optional or params.
+                for (int i = requiredLength; i < parameters.Length; i++)
+                {
+                    if (parameters[i] is { IsOptional: false, IsParams: false })


IsParams

The IsParams check is inconsistent with how overload resolution does that.

Only value on corresponding least overridden method matters

Only value on the last parameter matters.

The definition type of the least overridden method parameter matters, see OverloadResolution.IsValidParams. In fact I suggest to use that helper here #Closed

AlekseyTs · 2024-03-22T15:19:35Z

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

+                return true;
+            }
+
+            var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };


var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };

I assume the logic starting from here to the end of the method corresponds to what is happening in MethodGroupResolution Binder.BindExtensionMethod. Consider extracting this code into a local function, adding a comment referring to the "prototype" API (Binder.BindExtensionMethod), adding a comment to Binder.BindExtensionMethod stating that relevant changes there should be mirrored in the local function here. This will make it easier to make sure that necessary adjustments will be made to properly handle extension types. #Closed

AlekseyTs · 2024-03-22T15:34:50Z

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

+            var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
+
+            var lookupResult = LookupResult.GetInstance();
+            LookupInstanceMember(lookupResult, targetType, leftIsBaseReference: false, rightName: methodName, rightArity: 0, invoked: true, ref useSiteInfo);


LookupInstanceMember

Rather than trying to duplicate the lookup code, I think we should use Binder.BindInstanceMemberAccess and proceed only if we get a BoundMethodGroup back. I am thinking of a call similar to the one in HasCollectionExpressionApplicableAddMethod.makeInvocationExpression. Afterwards, I think we should pay attention to the BoundMethodGroup.SearchExtensionMethods value before looking for extensions. #Closed

AlekseyTs · 2024-03-22T15:38:09Z

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

+                    }
+                }
+
+                // Value parameter must be by value, in, or ref readonly.


// Value parameter must be by value, in, or ref readonly.

Are there ref restrictions for optional parameters? #Closed

AlekseyTs · 2024-03-22T15:43:36Z

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

+                    return false;
+                }
+
+                // If the method is generic, can the type arguments be inferred from the one or two arguments?


can the type arguments be inferred

I assume we can talk only about a possibility of inference. The check below doesn't really guarantee that the inference is going to succeed. #Closed

AlekseyTs · 2024-03-22T15:49:04Z

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

+                var typeParameters = method.TypeParameters;
+                if (typeParameters.Length > 0)
+                {
+                    var usedParameterTypes = parameters.Slice(0, requiredLength).SelectAsArray(p => p.Type);


SelectAsArray

What is the result type of Slice? Do we really need to allocate an array? Could we perhaps use an ArrayBuilder from a pool? #Closed

Changed to AsSpan(0, requiredLength).

AlekseyTs · 2024-03-22T15:50:40Z

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

+                var typeParameters = method.TypeParameters;
+                if (typeParameters.Length > 0)
+                {
+                    var usedParameterTypes = parameters.Slice(0, requiredLength).SelectAsArray(p => p.Type);


p => p.Type

The projection doesn't look necessary, we should be able to get to the type from ParameterSymbol right when it is needed #Closed

AlekseyTs · 2024-03-22T16:04:37Z

        // For purpose of conversion, we rely the existence of an Add method.

Typo? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:26813 in 442498d. [](commit_id = 442498d, deletion_comment = False)

AlekseyTs · 2024-03-22T16:05:05Z

        // For purpose of conversion, we rely the existence of an Add method.

Typo? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:26841 in 442498d. [](commit_id = 442498d, deletion_comment = False)

AlekseyTs · 2024-03-22T16:08:48Z

                private void Add(int i) { _list.Add(i is T t ? t : default); }

Do we have a test demonstrating that inaccessible instance method doesn't prevent us from finding an extension method? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:34333 in 442498d. [](commit_id = 442498d, deletion_comment = False)

AlekseyTs · 2024-03-22T16:13:01Z

                public static void Add<T>(this MyCollection<T> collection, {{refKind}} T t) { collection.__AddInternal(t); }

Are there ref restrictions for this parameter of extension method? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:34608 in 442498d. [](commit_id = 442498d, deletion_comment = False)

AlekseyTs · 2024-03-22T16:29:03Z

                public void Add(A a) => throw null;

It is probably worth adding a similar test but with type for an additional params collection parameter missing instead. It would be good to observe what effect this is going to have on conversion classification. #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:36190 in 442498d. [](commit_id = 442498d, deletion_comment = False)

AlekseyTs · 2024-03-22T16:32:53Z

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

+
+            return false;
+
+            static bool isApplicableAddMethod(MethodSymbol method, bool expectingExtensionMethod)


isApplicableAddMethod

We probably should pay attention to use-site diagnostics (ignore methods with errors), and make consumers of HasCollectionExpressionAddMethod aware of them in case of a failure. #Closed

AlekseyTs · 2024-03-22T16:36:31Z

It looks like there are legitimate test failures in the IOperation leg. #Resolved

AlekseyTs · 2024-03-22T16:36:53Z

Done with review pass (commit 1)

AlekseyTs · 2024-03-22T17:16:16Z

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

@@ -1285,6 +1285,85 @@ internal bool HasCollectionExpressionApplicableAddMethod(SyntaxNode syntax, Type
            }
        }

+        internal bool HasCollectionExpressionAddMethod(SyntaxNode syntax, TypeSymbol targetType)


HasCollectionExpressionAddMethod

I am not sure how easy it will be to come up with a scenario making importance of this obvious, but I think this helper should keep track of all dependencies (ref assemblies) that were involved on the path to success and make the caller aware of that set. I do not think the set must be explicitly narrowed, i.e. no reason to trim dependencies from "failed" attempts. Our primary concern is to not drop dependencies for "success". #Closed

AlekseyTs · 2024-03-25T14:53:18Z

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

+                switch (element)
+                {
+                    case BoundCall call:
+                        return call.Arguments[call.InvokedAsExtensionMethod ? 1 : 0];


return call.Arguments[call.InvokedAsExtensionMethod ? 1 : 0];

This logic looks suspicious. It is not obvious what are we trying to accomplish and why. It looks like BoundCollectionExpression holds onto the original UnconvertedCollectionExpression node. Could we. perhaps, use information from that node instead? Not sure what do we want to do with expressions that do not have a natural type though. #Closed

It feels like it would be better to place this special logic into CreateBoundCollectionExpressionSpreadElement, where it would make sense as an error recovery logic. It doesn't feel like it belongs in this general helper because we don't really know where the element node is coming from and whether it is appropriate to dig into it this way.

This case is to unwrap the Add() argument when bindCollectionInitializerElementAddMethod fails in overload resolution with one or more inapplicable or ambiguous Add methods. We hit this case for spreads and for regular elements.

We hit this case for spreads and for regular elements.

Thanks for the clarification. Consider adding a comment what the code is trying to accomplish and why it makes sense to do what it is doing.

AlekseyTs · 2024-03-25T15:23:32Z

Done with review pass (commit 2)

cston · 2024-03-25T19:49:02Z

                private void Add(int i) { _list.Add(i is T t ? t : default); }

Added AddMethod_Accessibility_03.

In reply to: 2015424773

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:34333 in 442498d. [](commit_id = 442498d, deletion_comment = False)

AlekseyTs

LGTM (commit 13), assuming CI is passing

cston · 2024-03-27T18:26:19Z

@dotnet/roslyn-compiler for a second review, thanks.

AlekseyTs

LGTM (commit 17)

333fred

Generally this looks good, and I don't think any of my test suggestions should block merging, so I'm signing off. But let's get those tests in in a separate PR.

333fred · 2024-03-27T22:33:45Z

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

@@ -4521,12 +4509,18 @@ static void Main()
                // (7,13): error CS1061: 'string' 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?)
                //         s = [default];
                Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "[default]").WithArguments("string", "Add").WithLocation(7, 13),
+                // (7,13): error CS9215: Collection expression type 'string' must have an instance or extension method 'Add' that can be called with a single argument.


Can we do something to suppress one of the duplicate errors here?

333fred · 2024-03-28T00:04:05Z

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

+        [Theory]
+        [InlineData("")]
+        [InlineData("in")]
+        [InlineData("ref readonly")]


Why does this work for ref readonly? I would, at the very least, expect a warning? #Resolved

Why does this work for ref readonly?

Why do you think it shouldn't?

I would, at the very least, expect a warning?

A warning where? Saying what?

Are you referring to: "warning CS9192: Argument 1 should be passed with 'ref' or 'in' keyword"

Note, we intentionally do not report it for MyCollection<C> z = new () {x}; today because there is no place to add any of the suggested keywords to make the warning to go away.

333fred · 2024-03-28T00:04:15Z

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

+        [Theory]
+        [InlineData("")]
+        [InlineData("in")]
+        [InlineData("ref readonly")]


Why does this work for ref readonly? I would, at the very least, expect a warning? #Resolved

Same response, I guess.

333fred · 2024-03-28T00:04:39Z

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

+        [Theory]
+        [InlineData("")]
+        [InlineData("in")]
+        [InlineData("ref readonly")]


Why does this work for ref readonly? I would, at the very least, expect a warning? #Resolved

Same response, I guess.

333fred · 2024-03-28T00:04:58Z

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

+        [Theory]
+        [InlineData("")]
+        [InlineData("in")]
+        [InlineData("ref readonly")]


Why does this work for ref readonly? I would, at the very least, expect a warning? #Resolved

Same response, I guess.

333fred · 2024-03-28T00:19:12Z

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

+                  .method public instance void Add(!T x, !T y)
+                  {
+                    .param [2]
+                    .custom instance void [mscorlib]System.ParamArrayAttribute::.ctor() = ( 01 00 00 00 ) 


Should we have a version with ParamCollectionAttribute? #Resolved

Should we have a version with ParamCollectionAttribute?

AddMethod_ParamCollectionAttribute_03

333fred · 2024-03-28T00:19:36Z

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

+                        int x = 1;
+                        int[] y = [2, 3];
+                        MyCollection<int> z = [x, ..y];
+                        MyCollection<int> w = new() { x };


What if we do MyCollection<int[]>?

333fred · 2024-03-28T00:23:04Z

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

        }

        [Fact]
-        public void TargetTypedElement_PublicAPI_List()
+        public void AddMethod_Extension_12_WrongThisType()


Let's have a version of this one with a variant interface: IE, the Add method is defined as an extension on IEnumerable<object>, when I have a MyCollection<string> and it implements IEnumerable<string>. #Resolved

Added AddMethod_Extension_13_WrongThisType.

333fred · 2024-03-28T00:24:02Z

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

+            comp.VerifyEmitDiagnostics(
+                // (11,36): error CS1103: The first parameter of an extension method cannot be of type 'dynamic'
+                //     public static void Add<T>(this dynamic d, T t) { d.__AddInternal(t); }
+                Diagnostic(ErrorCode.ERR_BadTypeforThis, "dynamic").WithArguments("dynamic").WithLocation(11, 36));


And what happens when we see this in IL? Will we call it from Main in source?

333fred · 2024-03-28T00:27:40Z

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

                    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
-                    private List<MyCollection?> GetList() => _list ??= new();
+                    unsafe public void Add(void* p) => throw null;


Let's have a variant where this is the closest extension method, but there's a better (safe) extension method further away. #Resolved

cston · 2024-03-28T05:03:11Z

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

                {
                    return Conversion.NoConversion;
                }
+
+                // PROTOTYPE: Probably should at least accumulate dependencies and propagate them in case of success.
            }



// PROTOTYPE: Probably should at least accumulate dependencies and propagate them in case of success.

Logged #72770.

AlekseyTs

LGTM (commit 18)

jjonescz · 2024-03-28T08:44:20Z

Looks like there's legitimate test failure in UsedAssemblies leg for CollectionExpressionTests.AddMethod_RefOmittedArguments (assertion failed at Microsoft.CodeAnalysis.CSharp.LocalRewriter.MakeCollectionInitializer(BoundExpression rewrittenReceiver, BoundCollectionElementInitializer initializer) in /_/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs:line 145)

AlekseyTs · 2024-03-28T12:46:14Z

Looks like there's legitimate test failure in UsedAssemblies leg for CollectionExpressionTests.AddMethod_RefOmittedArguments (assertion failed at Microsoft.CodeAnalysis.CSharp.LocalRewriter.MakeCollectionInitializer(BoundExpression rewrittenReceiver, BoundCollectionElementInitializer initializer) in /_/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs:line 145)

How the CI passed then?

jjonescz · 2024-03-28T12:53:40Z

How the CI passed then?

The test was marked as skipped in the last commit (1316830)

AlekseyTs · 2024-03-28T13:23:44Z

The assert is not specific to the UsedAssemblies leg, I think.

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

cston force-pushed the any-add branch 2 times, most recently from e2ac0e8 to 051d5ec Compare March 22, 2024 00:31

Collection expressions: require Add method callable with single argum…

442498d

…ent for class and struct types that do not use [CollectionBuilder]

cston force-pushed the any-add branch from 051d5ec to 442498d Compare March 22, 2024 04:36

cston requested review from AlekseyTs and jcouv March 22, 2024 04:38

build-analysis bot mentioned this pull request Mar 22, 2024

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

Open

3 tasks

AlekseyTs reviewed Mar 22, 2024

View reviewed changes

IOperation

88caf12

AlekseyTs reviewed Mar 25, 2024

View reviewed changes

Address feedback

726896e

cston requested review from a team as code owners March 27, 2024 14:20

AlekseyTs approved these changes Mar 27, 2024

View reviewed changes

cston added 3 commits March 27, 2024 09:54

Update repro tests

d7811fc

Update breaking change description

59bfc0d

Test additional code path

70f94fb

dynamic extension test

4330e53

AlekseyTs approved these changes Mar 27, 2024

View reviewed changes

cston requested review from 333fred and RikkiGibson March 27, 2024 20:27

333fred approved these changes Mar 28, 2024

View reviewed changes

Address some PROTOTYPE comments and PR feedback

69d499d

cston commented Mar 28, 2024

View reviewed changes

cston mentioned this pull request Mar 28, 2024

GetCollectionExpressionConversion should accumulate and propagate dependencies from HasCollectionExpressionApplicableAddMethod in the success case #72770

Open

AlekseyTs approved these changes Mar 28, 2024

View reviewed changes

cston enabled auto-merge (squash) March 28, 2024 05:32

cston disabled auto-merge March 28, 2024 05:32

cston enabled auto-merge (squash) March 28, 2024 05:33

Skip failing test

1316830

cston disabled auto-merge March 28, 2024 10:23

cston enabled auto-merge (squash) March 28, 2024 10:25

cston merged commit 711ee10 into dotnet:features/CollectionAdd Mar 28, 2024
25 of 30 checks passed

cston deleted the any-add branch March 28, 2024 11:48

dotnet-bot mentioned this pull request Apr 7, 2024

[Automated] PRs inserted in VS build main-34806.19 #72921

Closed

dotnet-bot mentioned this pull request Apr 16, 2024

[Automated] PRs inserted in VS build feature.debugger.main-34815.163 #73037

Closed


		return false;

		static bool isApplicableAddMethod(MethodSymbol method, bool expectingExtensionMethod)

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

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

Conversation

cston commented Mar 21, 2024 • edited Loading

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

cston Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 22, 2024 • edited Loading

AlekseyTs commented Mar 22, 2024 • edited Loading

AlekseyTs commented Mar 22, 2024 • edited Loading

AlekseyTs commented Mar 22, 2024 • edited Loading

AlekseyTs commented Mar 22, 2024 • edited Loading

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 22, 2024 • edited by cston Loading

AlekseyTs commented Mar 22, 2024

AlekseyTs Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 25, 2024

cston commented Mar 25, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Mar 27, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Mar 28, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Mar 28, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Mar 28, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Mar 28, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Mar 28, 2024 • 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

333fred Mar 28, 2024 • 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

333fred Mar 28, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jjonescz commented Mar 28, 2024 • edited Loading

AlekseyTs commented Mar 28, 2024

jjonescz commented Mar 28, 2024 • edited Loading

AlekseyTs commented Mar 28, 2024

cston commented Mar 21, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

cston Mar 25, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs commented Mar 22, 2024 •

edited by cston

Loading

AlekseyTs Mar 22, 2024 •

edited

Loading

AlekseyTs Mar 25, 2024 •

edited

Loading

AlekseyTs Mar 26, 2024 •

edited

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

333fred Mar 28, 2024 •

edited by cston

Loading

jjonescz commented Mar 28, 2024 •

edited

Loading

jjonescz commented Mar 28, 2024 •

edited

Loading