Skip to content
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

C#: LINQ recommendation queries. #13885

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Aug 4, 2023

In this PR we fix an issue with the LINQ recommendation queries.
The LINQ related queries provide suggestions for re-writes of foreach statements

foreach (type x in exp) {
...
}

where the exp is considered enumerable.

The expression exp is considered enumerable if either

  • type implements the IEnumerable or IEnumerable<T> interface.
  • type implements a public instance method GetEnumerator().

However, the LINQ extension methods are only defined on types implementing either the IEnumerable or IEnumerable<T> interface.
More specifically,

  • Select, All and Where are extension methods on IEnumerable<T>
  • Cast and OfType are extension methods in IEnumerable.

Furthermore, single dimensional arrays (arrays of rank 1) also implement the IEnumerable<T> interface: https://learn.microsoft.com/en-gb/dotnet/api/system.array?view=netframework-4.7#remarks

That is, we need to narrow the LINQ recommendation to only those cases.

@github-actions github-actions bot added the C# label Aug 4, 2023
csharp/ql/lib/Linq/Helpers.qll Fixed Show fixed Hide fixed
csharp/ql/lib/Linq/Helpers.qll Fixed Show fixed Hide fixed
csharp/ql/lib/Linq/Helpers.qll Fixed Show fixed Hide fixed
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Aug 7, 2023
@michaelnebel michaelnebel marked this pull request as ready for review August 7, 2023 06:59
@michaelnebel michaelnebel requested a review from a team as a code owner August 7, 2023 06:59
hvitved
hvitved previously approved these changes Aug 7, 2023
hvitved
hvitved previously approved these changes Aug 7, 2023
@michaelnebel michaelnebel marked this pull request as draft August 7, 2023 13:55
@michaelnebel
Copy link
Contributor Author

After inspecting the first DCA run it became evident that the initial version of the narrowing of the LINQ recommendations was to conservative. I have changed the implementation and re-based the PR. Let's see what DCA says before spending more time on review'ing.

@michaelnebel
Copy link
Contributor Author

michaelnebel commented Aug 9, 2023

This time the reduction in results look more accurate.
I checked all the differences for the Powershell project and the removed results where indeed false positives. Furthermore, I checked the single removed result for the MissedAllOpportunity for the dotnet runtime and this was indeed also a false positive.

@michaelnebel
Copy link
Contributor Author

DCA reports a slowdown for the Jetbrains re-sharper plugin, but I can't reproduce it locally. The logs also look fine. Overall I think the change is good.

@michaelnebel michaelnebel marked this pull request as ready for review August 9, 2023 11:04
@michaelnebel michaelnebel merged commit f6aca58 into github:main Aug 10, 2023
15 checks passed
@michaelnebel michaelnebel deleted the csharp/linqforeach branch August 10, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants