Align `params` type validation with the the spec #71918

AlekseyTs · 2024-02-01T20:21:24Z

https://github.com/dotnet/csharplang/blob/main/proposals/params-collections.md#method-parameters

AlekseyTs · 2024-02-01T22:32:50Z

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2024-02-02T13:35:58Z

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2024-02-05T16:00:23Z

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2024-02-06T14:34:30Z

@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review.

cston · 2024-02-06T20:37:36Z

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

+                implicitReceiver,
+                out addMethods);
+
+            // This is what BindCollectionInitializerElementAddMethod is doing in terms of reporting diagnostics and detecting a failure


BindCollectionInitializerElementAddMethod

This seems like a lot of code that needs to stay in sync with BindCollectionInitializerElementAddMethod. Is the reason for duplicating the code to handle cycles and return the add methods, and is there an easy way to use the existing Binder methods? For instance, if the reason is to handle cycles, could we use throw an exception at the point HasParamsCollectionTypeInProgress(namedType) is determined and have the outer most caller that created the ParamsCollectionTypeInProgressBinder catch that case? #Resolved

There are several reasons. Main reasons are to reduce amount of binding and have a better control what is causing a failure. And cycles, of course. For example, in general we are not supposed to make semantic decisions based on BoundNode.HasErrors flag, but we did. We are trying to not set the flag too liberally, but that guideline is not always followed (especially in a very old code), because we are not supposed to use its value for semantic decisions. Also, I did find examples when some errors that are not supposed to fail conversion classification failed it, etc. I prefer to have a little code duplication, but have a better control over what we are doing. I do not expect the methods to change much in the future. In the process, in the new code I was able to drop a lot of original code that is not relevant to our scenarios and cause. Therefore, no need to think about "protecting" those code paths. The chosen approach gives me greater confidence that we are doing the right thing from behavior point of view.

cston · 2024-02-06T22:22:21Z

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

+                                if (!paramsType.IsSZArray())
+                                {
+                                    var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
+                                    emptyCollection ??= new BoundUnconvertedCollectionExpression(node, ImmutableArray<BoundNode>.CastUp(ImmutableArray<BoundExpression>.Empty)) { WasCompilerGenerated = true, IsParamsCollection = true };


ImmutableArray.CastUp(ImmutableArray.Empty)

ImmutableArray<BoundNode>.Empty? #Resolved

No, we need this array of BoundExpression underneath, so that we could do a cast down without allocation/copy, if needed. Perhaps for an empty array, there isn't much to allocate and copy, but I prefer to be consistent

cston · 2024-02-06T22:44:50Z

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

+                case CollectionExpressionTypeKind.ImplementsIEnumerable:
+                case CollectionExpressionTypeKind.CollectionBuilder:
+                    {
+                        binder.TryGetCollectionIterationType(CSharpSyntaxTree.Dummy.GetRoot(), type, out elementType);


CSharpSyntaxTree.Dummy.GetRoot()

Consider extracting a local. #Closed

Consider extracting a local.

Done

cston · 2024-02-06T23:03:28Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs

+                }
+            }
+
+            void reportInvalidParams(BindingDiagnosticBag diagnostics)


reportInvalidParams

Consider declaring this local function and the two above at the same scope. #Closed

Consider declaring this local function and the two above at the same scope.

Done

RikkiGibson · 2024-02-06T20:40:52Z

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

@@ -3707,30 +4937,12 @@ static void Main()

            // Inline collection expression results in an ambiguity.


Is this comment still accurate? I thought collection-expressions also made a change to make this conversion not applicable when the expected APIs are not present on the collection type. #Resolved

Is this comment still accurate?

Will remove. Thanks.

RikkiGibson · 2024-02-06T21:10:27Z

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

+        var x = (params int a) => a;
+        local (0);
+
+        int local(params int a) => a; 


It's possible I missed it, but I did not see a test case which demonstrates the lack of accessibility checks on the constructor and Add method when the params parameter is in a local function or lambda. Please add such a test if one does not already exist, for example:

using System.Collections; using System.Collections.Generic; public class MyCollection : IEnumerable<string> { internal MyCollection() { } internal void Add(string s) { } IEnumerator<string> IEnumerable<string>.GetEnumerator() => throw null!; IEnumerator IEnumerable.GetEnumerator() => throw null!; } public class Program { public void Test1() { local(); local("a"); // expect no diagnostic void local(params MyCollection collection) { } // expect no diagnostic var x = (params MyCollection collection) => { }; } // expect a declaration diagnostic for insufficient accessibility of MyCollection..ctor/Add() public void Test2(params MyCollection collection) { } } ``` #Resolved

It's possible I missed it, but I did not see a test case which demonstrates the lack of accessibility checks on the constructor and Add method when the params parameter is in a local function or lambda. Please add such a test if one does not already exist

Added ImplementsIEnumerableT_20_LessAccessibleConstructorAndAdd_NoError_In_LambdaOrLocalFunction

RikkiGibson · 2024-02-06T22:30:43Z

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

+                binder.ReportDiagnosticsIfObsolete(diagnostics, method, node, hasBaseReceiver: false);
+                // NOTE: Use-site diagnostics were reported during overload resolution.
+
+                CheckRequiredMembersInObjectInitializer(method, initializers: default, node, diagnostics);


Perhaps we should have a test for when the collection type has a required property. I would expect an error when using 'params' with such a type or when converting to the type from collection expression. But that the conversion would still exist. #Resolved

Perhaps we should have a test for when the collection type has a required property. I would expect an error when using 'params' with such a type or when converting to the type from collection expression. But that the conversion would still exist.

Added tests, they behave as you expect, but I am not sure whether I am happy about that fact.

RikkiGibson · 2024-02-06T23:03:29Z

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

-                    type: GetSpecialType(SpecialType.System_Void, diagnostics, elementInitializer),
-                    hasErrors: hasErrors);
+                var d = BindingDiagnosticBag.GetInstance();
+                bool toCheck = collectionInitializerAddMethodBinder.HasCollectionExpressionApplicableAddMethod(elementInitializer, implicitReceiver.Type, boundElementInitializerExpressions[0].Type, addMethods: out _, d);


Please add a comment to explain what this assert is checking in future PR #Resolved

Please add a comment to explain what this assert is checking in future PR

Added a comment.

AlekseyTs · 2024-02-07T02:33:07Z

@333fred, @dotnet/roslyn-compiler For the second review

AlekseyTs · 2024-02-07T16:25:27Z

@333fred, @dotnet/roslyn-compiler For the second review

cston · 2024-02-07T19:00:05Z

    public void CreateMethod_10_NotExtensionGetEnumerator()

Is this distinct from CreateMethod_04_NoElementType? #Resolved

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1019 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

cston · 2024-02-07T19:03:50Z

        comp.VerifyEmitDiagnostics();

Consider executing this code, and verifying which Add() is invoked. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1885 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

cston · 2024-02-07T19:03:57Z

        comp.VerifyEmitDiagnostics();

Consider executing this code, and verifying which Add() is invoked. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1925 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

cston · 2024-02-07T19:15:00Z

Consider including:

Test([]);
Test([4]);
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:10458 in 9562c7c. [](commit_id = 9562c7c9baa1bc082c7e969a8cfd8eb04d26d4ee, deletion_comment = False)

cston · 2024-02-07T19:17:54Z

Looks like these cases are covered in Cycle_04.

In reply to: 1932705423

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:10458 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

AlekseyTs · 2024-02-07T19:18:23Z

    public void CreateMethod_10_NotExtensionGetEnumerator()

Is this distinct from CreateMethod_04_NoElementType?

Looks like a dupe indeed.

In reply to: 1932682227

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1019 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

AlekseyTs · 2024-02-07T20:27:18Z

        comp.VerifyEmitDiagnostics();

Consider executing this code, and verifying which Add() is invoked.

The runtime behavior isn't something that I am interested in. We can "bind" creation and that is good enough for the purpose of the feature.

In reply to: 1932688118

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1885 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

AlekseyTs · 2024-02-07T20:29:14Z

        comp.VerifyEmitDiagnostics();

Consider executing this code, and verifying which Add() is invoked.

The runtime behavior isn't something that I am interested in. We can "bind" creation and that is good enough for the purpose of the feature.

In reply to: 1932688311

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1925 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

AlekseyTs · 2024-02-07T20:38:41Z

        comp.VerifyEmitDiagnostics();

To clarify, the process of creating the collection and adding elements to it is handled by Collection Expressions code

In reply to: 1932813913

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1925 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

AlekseyTs · 2024-02-07T20:45:56Z

    public void CreateMethod_10_NotExtensionGetEnumerator()

Is this distinct from CreateMethod_04_NoElementType?

Looks like a dupe indeed.

Since I am not planning to make other changes in this PR. I am going to merge as is and get rid of the dupe in the next PR

In reply to: 1932710683

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1019 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

RikkiGibson

Resolution of the PR comments looks good to me, thanks.

AlekseyTs · 2024-02-07T22:24:09Z

    public void CreateMethod_10_NotExtensionGetEnumerator()

Removed the dupe in #72000

In reply to: 1932839926

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs:1019 in 9562c7c. [](commit_id = 9562c7c, deletion_comment = False)

Align params type validation with the the spec

bb6aea4

https://github.com/dotnet/csharplang/blob/main/proposals/params-collections.md#method-parameters

AlekseyTs added Area-Compilers Feature - ParamsCollections labels Feb 1, 2024

AlekseyTs requested review from 333fred and RikkiGibson February 1, 2024 20:21

AlekseyTs requested a review from a team as a code owner February 1, 2024 20:21

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 1, 2024

RikkiGibson self-assigned this Feb 1, 2024

cston reviewed Feb 6, 2024

View reviewed changes

RikkiGibson approved these changes Feb 6, 2024

View reviewed changes

PR feedback

9562c7c

cston approved these changes Feb 7, 2024

View reviewed changes

AlekseyTs merged commit 6cd3f1e into dotnet:features/ParamsCollections Feb 7, 2024
25 checks passed

RikkiGibson reviewed Feb 7, 2024

View reviewed changes

AlekseyTs mentioned this pull request Feb 10, 2024

An unsafe/safe context has unexpected effect on conversion classification for collection expressions #71840

Open

dotnet-bot mentioned this pull request Mar 21, 2024

[Automated] PRs inserted in VS build main-34720.100 #72629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align `params` type validation with the the spec #71918

Align `params` type validation with the the spec #71918

AlekseyTs commented Feb 1, 2024

AlekseyTs commented Feb 1, 2024

AlekseyTs commented Feb 2, 2024

AlekseyTs commented Feb 5, 2024

AlekseyTs commented Feb 6, 2024

cston Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 6, 2024 •

edited

Loading

cston Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 6, 2024 •

edited

Loading

cston Feb 6, 2024 •

edited

Loading

AlekseyTs Feb 7, 2024

cston Feb 6, 2024 •

edited

Loading

AlekseyTs Feb 7, 2024

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 7, 2024

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 7, 2024

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 7, 2024 •

edited

Loading

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

cston commented Feb 7, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited

Loading

cston commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

RikkiGibson left a comment

AlekseyTs commented Feb 7, 2024

		@@ -3707,30 +4937,12 @@ static void Main()

		// Inline collection expression results in an ambiguity.

Align params type validation with the the spec #71918

Align params type validation with the the spec #71918

Conversation

AlekseyTs commented Feb 1, 2024

AlekseyTs commented Feb 1, 2024

AlekseyTs commented Feb 2, 2024

AlekseyTs commented Feb 5, 2024

AlekseyTs commented Feb 6, 2024

cston Feb 6, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

cston Feb 6, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

cston Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 7, 2024

Choose a reason for hiding this comment

cston Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 7, 2024

Choose a reason for hiding this comment

RikkiGibson Feb 6, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Feb 7, 2024

Choose a reason for hiding this comment

RikkiGibson Feb 6, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Feb 7, 2024

Choose a reason for hiding this comment

RikkiGibson Feb 6, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Feb 6, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Feb 7, 2024

Choose a reason for hiding this comment

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

cston commented Feb 7, 2024 • edited by AlekseyTs Loading

cston commented Feb 7, 2024 • edited by AlekseyTs Loading

cston commented Feb 7, 2024 • edited by AlekseyTs Loading

cston commented Feb 7, 2024 • edited Loading

cston commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

AlekseyTs commented Feb 7, 2024

RikkiGibson left a comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 7, 2024

Align `params` type validation with the the spec #71918

Align `params` type validation with the the spec #71918

cston Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 6, 2024 •

edited

Loading

cston Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 6, 2024 •

edited

Loading

cston Feb 6, 2024 •

edited

Loading

cston Feb 6, 2024 •

edited

Loading

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Feb 7, 2024 •

edited

Loading

RikkiGibson Feb 6, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited by AlekseyTs

Loading

cston commented Feb 7, 2024 •

edited

Loading