C# 13: params modifier on collection types. #18329

michaelnebel · 2024-12-19T11:51:26Z

In this PR we make the relevant updates to the CodeQL library to support collection like types for params parameters. The extractor worked out of the box for extracting these types.
The feature is described here: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-13#params-collections

Copilot reviewed 13 out of 30 changed files in this pull request and generated 2 comments.

Files not reviewed (17)

csharp/ql/lib/semmle/code/csharp/Callable.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/Variable.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/commons/Collections.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/frameworks/System.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/frameworks/system/collections/Generic.qll: Language not supported
csharp/ql/test/library-tests/arguments/argumentByName.expected: Language not supported
csharp/ql/test/library-tests/arguments/argumentByParameter.expected: Language not supported
csharp/ql/test/library-tests/arguments/argumentByParameter.ql: Language not supported
csharp/ql/test/library-tests/arguments/argumentName.expected: Language not supported
csharp/ql/test/library-tests/arguments/argumentType.expected: Language not supported
csharp/ql/test/library-tests/arguments/parameterGetArguments.expected: Language not supported
csharp/ql/test/library-tests/arguments/parameterGetArguments.ql: Language not supported
csharp/ql/test/library-tests/dispatch/CallContext.expected: Language not supported
csharp/ql/test/library-tests/dispatch/GetADynamicTarget.expected: Language not supported

Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more

2024-12-20T13:27:24Z

csharp/ql/test/library-tests/arguments/arguments.cs

+    {
+        f10(0, 1, 2);
+        f10(0, [1, 2]);
+        f10(args: 1, o: 0);


The params modifier expects a List, but 1 is an int. It should be f10(args: new List { 1 }, o: 0);

Suggested change

f10(args: 1, o: 0);

f10(args: new List<int> { 1 }, o: 0);

2024-12-20T13:27:24Z

csharp/ql/test/library-tests/dataflow/collections/CollectionFlow.cs

+        SinkCollectionParams(new A()); // flow
+        SinkCollectionParams(null, new A()); // flow
+        SinkCollectionParams(null, new A(), null); // flow
+        SinkCollectionParams([new A()]); // flow


The use of square brackets '[]' for collection initialization is incorrect in C#. It should be replaced with curly braces '{}'.

Suggested change

SinkCollectionParams([new A()]); // flow

SinkCollectionParams(new A[] { new A() }); // flow

tamasvajk

LGTM, added two minor comments.

tamasvajk · 2024-12-30T13:44:15Z

csharp/ql/lib/semmle/code/csharp/commons/Collections.qll

+  abstract Type getElementType();
+}
+
+private class AddArrayType extends ParamsCollectionTypeImpl instanceof ArrayType {


Why is this class named AddArrayType? An alternative would be ParamsArrayType.

I have used the prefix Add in other places for private classes, which only purpose is to populate (or add) elements to an abstract class. In any case, I don't have any strong preference :-)

tamasvajk · 2024-12-30T13:45:34Z

csharp/ql/lib/semmle/code/csharp/commons/Collections.qll

+  override Type getElementType() { result = ArrayType.super.getElementType() }
+}
+
+private class AddCollectionTypes extends ParamsCollectionTypeImpl {


An alternative name could be ParamsConstructedCollectionTypes?

…e the test reports indexer access and calls to Add for list and collection creations).

…tion types.

…s to allow ParamsCollectionType.

michaelnebel · 2025-01-02T10:13:37Z

DCA looks good.

github-actions bot added the C# label Dec 19, 2024

michaelnebel force-pushed the csharp/params branch 2 times, most recently from ecced67 to f4539f7 Compare December 20, 2024 10:29

github-actions bot added the documentation label Dec 20, 2024

michaelnebel marked this pull request as ready for review December 20, 2024 13:26

Copilot bot review requested due to automatic review settings December 20, 2024 13:26

michaelnebel requested a review from a team as a code owner December 20, 2024 13:26

Copilot AI reviewed Dec 20, 2024

View reviewed changes

tamasvajk previously approved these changes Dec 30, 2024

View reviewed changes

michaelnebel added 6 commits January 2, 2025 11:13

C#: Exclude non source parameters from arguments test cases (otherwis…

8e9551d

…e the test reports indexer access and calls to Add for list and collection creations).

C#: Add more tests for params and update expected output.

89c16ab

C#: Update dispatch logic and other libraries to handle params collec…

8f5b8f4

…tion types.

C#: Update tests expected output.

c8e72c0

C#: Let the params test look in reference assemblies and update param…

4b7a6c5

…s to allow ParamsCollectionType.

C#: Add change-note.

596fdf0

C#: Address review comments.

fe4ec59

michaelnebel dismissed tamasvajk’s stale review via fe4ec59 January 2, 2025 10:21

michaelnebel force-pushed the csharp/params branch from fc22006 to fe4ec59 Compare January 2, 2025 10:21

michaelnebel requested a review from tamasvajk January 2, 2025 10:26

michaelnebel changed the title ~~C#: params modifier on collection types.~~ C# 13: params modifier on collection types. Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C# 13: params modifier on collection types. #18329

C# 13: params modifier on collection types. #18329

michaelnebel commented Dec 19, 2024 •

edited

Loading

Copilot AI left a comment

Copilot AI Dec 20, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Dec 20, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

tamasvajk left a comment

tamasvajk Dec 30, 2024

michaelnebel Jan 2, 2025

tamasvajk Dec 30, 2024

michaelnebel commented Jan 2, 2025

	SinkCollectionParams([new A()]); // flow
	SinkCollectionParams(new A[] { new A() }); // flow

C# 13: params modifier on collection types. #18329

Are you sure you want to change the base?

C# 13: params modifier on collection types. #18329

Conversation

michaelnebel commented Dec 19, 2024 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

tamasvajk left a comment

Choose a reason for hiding this comment

tamasvajk Dec 30, 2024

Choose a reason for hiding this comment

michaelnebel Jan 2, 2025

Choose a reason for hiding this comment

tamasvajk Dec 30, 2024

Choose a reason for hiding this comment

michaelnebel commented Jan 2, 2025

michaelnebel commented Dec 19, 2024 •

edited

Loading