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

Prevent SemaphoreSlim.Wait(0) from triggering CA1849 #7310

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

CollinAlpert
Copy link
Contributor

Affected analyzer: UseAsyncMethodInAsyncContext
Affected diagnostic IDs: CA1849

This PR suppresses CA1849 for calls to SemaphoreSlim.Wait(0), SemaphoreSlim.Wait(0, CancellationToken), SemaphoreSlim.Wait(TimeSpan.Zero) and SemaphoreSlim.Wait(TimeSpan.Zero, CancellationToken).

Fixes #7271

@CollinAlpert CollinAlpert requested a review from a team as a code owner May 10, 2024 17:56
Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 98.41270% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 96.48%. Comparing base (bc8aca0) to head (6d45c87).

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7310    +/-   ##
========================================
  Coverage   96.47%   96.48%            
========================================
  Files        1443     1443            
  Lines      345400   345525   +125     
  Branches    11364    11369     +5     
========================================
+ Hits       333239   333368   +129     
+ Misses       9282     9275     -7     
- Partials     2879     2882     +3     

async Task M()
{
SemaphoreSlim s = new SemaphoreSlim(0);
s.Wait(0);
Copy link
Member

Choose a reason for hiding this comment

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

💭 I feel like all four of the "zero" cases could be in one [Theory], but broken out is fine.

IFieldSymbol? timeSpanZero = timeSpanType?.GetMembers(nameof(TimeSpan.Zero))
.OfType<IFieldSymbol>()
.FirstOrDefault();
ISet<IMethodSymbol> semaphoreSlimWaitMethods = semaphoreSlimType
Copy link
Member

Choose a reason for hiding this comment

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

  • ImmutableArray<T> will be at least as fast as ISet for such a small collection
  • Not all overloads are included here, so I suggested a clarifying name
Suggested change
ISet<IMethodSymbol> semaphoreSlimWaitMethods = semaphoreSlimType
ImmutableArray<IMethodSymbol> semaphoreSlimWaitWithTimeoutMethods = semaphoreSlimType

@@ -148,6 +165,21 @@ public override void Initialize(AnalysisContext context)
});
}

private static bool IsSemaphoreSlimWaitWithZeroArgumentInvocation(IInvocationOperation invocation, IFieldSymbol? timeSpanZero, ISet<IMethodSymbol> semaphoreSlimWaitMethods)
{
if (!semaphoreSlimWaitMethods.Contains(invocation.TargetMethod, SymbolEqualityComparer.Default) || invocation.Arguments.IsEmpty)
Copy link
Member

Choose a reason for hiding this comment

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

The arguments length check here is redundant, right? I would expect IOperation invocation to always have the same number of arguments as parameters, and we already know from the set creation above that only overloads with one or more parameters is contained in the collection.

Suggested change
if (!semaphoreSlimWaitMethods.Contains(invocation.TargetMethod, SymbolEqualityComparer.Default) || invocation.Arguments.IsEmpty)
if (!semaphoreSlimWaitMethods.Contains(invocation.TargetMethod, SymbolEqualityComparer.Default))

If you remove the check, feel free to include a Debug.Assert line to make it clear for future readers users why it was excluded.

@CollinAlpert
Copy link
Contributor Author

@sharwell Thanks for reviewing this!

@CollinAlpert
Copy link
Contributor Author

@sharwell Can this be merged?

@sharwell sharwell merged commit 333f90a into dotnet:main Jun 4, 2024
11 checks passed
@CollinAlpert CollinAlpert deleted the issue-7271 branch June 4, 2024 20:50
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.

CA1849 reports zero length timeouts as blocking (eg. SemaphoreSlim.Wait(0))
2 participants