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.

Sign up for GitHub

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

Jump to bottom

Add WhenAll and WaitAll analyzer for single task argument #4841

Merged
merged 22 commits into from
Apr 22, 2021
Merged

Add WhenAll and WaitAll analyzer for single task argument #4841

merged 22 commits into from
Apr 22, 2021

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Feb 12, 2021 •
edited
Loading

Fixes dotnet/runtime#33806 and dotnet/runtime#33807

Definition of Done Checklist

  • Analyzer and Code Fix implemented for C# and VB
  • Run against dotnet/runtime
  • Run against dotnet/roslyn-analyzers
  • Run against dotnet/roslyn
  • Document for review: severity, default, categorization, numbering, titles, messages, and descriptions.

Youssef1313
Youssef1313 reviewed Feb 13, 2021
View reviewed changes
...alyzers/Core/Microsoft.NetCore.Analyzers/Tasks/DoNotUseWhenAllOrWaitAllWithSingleArgument.cs
Comment on lines +20 to +24
new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.DoNotUseWhenAllWithSingleTaskTitle), MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)),
new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.DoNotUseWhenAllWithSingleTaskTitle), MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)),
Copy link
Member

Choose a reason for hiding this comment

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

Was it intentional to pass the title as the message too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case they are the same, so I didn't add a separate message.

Youssef1313
Youssef1313 reviewed Feb 13, 2021
View reviewed changes
...rs/Core/Microsoft.NetCore.Analyzers/Tasks/DoNotUseWhenAllOrWaitAllWithSingleArgumentFixer.cs

return editor.GetChangedDocument();
},
equivalenceKey: title),
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, there was a comment from @sharwell somewhere that equivalence key shouldn't be localized and use nameof(Resources.FixTitle) instead. I don't know the reason though, but just pointing this out.

Copy link
Member

Choose a reason for hiding this comment

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

dotnet/roslyn#52077 (comment)

Youssef1313
Youssef1313 reviewed Feb 13, 2021
View reviewed changes
...itTests/Microsoft.NetCore.Analyzers/Tasks/DoNotUseWhenAllOrWaitAllWithSingleArgumentTests.cs Outdated
Comment on lines 203 to 215
var fixedSource = @"
using System.Threading.Tasks;

class C
{
async Task M()
{
var t1 = CreateTask();
var objectTask1 = CreateObjectTask();

await t1;
await CreateTask();
var t1WhenAll = {|CA2250:Task.WhenAll(t1)|};
Copy link
Member

Choose a reason for hiding this comment

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

the fixed code seems incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var returnedTask = Task.WhenAll(t1) was recommended to remain unfixed as designed in this comment dotnet/runtime#33806 (comment)

Base automatically changed from master to main March 5, 2021 02:20
@ryzngard ryzngard force-pushed the feature/task_when_wait_all_analyzer branch 2 times, most recently from ff872d0 to 7a88664 Compare April 8, 2021 19:36
@ryzngard ryzngard marked this pull request as ready for review April 14, 2021 21:23
@ryzngard ryzngard requested a review from a team as a code owner April 14, 2021 21:23
mavasani
mavasani reviewed Apr 15, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

New .NET6 analyzers need to target release/6.0.1xx-preview4 branch

mavasani
mavasani reviewed Apr 15, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated
<data name="DoNotUseWaitAllWithSingleTaskDescription" xml:space="preserve">
<value>Using 'WaitAll' with a single task should be avoided in favor of directly awaiting that task or returning it as is.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also include the why part in the description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it enough to say something like

"Using 'WaitAll' may result in performance loss, await or return the task instead" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds fine to me.

mavasani
mavasani reviewed Apr 15, 2021
View reviewed changes
...alyzers/Core/Microsoft.NetCore.Analyzers/Tasks/DoNotUseWhenAllOrWaitAllWithSingleArgument.cs
Comment on lines +81 to +90
return nameMatches &&
targetMethod.IsStatic &&
SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType, taskType) &&
parameters.Length == 1 &&
parameters[0].IsParams;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical, but I think a bit more performant implementation would be to compute and store the method symbols for Task.WhenAll and Task.WaitAll upfront in the compilation start action and then this method would just need to perform symbol equality checks.

mavasani
mavasani reviewed Apr 15, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Also please make sure to run msbuild /v:m /t:pack at the root of the repo for auto-updating the documentation files in the repo and push the changes to the PR.

mavasani
mavasani reviewed Apr 15, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall, this looks great to me. Please address the suggestions and this should be good to merge.

@ryzngard ryzngard force-pushed the feature/task_when_wait_all_analyzer branch from e2a9359 to 604a1f0 Compare April 16, 2021 22:58
@ryzngard ryzngard changed the base branch from main to release/6.0.1xx-preview4 April 16, 2021 23:00
@ryzngard ryzngard requested a review from mavasani April 21, 2021 20:13
@codecov
Copy link

codecov bot commented Apr 21, 2021 •
edited
Loading

Codecov Report

Merging #4841 (dd00427) into release/6.0.1xx-preview4 (138ab76) will increase coverage by 0.00%.
The diff coverage is 96.88%.

@@                    Coverage Diff                     @@
##           release/6.0.1xx-preview4    #4841    +/-   ##
==========================================================
  Coverage                     95.55%   95.55%            
==========================================================
  Files                          1200     1205     +5     
  Lines                        273798   274343   +545     
  Branches                      16611    16632    +21     
==========================================================
+ Hits                         261624   262146   +522     
- Misses                         9978     9989    +11     
- Partials                       2196     2208    +12

mavasani
mavasani approved these changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge when ready. Thanks!

ryzngard and others added 3 commits April 22, 2021 10:52
@ryzngard ryzngard merged commit 535cbe7 into dotnet:release/6.0.1xx-preview4 Apr 22, 2021
@ryzngard ryzngard deleted the feature/task_when_wait_all_analyzer branch April 22, 2021 21:31
This was referenced Apr 23, 2021
Merged
Closed
@jeffhandley jeffhandley mentioned this pull request Aug 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@mavasani mavasani mavasani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not call Task.WhenAll with a single argument
3 participants
@ryzngard @mavasani @Youssef1313