Apply CA2016: Forward CancellationToken to methods that can take it #37607
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carlossanlop
requested review from
AArnott,
jeffhandley,
stephentoub,
pgovind,
mavasani,
bartonjs,
jozkee,
buyaa-n and
a team
carlossanlop
changed the title
Merged
4 tasks
mavasani
reviewed
Jun 8, 2020
src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/HttpContentJsonExtensions.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jun 8, 2020
src/libraries/Common/src/System/Net/WebSockets/ManagedWebSocket.cs
Outdated
Show resolved
Hide resolved
...raries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs
Outdated
Show resolved
Hide resolved
...raries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliStream.Decompress.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/EmptyContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/EmptyContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.HttpListener/src/System/Net/Managed/HttpResponseStream.Managed.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.Channels/src/System/Threading/Channels/SingleConsumerUnboundedChannel.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.Channels/src/System/Threading/Channels/SingleConsumerUnboundedChannel.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jun 9, 2020
src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/HttpContentJsonExtensions.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jun 9, 2020
src/libraries/System.Net.Http/src/System/Net/Http/MultipartContent.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jun 9, 2020
...libraries/Microsoft.Extensions.Hosting.Abstractions/src/HostingAbstractionsHostExtensions.cs
Outdated
Show resolved
Hide resolved
Tratcher
reviewed
Jun 9, 2020
...libraries/Microsoft.Extensions.Hosting.Abstractions/src/HostingAbstractionsHostExtensions.cs
Outdated
Show resolved
Hide resolved
carlossanlop
commented
Jun 23, 2020
...libraries/Microsoft.Extensions.Hosting.Abstractions/src/HostingAbstractionsHostExtensions.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jun 25, 2020
There was a problem hiding this comment.
LGTM
Can you also change:
runtime/eng/CodeAnalysis.ruleset
Line 117 in f40c6b6
to be Warning instead of Info? That will help to ensure we haven't missed any and don't miss any more moving forward.
|
The latest bugs in this analyzer have been merged with this PR. I merged it yesterday. |
|
@carlossanlop I believe you can just bump up Line 9 in 872da58 |
carlossanlop
commented
Jul 7, 2020
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs
Show resolved
Hide resolved
carlossanlop
commented
Jul 7, 2020
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs
Show resolved
Hide resolved
stephentoub
reviewed
Jul 9, 2020
src/libraries/System.Net.HttpListener/src/System/Net/Managed/HttpResponseStream.Managed.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jul 9, 2020
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
Outdated
Show resolved
Hide resolved
…erify build and test CI results
…in its default overload
jeffhandley
approved these changes
Jul 15, 2020
...raries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliStream.Decompress.cs
Show resolved
Hide resolved
|
Thanks for the sign-off, @jeffhandley. Can I get one more @stephentoub @mavasani @AArnott ? |
mavasani
approved these changes
Jul 15, 2020
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR applies Roslyn analyzer CA2016: It identified methods that had a
CancellationTokenparameter, and then looked inside its method body for any method invocations that could accept a token as an argument, but weren't taking it yet.The Roslyn analyzer/fixer is up for review here: dotnet/roslyn-analyzers#3641
Original issue here: #33774
@mavasani there were a couple of cases were the rule behaved strange:
A) In
MockConnection.cs, theReadAsyncmethod is located in an extension class. The rule wasn't supposed to look for extension method overloads, but it did anyway, and offered a fix. Applying the fix causes the object to be substituted with the extension class, and the code now does not compile. Here's the unit test that verifies we don't offer a diagnostic when the overload is in an extension class, and it is passing. Do you know what's causing this unexpected diagnostic to be triggered?B) In
HttpContentJsonExtensions.cs, the rule triggered a diagnostic and offered a fix, but applying it would cause a build break because the method overload that takes a token is not available in the same .NET Standard version, so I decided to wrap those invocations with a pragma directive to prevent getting the rule triggered. If this is something I can detect on the analyzer side, let me know.