Skip to content

C# 12: Support for collection expressions. #15426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jan 30, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Jan 24, 2024

In this PR we introduce extractor, QL library and dataflow support for collection expressions.
Example of collection expressions are [1, x, 3] and [2, .. y] in

var x = 6;
int[] y = [1, x, 3];
int[] z = [2, .. y];

Microsoft has documented collection expressions here. However, it is worth noting that even though the documentation states that collection expressions are also fully supported for inline arrays - but this appears not to be the case - which is indicated here.

@github-actions github-actions bot added the C# label Jan 24, 2024
@michaelnebel michaelnebel force-pushed the csharp/collectionexpressions branch 2 times, most recently from 61e329f to 724e688 Compare January 25, 2024 18:36
@michaelnebel michaelnebel force-pushed the csharp/collectionexpressions branch 2 times, most recently from e0b4a51 to 1bbe3cd Compare January 26, 2024 09:51
@michaelnebel michaelnebel force-pushed the csharp/collectionexpressions branch from 1bbe3cd to d34610f Compare January 26, 2024 10:50
@michaelnebel michaelnebel force-pushed the csharp/collectionexpressions branch from d34610f to 13b8d57 Compare January 26, 2024 13:18
@michaelnebel
Copy link
Contributor Author

DCA
(1) Performance looks good on average.
(2) A lot of extraction errors have been removed from the MS repositories (it looks like the collection expression feature has been used quite a lot already). There is some discrepancy in the alerts - especially in the repositories that uses the collection expression features. I still need to look into some of these.

@michaelnebel michaelnebel marked this pull request as ready for review January 26, 2024 15:03
@michaelnebel michaelnebel requested a review from a team as a code owner January 26, 2024 15:03
tamasvajk
tamasvajk previously approved these changes Jan 29, 2024
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The DB stats seem to be manually added. Should the stats be generated to have real numbers?

@michaelnebel
Copy link
Contributor Author

LGTM. The DB stats seem to be manually added. Should the stats be generated to have real numbers?

Yes, they are manually added.
@hvitved is in the process of figuring out whether we need the DB stats at all. I will postpone updating stats unless we run into performance issues, or if it turns out that they can't be scrapped all together or if I manage to finish all the C# 12 updates before we identify, whether stats can be scrapped :-D

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One suggestion.

@@ -250,6 +250,10 @@ module LocalFlow {
scope = e2 and
isSuccessor = true
or
e1 = e2.(CollectionExpression).getAnElement().(SpreadElementExpr).getExpr() and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to break this up into two steps:

For [..x], add a local flow step from x to ..x, and another local flow step from ..x to [..x]. We can then implement expectsContent on the ..x node, so that flow will only be allowed when we are in fact tracking flow inside a collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that is a really nice catch @hvitved !!!

@michaelnebel
Copy link
Contributor Author

DCA (1) Performance looks good on average. (2) A lot of extraction errors have been removed from the MS repositories (it looks like the collection expression feature has been used quite a lot already). There is some discrepancy in the alerts - especially in the repositories that uses the collection expression features. I still need to look into some of these.

I did some spot checking on the alerts changes on dotnet/roslyn.

  • All removed cs/unused-collection were false positives.
  • Checked several cs/useless-assignment-of-local and these were false positives as well as the query couldn’t detect uses of variables in collection expressions.
  • The added alerts to cs/call-to-object-tostring are true positives.

@michaelnebel
Copy link
Contributor Author

DCA looks good.

@michaelnebel michaelnebel merged commit 41cca47 into github:main Jan 30, 2024
@michaelnebel michaelnebel deleted the csharp/collectionexpressions branch January 30, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants