Use ParamCollectionAttribute to mark params collections in metadata #71747

AlekseyTs · 2024-01-22T05:36:12Z

Corresponding speclet update is dotnet/csharplang#7850

AlekseyTs · 2024-01-22T16:41:57Z

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

AlekseyTs · 2024-01-23T18:52:54Z

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

RikkiGibson · 2024-01-22T23:55:44Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/SynthesizedClosureMethod.cs

@@ -124,6 +124,7 @@ private void EnsureAttributesExist(TypeCompilationState compilationState)
            }

            ParameterHelpers.EnsureRefKindAttributesExist(moduleBuilder, Parameters);
+            // Not emitting ParamCollectionAttribute/ParamArrayAttribute for these methods it is not a SynthesizedDelegateInvokeMethod


Suggested change

// Not emitting ParamCollectionAttribute/ParamArrayAttribute for these methods it is not a SynthesizedDelegateInvokeMethod

// Not emitting ParamCollectionAttribute/ParamArrayAttribute for these methods because it is not a SynthesizedDelegateInvokeMethod

``` #Closed

AlekseyTs · 2024-01-24T23:59:35Z

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

AlekseyTs · 2024-01-25T13:49:47Z

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

333fred

Done review pass

333fred · 2024-01-25T19:48:04Z

...Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DelegateCreationExpression.cs

@@ -13,6 +13,8 @@ internal partial class LocalRewriter
    {
        public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationExpression node)
        {
+            Debug.Assert(!node.Type.IsAnonymousType); // Missing EnsureParamCollectionAttributeExists call?


Is this supposed to be a prototype comment? #Resolved

Is this supposed to be a prototype comment?
No

333fred · 2024-01-25T19:52:32Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+                    result |= IsParamsValues.Collection;
+                }
+
+                _lazyIsParams = result;


The normal pattern we use for these is using ThreadSafeFlagsOperation.Set. I would prefer if we did that instead. #Resolved

The normal pattern we use for these is using ThreadSafeFlagsOperation.Set. I would prefer if we did that instead.

I do not think it is warranted in this case, we are not changing different bits concurrently. The value is initialized once. It used to be ThreeState, now it is four state. Once initialized, no bits are changing.

333fred · 2024-01-25T19:54:57Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+            Collection = 4,
+        }
+
+        private IsParamsValues _lazyIsParams;


If we're going to add a new flags (it looks like PackedFlags is at its size limit), should we consider just adding a second PackedFlags int now, and putting these bits in there? It should have the same effect on type size, and is more consistent with the standard way we do this here. #Resolved

I do not intend to merge this value into PackedFlags and the change in the field's type doesn't increase memory used. It used to be byte backed enum and it remains byte backed enum, only the set of possible values changed. It used to be a "monolithic" value and it remains a "monolithic" value. The bit nature of the enum is simply to make it easier to work with the value, it is not like parts of the value are changing concurrently with other parts of the value. I could easily enumerate all possible combinations , encode them as regular values and work with them as such. However the code would be more complicated then.

333fred · 2024-01-25T19:58:11Z

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

-        public sealed override bool IsParams => (_parameterSyntaxKind & ParameterSyntaxKind.ParamsParameter) != 0;
+        public sealed override bool IsParamArray => (_parameterSyntaxKind & ParameterSyntaxKind.ParamsParameter) != 0 && this.parameterType.IsSZArray();
+
+        public sealed override bool IsParamCollection => (_parameterSyntaxKind & ParameterSyntaxKind.ParamsParameter) != 0 && !this.parameterType.IsSZArray();


Do we need to be more precise for this and actually check that the parameter type is a valid params collection type? We validate that for IsParamArray, and I'm a bit worried that the symmetry with naming here implies that if IsParamsCollection is true, the parameter is a valid params collection. #Resolved

Do we need to be more precise for this and actually check that the parameter type is a valid params collection type?

No we do not need that, and that would actually be incorrect. If there is a params modifier in code, one of the properties should return true. The property is not meant to indicate validity of the modifier, just its presence. Therefore, what is not a valid params array is considered a params collection. It used to be that original IsParams property was true even for non-array types.

333fred · 2024-01-25T22:05:30Z

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

@@ -263,6 +264,14 @@ private static void EnsureRefKindAttributesExist(CSharpCompilation compilation,
            }
        }

+        internal static void EnsureParamCollectionAttributeExistsAndModifyCompilation(CSharpCompilation compilation, ImmutableArray<ParameterSymbol> parameters, BindingDiagnosticBag diagnostics)
+        {
+            if (parameters.FirstOrDefault(static (p) => p.IsParamCollection) is { } parameter)


Minor nit: it would probably be more efficient to use LastOrDefault so we start the search from the last parameter, rather than the first, as that will be the right parameter in 99% of cases, and I don't think it matters if we report errors on the second params parameter instead of the first if a user specified multiple in error. #Resolved

333fred · 2024-01-25T22:15:07Z

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

+}
+";
+
+        private static void VerifyParamAttribute(ParameterSymbol parameter, bool isParamArray = false, bool isParamCollection = false)


Small nit: would be slightly clearer for tests if this was named VerifyParamsAndAttribute, so that when I reading the validators I understand that this verifies both the standard params properties and attribute presence. #Resolved

333fred · 2024-01-25T22:19:03Z

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

+    }
+}
+""";
+            CreateCompilationWithIL(src, il).VerifyDiagnostics();


Nit: VerifyEmitDiagnostics #Resolved

333fred · 2024-01-25T22:19:08Z

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

+    }
+}
+""";
+            CreateCompilation(src).VerifyDiagnostics();


Nit: VerifyEmitDiagnostics #Resolved

333fred · 2024-01-25T22:29:39Z

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

+            comp = CreateCompilationWithIL("", il);
+
+            test1 = comp.GetMember<MethodSymbol>("Params.Test1").Parameters.Last();
+            test2 = comp.GetMember<MethodSymbol>("Params.Test2").Parameters.Last();
+
+            Assert.Empty(test1.GetAttributes());
+            Assert.Empty(test2.GetAttributes());
+
+            VerifyParamAttribute(test1, isParamCollection: true);
+            VerifyParamAttribute(test2, isParamArray: true);
+
+            AssertEx.Equal("System.Runtime.CompilerServices.ParamCollectionAttribute", test1.GetCustomAttributesToEmit(null).Single().ToString());
+            AssertEx.Equal("System.ParamArrayAttribute", test2.GetCustomAttributesToEmit(null).Single().ToString());
+


I'm not sure if I'm missing something, but this seems to be exactly the same as the first part of the test, minus 2 calls to VerifyParamAttribute on test1 and test2? #Resolved

I'm not sure if I'm missing something, but this seems to be exactly the same as the first part of the test, minus 2 calls to VerifyParamAttribute on test1 and test2?

Are you referring to the to different sets of asserts performed on "equal" compilations above?

Since PEParameterSymbol._lazyIsParams can be initialized by two different APIs, the IsParamArray/IsParamCollection properties and GetAttributes, I am asserting that result doesn't depend on the order in which the APIs are used first.

Use ParamCollectionAttribute to mark params collections in metadata

0700420

Corresponding speclet update is dotnet/csharplang#7850

AlekseyTs added Area-Compilers Feature - ParamsCollections labels Jan 22, 2024

AlekseyTs requested review from 333fred and RikkiGibson January 22, 2024 05:36

AlekseyTs requested review from a team as code owners January 22, 2024 05:36

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

AlekseyTs marked this pull request as draft January 22, 2024 15:51

AlekseyTs added 2 commits January 22, 2024 08:16

Fixup

cf9d5b4

Strengthen tests

ec3d6b1

AlekseyTs marked this pull request as ready for review January 22, 2024 16:41

RikkiGibson self-assigned this Jan 22, 2024

RikkiGibson approved these changes Jan 24, 2024

View reviewed changes

333fred reviewed Jan 25, 2024

View reviewed changes

333fred approved these changes Jan 25, 2024

View reviewed changes

PR feedback

95004df

AlekseyTs enabled auto-merge (squash) January 26, 2024 00:31

AlekseyTs merged commit ba5ee92 into dotnet:features/ParamsCollections Jan 26, 2024
26 of 31 checks passed

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

Use ParamCollectionAttribute to mark params collections in metadata #71747

Use ParamCollectionAttribute to mark params collections in metadata #71747

AlekseyTs commented Jan 22, 2024

AlekseyTs commented Jan 22, 2024

AlekseyTs commented Jan 23, 2024

RikkiGibson Jan 22, 2024 •

edited by AlekseyTs

Loading

AlekseyTs commented Jan 24, 2024

AlekseyTs commented Jan 25, 2024

333fred left a comment

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Jan 25, 2024

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Jan 25, 2024

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Jan 25, 2024

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Jan 25, 2024

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Jan 25, 2024

	// Not emitting ParamCollectionAttribute/ParamArrayAttribute for these methods it is not a SynthesizedDelegateInvokeMethod
	// Not emitting ParamCollectionAttribute/ParamArrayAttribute for these methods because it is not a SynthesizedDelegateInvokeMethod
	``` #Closed

Use ParamCollectionAttribute to mark params collections in metadata #71747

Use ParamCollectionAttribute to mark params collections in metadata #71747

Conversation

AlekseyTs commented Jan 22, 2024

AlekseyTs commented Jan 22, 2024

AlekseyTs commented Jan 23, 2024

RikkiGibson Jan 22, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 24, 2024

AlekseyTs commented Jan 25, 2024

333fred left a comment

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 25, 2024

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 25, 2024

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 25, 2024

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 25, 2024

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Jan 25, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Jan 25, 2024

Choose a reason for hiding this comment

RikkiGibson Jan 22, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading

333fred Jan 25, 2024 •

edited by AlekseyTs

Loading