Use AsParallel() correctly

[terrajobst]

Using .AsParallel() at the end of a LINQ query, e.g. foreach (var item in src.Select(...).Where(...).AsParallel(...)), is a nop and should either be removed or the AsParallel() moved earlier in the query. I've even seen developers write foreach (var item in src.AsParallel()) thinking it parallelizes the foreach loop, which it doesn't... it'd be good to warn about such misuse.

Category: Performance

[stephentoub]

Here's an example of the kinds of things we'd like to catch:
dotnet/sdk#10937

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Medium

[Mrnikbobjeff]

Would the fixer simply remove the call? I do not think it can add it earlier in the chain as it may cause race conditions depending on the implementation of the earlier linq chain, e.g. list.Select(x => NonThreadSafeMethod(x)).AsParallel() can only be resolved by removing the code. I guess that is the intended fix? Also, I guess the analyzer only triggers when the AsParallel is directly inside a foreach? Otherwise i may add other methods to the linq call chain after assigning the AsParallel() result at the end of the chain.

[stephentoub]

guess the analyzer only triggers when the AsParallel is directly inside a foreach

Yes

can only be resolved by removing the code. I guess that is the intended fix

That's the safe fix, in that it doesn't change the semantics. It'd be fine to offer multiple fixes, though, with a second option being to move the call to the beginning of the chain, and if the developer chooses that, it's their choice.

[Mrnikbobjeff]

I have a working analyzer for the described scenario, as well as the safe fix as you described it. I would still have to integrate it with the analyzer structure in this project but otherwise I would love to work on this if this is available.

[jeffhandley]

@carlossanlop @buyaa-n Can you follow up with @Mrnikbobjeff on this? We would need to bring this in for API Review if we want to move it forward.

[carlossanlop]

This comment in dotnet/roslyn-analyzers#2812 gives a clear example of what to detect in a foreach and what should be suggested as a fix.
This issue gives an example of an enumeration that saves the value into a variable.

• If an AsParallel invocation is found, check if it was added after enumerating invocations like Select or Where.
  • The first case to address should be to detect AsParallel calls inside a foreach.
  • The second case to address should be to detect AsParallel calls anywhere else, like when an enumeration result is saved into a variable.
• If any of those two cases were found, then the fixer should suggest moving the AsParallel to the first position.

As long as we keep those rules in mind, we think this is ready for review. After it gets approved, we will let @Mrnikbobjeff know so he can start working on it.

Category: Reliability Performance
Analyzer size: Medium
Fixer size: Medium
Suggested severity: Warning
Suggested messages:
- Title: Use 'AsParallel()' correctly
- Message: The 'AsParallel()' method should be called before enumerating.
- Description: The 'AsParallel()' method result is only effective when its returned value is consumed by enumerating methods.

cc @buyaa-n

[buyaa-n]

Thank you for your interest in contributing to this analzyer @Mrnikbobjeff, until the proposal get approved please take a look at our definition of done list - the list of tasks/actions need to be completed/taken before, during, and after the analyzer implementation. You do not have to complete all tasks, but your PR needs to cover all requirements related to the analyzer and fixer implementations (first 2 bullets) at least.

[terrajobst]

Video

This makes sense. The only thing we have to check with Manish is whether they analyzer should report separate IDs due to how fixers are advertised in the IDE.

[Mrnikbobjeff]

• If any of those two cases were found, then the fixer should suggest moving the AsParallel to the first position.

This is surely something which could be done, but I do not think that would be advisable. Suppose I have a Select method which is not threadsafe, then we would introduce bugs with the fixer. This was not mentioned in the Video. I only have the remove fixer implemented as this is always valid.
@carlossanlop

[stephentoub]

I do not think that would be advisable

If someone added the AsParallel(), they intended for something to be run in parallel. And while we can't know for sure how much of the query that comes before it was intended, as an option I think it's reasonable to offer to fix it with our best guess for where it would go, which is at the beginning. The only 100% safe fix is to remove it, but both can be offered, and if we have any control which is the "default" / first option, it should be the removal.

[Mrnikbobjeff]

I do not think that would be advisable

If someone added the AsParallel(), they intended for something to be run in parallel. And while we can't know for sure how much of the query that comes before it was intended, as an option I think it's reasonable to offer to fix it with our best guess for where it would go, which is at the beginning. The only 100% safe fix is to remove it, but both can be offered, and if we have any control which is the "default" / first option, it should be the removal.

I see the reasoning behing not completely removing the AsParallel call, I just was curious as I can not recall a codefix provider which might introduce bugs for fixing compiler warnings. As fas as I know this would be a first for codefix providers. I will still work on adding the second codefix provider

[eiriktsarpalis]

Hey @Mrnikbobjeff, would you still be interested in picking this up?

[ghost]

Tagging subscribers to this area: @tarekgh, @dotnet/area-system-linq-parallel
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Using .AsParallel() at the end of a LINQ query, e.g. foreach (var item in src.Select(...).Where(...).AsParallel(...)), is a nop and should either be removed or the AsParallel() moved earlier in the query. I've even seen developers write foreach (var item in src.AsParallel()) thinking it parallelizes the foreach loop, which it doesn't... it'd be good to warn about such misuse.

Category: Performance

Author:	terrajobst
Assignees:	Mrnikbobjeff
Labels:	api-approved, area-System.Linq, area-System.Linq.Parallel, code-analyzer, code-fixer, in pr
Milestone:	7.0.0