Detection of PLINQ nops analyzer #4126

Mrnikbobjeff · 2020-09-03T16:32:43Z

dnfadmin · 2020-09-03T16:33:00Z

All CLA requirements met.

mavasani

Marking as request changes due to few reasons:

Even though New rule: Detection of PLINQ nops #2812 was marked approved, we now have a process where you will need to file an issue on dotnet/runtime repo for analyzer suggestion. THe issue needs to be approved and also get an approval on the default RuleLevel. Note the bar for an analyzer to be a warning by default is very high, it should be only for correctness issues.
We are holding off on more .NET API analyzers for .NET5 - @jeffhandley to confirm. We will need a new branch for post .NET5 analyzers

Mrnikbobjeff · 2020-09-03T17:04:47Z

I see, does that include the analyzers already approved on the dotnet/runtime repo? As a learning exercise I whipped up analyzers for most issue there, if that applies to those analyzer suggestions as well I would stop working on integrating them into roslyn analyzers for a while until point number two is resolved :)

…jeff/roslyn-analyzers into feature/DetectPLINQNops

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

codecov · 2020-09-04T16:35:32Z

Codecov Report

Merging #4126 into master will increase coverage by 0.04%.
The diff coverage is 93.15%.

@@            Coverage Diff             @@
##           master    #4126      +/-   ##
==========================================
+ Coverage   95.78%   95.83%   +0.04%     
==========================================
  Files        1163     1173      +10     
  Lines      262725   266864    +4139     
  Branches    15829    16160     +331     
==========================================
+ Hits       251656   255738    +4082     
- Misses       9064     9077      +13     
- Partials     2005     2049      +44

…o replaced text based comparison to symbol based

…nto feature/DetectPLINQNops # Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNopsAnalyzer.cs

mavasani

Need to be written as an IOperation analyzer, and should not be a build warning by default. I haven't reviewed the code fix provider yet.

…n more in process

Mrnikbobjeff · 2020-10-13T08:18:05Z

@mavasani I migrated the analyzer to an IOperationAnalyzer which worked rather seemlessly. I will have to work on the fixer though, this is the first time coming into contact with the IOperation analyzer and fixer api

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNops.Fixer.cs

Evangelink · 2020-10-13T09:17:28Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNops.Fixer.cs

+            {
+                var newExpression = ((possibleInvocation as InvocationExpressionSyntax)!.Expression as MemberAccessExpressionSyntax)!.Expression;
+                possibleInvocation = newExpression;
+            } while (possibleInvocation is InvocationExpressionSyntax nestedInvocation && nestedInvocation.Expression is MemberAccessExpressionSyntax member && removableEnds.Contains(member.Name.Identifier.ValueText));


Do we want to have only a check on the method name as a string? It might be better to check for the method symbol.

This I was unclear about as it also came up in my other pr, do we have to recheck everything in the fixer that we already did in the analyzer? I.E. if we warn here we already checked the symbol in the actual analyzer

It is also bit unclear for me how much needs to be rechecked for the fixer.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNopsAnalyzer.cs

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNopsTest.cs

…jeff/roslyn-analyzers into feature/DetectPLINQNops # Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNopsAnalyzer.cs

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNopsTest.cs

Youssef1313

Nit:

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNops.Fixer.cs

mavasani · 2020-10-14T22:35:26Z

Closing and re-opening PR to retrigger tests, as we checked in new CI validation step for generated files.

Evangelink · 2020-10-15T07:28:34Z

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNops.Fixer.cs

+
+            context.RegisterCodeFix(
+                new AsParallelCodeAction(
+                    title: MicrosoftNetCoreAnalyzersResources.RemoveRedundantCall,


Not sure we do want to reuse this message or create a new specific one.

I have no idea, I'll adapt this to whatever is decided. It seemed to accurately reflect what the code fix does so I just chose that

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNops.Fixer.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/DetectPLINQNopsAnalyzer.cs

Youssef1313 · 2020-10-15T08:52:55Z

@Mrnikbobjeff FYI, you need to run msbuild RoslynAnalyzers.sln /t:pack and msbuild FxCopAnalyzers.sln /t:pack to regenerate auto-generated files. This will fix the CI failures.

Mrnikbobjeff · 2020-10-21T12:49:36Z

Some unanswered questions remain: Should we create a new Title for the code fix as we are currently reusing MicrosoftNetCoreAnalyzersResources.RemoveRedundantCall as title and equivalence key. Also, what should be rechecked in the codefix provider? This is relevant for both of my pull requests, as I do less computationally intensive operations by not rechecking much of what is checked in the analyzer

carlossanlop · 2020-10-27T20:25:14Z

@Mrnikbobjeff the analyzer has been approved today: dotnet/runtime#33769

Please take a look at the video when you can.

The analyzer has been approved for the Performance category, with Warning severity. The message/title/description can be decided/confirmed in the PR. Whether we want all cases to be covered by one ruleID or more than one, can also be decided here, depending on the complexity of each case to cover.

We should make sure to verify all the possible combinations:

The AsParallel invocation is the only invocation.
The AsParallel invocation is terminal, of a series of LINQ invocations like Select or Where.
The AsParallel invocation is non-terminal, and is preceded and followed by LINQ invocations like Select or Where.
The AsParallel invocation is non-terminal, and is followed by non-LINQ invocations like ToArray().
The AsParallel invocation is non-terminal, and is preceded by LINQ invocations, then followed by non-LINQ invocations.

Besides adding good unit tests, we must ensure to test actual usage in Visual Studio, to double check that the behavior makes sense, and we don't end up offering an empty or incorrect fix.

cc @mavasani @buyaa-n

Mrnikbobjeff · 2020-10-28T10:49:01Z

@Mrnikbobjeff FYI, you need to run msbuild RoslynAnalyzers.sln /t:pack and msbuild FxCopAnalyzers.sln /t:pack to regenerate auto-generated files. This will fix the CI failures.

Should building the FxCopAnalyzers solution work? the normal analyzer solution builds just fine, FxCopAnalyzers throw some errors though. I do not know whether the files were generated correctly

Youssef1313 · 2020-10-28T13:28:28Z

@Mrnikbobjeff It should build fine. Have you fixed the merge conflicts?

Youssef1313 · 2020-10-28T13:29:45Z

src/Microsoft.NetCore.Analyzers/Microsoft.NetCore.Analyzers.md

@@ -660,6 +660,18 @@ Calls to 'string.IndexOf' where the result is used to check for the presence/abs
 |CodeFix|True|
 ---

+## [CA2250](https://docs.microsoft.com/visualstudio/code-quality/ca2250): 'AsParallel' at the end of a LINQ query does not have an effect


After merging the current master into your branch, you will need to re-run msbuild pack.

The documentation links have moved from VS docs to .NET docs.
In current master you'll see these links are from .NET docs (while your branch is still on the old VS docs links).

mavasani

Please retarget this PR to https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx-preview1

mavasani · 2021-05-20T17:01:50Z

PR needs to target release\6.0.1xx branch.

Initial commit

2cec04e

Mrnikbobjeff requested a review from a team as a code owner September 3, 2020 16:32

mavasani suggested changes Sep 3, 2020

View reviewed changes

Schilli, Niklas added 4 commits September 4, 2020 13:47

Fixes for unit test and naming, adapted usage to other analyzers

14be941

Git mistake

2be0e72

Git mistake

e94d6e3

Merge branch 'feature/DetectPLINQNops' of https://github.com/Mrnikbob…

07cc042

…jeff/roslyn-analyzers into feature/DetectPLINQNops

Youssef1313 reviewed Sep 4, 2020

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Show resolved Hide resolved

Youssef1313 mentioned this pull request Sep 4, 2020

Update the code fix to respect markdown rules #4131

Closed

Schilli, Niklas added 4 commits September 8, 2020 19:55

Preemptively adapt formatting thanks to feedback in other branch. Als…

f903c05

…o replaced text based comparison to symbol based

More formatting and minor refactoring

54305c4

Merge branch 'master' of https://github.com/dotnet/roslyn-analyzers i…

38136bf

…nto feature/DetectPLINQNops # Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Renaming

578e280