Params side of the design changes around Add methods by AlekseyTs · Pull Request #72771 · dotnet/roslyn

AlekseyTs · 2024-03-28T05:11:51Z

The main observable change - the original (unconverted) params arguments are used in the process of populating target collection.

Spec changes: dotnet/csharplang#8022

Fixes #72098

Spec changes: dotnet/csharplang#8022 Fixes dotnet#72098

AlekseyTs · 2024-03-28T20:57:10Z

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

            }

-            return parameters[paramNum].TypeWithAnnotations;
+            BoundExpression bindInterpolatedStringHandlerInMemberCall(


bindInterpolatedStringHandlerInMemberCall

This used to be BindInterpolatedStringHandlerInMemberCall at the end of the Binder_InterpolatedString.cs. No meaningful changes during the move.

AlekseyTs · 2024-03-28T20:58:08Z

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

            return (builderAppendCallsArray.ToImmutableAndFree(), builderPatternExpectsBool ?? false, positionInfoArray.ToImmutableAndFree(), baseStringLength, numFormatHoles);
        }
-
-        private BoundExpression BindInterpolatedStringHandlerInMemberCall(


BindInterpolatedStringHandlerInMemberCall

Moved as a local function bindInterpolatedStringHandlerInMemberCall to Binder_Expressions.cs

I would have a slight leaning towards keeping this where it is. I won't block on it, but I did put this here intentionally, rather than making it a local function in CoerceArguments, because I wanted to keep all the interpolation logic together.

I would have a slight leaning towards keeping this where it is. I won't block on it, but I did put this here intentionally, rather than making it a local function in CoerceArguments, because I wanted to keep all the interpolation logic together.

The primary reason for the move is to constrain access to MemberAnalysisResult,.ArgsToParamsOpt to as few helpers that anyone can call in the future as possible. I was able make other helper(s) local functions, I wouldn't be able otherwise. I find that more important than a desire to keep interpolation logic in a separate file.

AlekseyTs · 2024-03-28T20:59:08Z

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

+            BoundExpression bindInterpolatedStringHandlerInMemberCall(
+                BoundExpression unconvertedString,
+                TypeSymbol handlerType,
+                ArrayBuilder<BoundExpression>? arguments,


ArrayBuilder? arguments,

Made this parameter nullable.

AlekseyTs · 2024-03-28T20:59:27Z

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

+                        diagnostics);
+                }
+
+                Debug.Assert(arguments is not null);


Debug.Assert(arguments is not null);

Added this assert.

AlekseyTs · 2024-03-28T21:06:27Z

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

-                        Debug.Assert(paramsArgsBuilder.Count == 0);
-
-                        firstParamsArgument = i;
-                        paramsArgsBuilder.Add(argumentsBuilder[i]);


paramsArgsBuilder.Add(argumentsBuilder[i]);

Logic for this and the next statement (the for) was moved to local function CheckAndCoerceArguments.collectParamsArgs

AlekseyTs · 2024-03-28T21:07:25Z

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

+                return coercedArgument;
+            }
+
+            ArrayBuilder<BoundExpression> collectParamsArgs(


collectParamsArgs

This is essentially the logic that used to be in a loop at the beginning of BindDefaultArgumentsAndParamsCollection

AlekseyTs · 2024-03-28T21:28:54Z

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

-                ImmutableArray<BoundExpression> collectionArgs = paramsArgsBuilder.ToImmutableAndFree();
-                int collectionArgsLength = collectionArgs.Length;
-
-                TypeSymbol collectionType = parameters[paramsIndex].Type;


TypeSymbol collectionType = parameters[paramsIndex].Type;

Code starting from here through Debug.Assert(collection.IsParamsArrayOrCollection); is extracted to CreateParamsCollection helper. No chnages to the code other than making parameters[paramsIndex] a parameter named "paramsParameter".

AlekseyTs · 2024-03-28T21:30:21Z

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


        }

+        private BoundExpression CreateParamsCollection(SyntaxNode node, ParameterSymbol paramsParameter, ImmutableArray<BoundExpression> collectionArgs, BindingDiagnosticBag diagnostics)


CreateParamsCollection

This is a helper extracted from BindDefaultArgumentsAndParamsCollection. See a comment at the original location

AlekseyTs · 2024-03-28T21:33:30Z

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

-                    Debug.Assert(!haveDefaultArguments || collectionArgsLength == 1);
-                    Debug.Assert(collectionArgsLength == 1 || firstParamsArgument + collectionArgsLength == argumentsBuilder.Count);
-
-                    for (var i = firstParamsArgument + collectionArgsLength - 1; i != firstParamsArgument; i--)


for (var i = firstParamsArgument + collectionArgsLength - 1; i != firstParamsArgument; i--)

Logic in this loop is moved to a loop at the end of the CheckAndCoerceArguments.createParamsCollection local function

AlekseyTs · 2024-03-28T21:34:22Z

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

+                    argsToParamsBuilder.AddRange(argsToParamsOpt);
+                }
+
+                for (var i = firstParamsArgument + collectionArgs.Length - 1; i != firstParamsArgument; i--)


for (var i = firstParamsArgument + collectionArgs.Length - 1; i != firstParamsArgument; i--)

Logic in this loop used to be in BindDefaultArgumentsAndParamsCollection. See a comment at the original location.

AlekseyTs · 2024-03-28T22:04:16Z

@cston, @RikkiGibson, @333fred Please review

cston · 2024-03-29T00:31:13Z

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

+                return coercedArgument;
+            }
+
+            ArrayBuilder<BoundExpression> collectParamsArgs(


collectParamsArgs

Can this local function be static? #Resolved

Can this local function be static?

Yes, changing.

cston · 2024-03-29T00:38:44Z

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

+                    (BoundKind.OutVariablePendingInference or BoundKind.OutDeconstructVarPendingInference or BoundKind.DiscardExpression or BoundKind.ArgListOperator));
+
+                // Conversions to elements of collection are applied in the process of collection construction
+                paramsArgsBuilder.Add(arguments[arg]);


Consider extracting a local function for use here and inside the while loop. #Resolved

Consider extracting a local function for use here and inside the while loop.

Are you referring to paramsArgsBuilder.Add(arguments[arg]);? If so, I do not believe it is worth the trouble. There isn't enough "critical mass" in combined complexity and volume to gain much from sharing. A call would look as complicated as the current code, in my opinion.

After a closer look, I ended up refactoring the loop and avoiding the duplication.

cston · 2024-03-29T00:39:27Z

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

+            }
+
+            // Note, this function is going to free paramsArgsBuilder
+            void createParamsCollection(


void

static? #Resolved

coerceArgument cannot be static, this disallows createParamsCollection to be static.

333fred

Nothing blocking, so I'm signing off.

333fred · 2024-03-28T22:16:38Z

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

+        }
+
+        /// <summary>
+        /// A bit vector representing whose true bits indicate indices of bad arguments


Suggested change

/// A bit vector representing whose true bits indicate indices of bad arguments

/// A bit vector whose true bits indicate indices of bad arguments

``` #WontFix

The comment was copied from below. I have no intent fixing it in this PR. Feel free to fix both comments after the PR is merged.

333fred · 2024-03-29T01:14:13Z

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


                var isExpanded = resolutionResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm;
-                var argsToParams = resolutionResult.Result.ArgsToParamsOpt;
+                ImmutableArray<int> argsToParams;


Nit: consider moving this declaration closer to the usage, or inline. #Resolved

333fred · 2024-03-29T01:15:27Z

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

            return (builderAppendCallsArray.ToImmutableAndFree(), builderPatternExpectsBool ?? false, positionInfoArray.ToImmutableAndFree(), baseStringLength, numFormatHoles);
        }
-
-        private BoundExpression BindInterpolatedStringHandlerInMemberCall(


I would have a slight leaning towards keeping this where it is. I won't block on it, but I did put this here intentionally, rather than making it a local function in CoerceArguments, because I wanted to keep all the interpolation logic together.

cston · 2024-03-29T03:15:29Z

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests.cs

            Assert.Equal("AAttribute..ctor(params System.Int32[] args)", attributeData.AttributeConstructor.ToTestDisplayString());
            Assert.Equal(1, attributeData.AttributeConstructor.ParameterCount);
-            Assert.Equal(new object[] { 1, 2, 3 }, attributeData.ConstructorArguments.Select(arg => arg.Value));
+            Assert.Equal(TypedConstantKind.Array, attributeData.ConstructorArguments.Single().Kind);


attributeData.ConstructorArguments.Single()

Is this a meaningful change to the public API behavior for AttributeData.ConstructorArguments? #Resolved

Is this a meaningful change to the public API behavior for AttributeData.ConstructorArguments?

Yes. In some error conditions we are actually manage to create an array. The behavior is close to the success case now, see the other test I modified in this file

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 28, 2024

AlekseyTs added 2 commits March 28, 2024 06:56

Params side of the design chnages around Add methods

7a4fcb0

Spec changes: dotnet/csharplang#8022 Fixes dotnet#72098

Fixup

6b30a4f

AlekseyTs force-pushed the any-add-params branch from dc3e2b1 to 6b30a4f Compare March 28, 2024 13:56

runfoapp bot mentioned this pull request Mar 28, 2024

NuGet.Protocol.Core.Types.FatalProtocolException exception in helix work items #58197

Open

AlekseyTs added 2 commits March 28, 2024 13:48

Only empty params collections are treated as default values

f401b7b

Rename

44f091f

AlekseyTs commented Mar 28, 2024

View reviewed changes

Formatting and small refactoring.

caac59c

AlekseyTs changed the title ~~Params side of the design chnages around Add methods~~ Params side of the design changes around Add methods Mar 28, 2024

AlekseyTs requested review from 333fred, RikkiGibson and cston March 28, 2024 22:03

AlekseyTs marked this pull request as ready for review March 28, 2024 22:03

AlekseyTs requested a review from a team as a code owner March 28, 2024 22:03

RikkiGibson self-assigned this Mar 28, 2024

cston reviewed Mar 29, 2024

View reviewed changes

333fred approved these changes Mar 29, 2024

View reviewed changes

AlekseyTs added 2 commits March 28, 2024 19:32

PR feedback

882232b

Reduce duplication

fee587f

AlekseyTs requested a review from cston March 29, 2024 02:51

cston reviewed Mar 29, 2024

View reviewed changes

cston approved these changes Mar 29, 2024

View reviewed changes

AlekseyTs enabled auto-merge (squash) March 29, 2024 03:27

AlekseyTs merged commit eadc0cc into dotnet:features/CollectionAdd Mar 29, 2024

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


		}

		private BoundExpression CreateParamsCollection(SyntaxNode node, ParameterSymbol paramsParameter, ImmutableArray<BoundExpression> collectionArgs, BindingDiagnosticBag diagnostics)

	/// A bit vector representing whose true bits indicate indices of bad arguments
	/// A bit vector whose true bits indicate indices of bad arguments
	``` #WontFix

Conversation

AlekseyTs commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 28, 2024

Uh oh!

cston Mar 29, 2024 • edited by AlekseyTs 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!

cston Mar 29, 2024 • edited by AlekseyTs 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!

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

cston Mar 29, 2024 • edited by AlekseyTs 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!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Mar 28, 2024 • edited by AlekseyTs 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!

333fred Mar 29, 2024 • edited by AlekseyTs 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!

cston Mar 29, 2024 • edited by AlekseyTs 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!

Reviewers

Assignees

Labels

Projects

Milestone

AlekseyTs commented Mar 28, 2024 •

edited

Loading

AlekseyTs Mar 29, 2024 •

edited

Loading

cston Mar 29, 2024 •

edited by AlekseyTs

Loading

cston Mar 29, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Mar 29, 2024 •

edited

Loading

cston Mar 29, 2024 •

edited by AlekseyTs

Loading

333fred Mar 28, 2024 •

edited by AlekseyTs

Loading

333fred Mar 29, 2024 •

edited by AlekseyTs

Loading

cston Mar 29, 2024 •

edited by AlekseyTs

Loading