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

Fix VSTHRD003 to allow for awaiting more Task properties #1252

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Oct 25, 2023

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.

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 .ConfigurueAwait that might follow it.

@AArnott AArnott added this to the v17.9 milestone Oct 25, 2023
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.
@AArnott AArnott merged commit cf86d12 into microsoft:main Oct 25, 2023
6 checks passed
@AArnott AArnott deleted the analyzerFixes branch October 25, 2023 20:50
@sandyarmstrong
Copy link
Member

Awaiting a TaskCompletionSource.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.

@AArnott I still see this scenario triggering VSTHRD003 with 17.9.27, even with code as simple as return new TaskCompletionSource<object?>().Task.

@AArnott
Copy link
Member Author

AArnott commented Dec 12, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants