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

Do not call Task.WhenAll with a single argument #33806

Closed
terrajobst opened this issue Mar 19, 2020 · 8 comments · Fixed by dotnet/roslyn-analyzers#4841
Closed

Do not call Task.WhenAll with a single argument #33806

terrajobst opened this issue Mar 19, 2020 · 8 comments · Fixed by dotnet/roslyn-analyzers#4841
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@terrajobst
Copy link
Member

There's no reason to call WhenAll with a single Task; just use the Task.

Category: Performance

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@dotnet dotnet deleted a comment from Dotnet-GitSync-Bot Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Small
  • Fixer: Medium

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@carlossanlop
Copy link
Member

carlossanlop commented Oct 30, 2020

If the API review folks agree, we can group this proposal with #33807.

If they prefer a separate rule ID, then please approve separately.

Here is a usage example:

using System.Threading.Tasks;

namespace MyNamespace
{
    class Sample
    {
        public static void Main()
        {
            Task task = ...;

            // Instead of this
            await Task.WhenAll(task);
            // Suggest this
            await task;

            // Instead of this
            await Task.WhenAll(GetTask());
            // Suggest this
            await GetTask();

            // Analyzer should trigger, but should not offer a fix because whenResult is used somewhere else
            Task whenResult = Task.WhenAll(task);
            DoSomethingWithTask(whenResult);
        }

        static Task GetTask()
        {
            return ...;
        }

        static void DoSomethingWithTask(Task task)
        {
            ...;
        }
    }
}
  • This analyzer should not affect any of the other WhenAll overloads, because they take a Task[]. Only this overload, which takes a params Task[] task would be affected.

  • This analyzer should also have a fixer, except for the 3rd case indicated in the usage example.

  • Suggested severity: Info.

  • Suggested milestone if approved: 6.0 (seems simple to address).

  • Edit: Removed the incorrect Wait() usages and added await as described in the video.

@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 30, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 4, 2021
@terrajobst
Copy link
Member Author

terrajobst commented Feb 4, 2021

Video

  • Makes sense, but we shouldn't use Wait():
async Task M(Task task)
{
    // Before
    await Task.WhenAll(task);

    // After
    await task;
}

@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Feb 4, 2021
@carlossanlop
Copy link
Member

Thanks for the correction, and thanks for approving.

@carlossanlop
Copy link
Member

This is also yours, @ryzngard, per your request in #33807. Thanks for your help! Let us know if you have any questions.

@jeffhandley
Copy link
Member

@ryzngard Here's a getting started guide as well. Included in there is our typical "Definition of Done" for creating .NET Libraries analyzers.

@ryzngard
Copy link

@jeffhandley @terrajobst checked into https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx-preview4

@stephentoub
Copy link
Member

This was implemented as CA1842/1843. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants