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.

Sign up for GitHub

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 VSTHRD012 false positive due to inaccessible overloads #1247

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Fix VSTHRD012 false positive due to inaccessible overloads #1247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Microsoft.VisualStudio.Threading.Analyzers/AbstractVSTHRD012SpecifyJtfWhereAllowed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,10 @@ private static void AnalyzeCall(OperationAnalysisContext context, Location locat
// The method being invoked doesn't take any JTC/JTF parameters.
// Look for an overload that does.
bool preferableAlternativesExist = otherOverloads
.Where(m => !m.IsObsolete())
.Any(m => m.Parameters.Skip(m.IsExtensionMethod ? 1 : 0).Any(IsImportantJtfParameter));
.Any(m =>
!m.IsObsolete() &&
m.Parameters.Skip(m.IsExtensionMethod ? 1 : 0).Any(IsImportantJtfParameter) &&
context.ContainingSymbol.FindContainingNamedOrAssemblySymbol() is ISymbol containingSymbol && context.Compilation.IsSymbolAccessibleWithin(m, containingSymbol));
Copy link

Choose a reason for hiding this comment

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

consider splitting each check onto it's own line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I generally like what you're proposing. This pair in particular share a line because it's really the second check that matters, but the first check is a special check that must be met before the second check can be allowed. All the 'lines' here could be in any order, but are sorted in order of runtime execution cost. So by putting these two checks on the same line, I make it easier to see that they are connected and in a particularly required order.

if (preferableAlternativesExist)
{
Diagnostic diagnostic = Diagnostic.Create(
Expand Down
16 changes: 16 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,22 @@ internal static IOperation FindFinalAncestor(IOperation operation)
return default;
}

internal static ISymbol? FindContainingNamedOrAssemblySymbol(this ISymbol? symbol)
{
ISymbol? candidate = symbol;
while (candidate is not null)
{
if (candidate is INamedTypeSymbol or IAssemblySymbol)
{
return candidate;
}

candidate = candidate.ContainingSymbol;
}

return null;
}

private static bool IsSymbolTheRightType(ISymbol symbol, string typeName, IReadOnlyList<string> namespaces)
{
var fieldSymbol = symbol as IFieldSymbol;
Expand Down
57 changes: 57 additions & 0 deletions .../Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD012SpecifyJtfWhereAllowedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,4 +254,61 @@ void F() {

await CSVerify.VerifyAnalyzerAsync(test);
}

[Fact]
public async Task InaccessibleMembers_Private_GeneratesNoWarning()
{
string test = """
using System;
using Microsoft.VisualStudio.Threading;

static class Extensions
{
public static void OnMainThread(Action action) => OnMainThread(null, action);

private static void OnMainThread(JoinableTaskFactory factory, Action action) => factory.Run(async delegate
{
await factory.SwitchToMainThreadAsync();
action();
});
}

class Foo
{
void Bar()
{
Extensions.OnMainThread(() => { });
}
}
""";

await CSVerify.VerifyAnalyzerAsync(test);
}

[Fact]
public async Task AccessibleMembers_Private_GeneratesWarning()
{
string test = """
using System;
using Microsoft.VisualStudio.Threading;

static class Extensions
{
public static void OnMainThread(Action action) => OnMainThread(null, action);

private static void OnMainThread(JoinableTaskFactory factory, Action action) => factory.Run(async delegate
{
await factory.SwitchToMainThreadAsync();
action();
});

static void Bar()
{
[|OnMainThread|](() => { });
}
}
""";

await CSVerify.VerifyAnalyzerAsync(test);
}
}