Fix VSTHRD003 to allow for awaiting more Task properties - #1252
#1252Fix VSTHRD003 to allow for awaiting more Task properties#1252
Conversation
When integrating into the VS repo, recent enhancements to VSTHRD003 made insertion of the new analyzers problematic. In particular, two patterns were quite common and arguably should be allowed: - Awaiting on the `JoinableTask.Task` property on the result of a `JoinableTaskFactory.RunAsync` method call. In general, if a method returns an object, awaiting one of that object's properties *might not* be fair game, but it seems more likely than not. So in this change, that is now allowed. - Awaiting a `TaskCompletionSource<T>.Task` property on a TCS that was created in that same method. Test methods often do this. Awaiting a TCS.Task property is never guaranteed to be safe as far as analyzers can tell (though the 3rd threading rule can make it safe), and when common patterns appear that are _typically_ safe, we should allow it.
02b81d1 to
b561132
Compare
@AArnott I still see this scenario triggering VSTHRD003 with 17.9.27, even with code as simple as |
|
@sandyarmstrong If you can provide a specific repro that we can add as a test case, I'll be happy to take a look at it. The specific example you just gave would not make a compelling test case because it would actually be a bug, since the returned Task would be uncompletable. I'd like a test case that demonstrates correct code that is being mis-flagged. |
When integrating into the VS repo, recent enhancements to VSTHRD003 made insertion of the new analyzers problematic. In particular, two patterns were quite common and arguably should be allowed:
JoinableTask.Taskproperty on the result of aJoinableTaskFactory.RunAsyncmethod call. In general, if a method returns an object, awaiting one of that object's properties might not be fair game, but it seems more likely than not. So in this change, that is now allowed.TaskCompletionSource<T>.Taskproperty on a TCS that was created in that same method. Test methods often do this. Awaiting a TCS.Task property is never guaranteed to be safe as far as analyzers can tell (though the 3rd threading rule can make it safe), and when common patterns appear that are typically safe, we should allow it.As a part of this change, I updated a couple utility functions, including in at least one way that wasn't directly relevant to these changes but seemed like a good change considering the nature of the analyzer bugs that keep coming in (e.g. recognizing LocalFunctionDeclarationSyntax in more places).
I also made the diagnostic's location focus on the awaited object itself rather than including the
.ConfigurueAwaitthat might follow it.