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

VSTHRD103 calls out using IVsTask.Wait() in async methods #294

Merged
merged 1 commit into from
Jun 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -165,14 +165,21 @@ private void VerifyFix(string language, DiagnosticAnalyzer analyzer, CodeFixProv
}
}

Assert.True(fixApplied, "No code fix offered.");
if (newSources != null && newSources[0] != null)
{
Assert.True(fixApplied, "No code fix offered.");

// After applying all of the code fixes, compare the resulting string to the inputted one
int j = 0;
foreach (var document in project.Documents)
// After applying all of the code fixes, compare the resulting string to the inputted one
int j = 0;
foreach (var document in project.Documents)
{
var actual = GetStringFromDocument(document);
Assert.Equal(newSources[j++], actual, ignoreLineEndingDifferences: true);
}
}
else
{
var actual = GetStringFromDocument(document);
Assert.Equal(newSources[j++], actual, ignoreLineEndingDifferences: true);
Assert.False(fixApplied, "No code fix expected, but was offered.");
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,116 @@ async Task T() {
this.VerifyCSharpFix(test, withFix);
}

[Fact]
public void IVsTaskWaitInTaskReturningMethodGeneratesWarning()
{
var test = @"
using System.Threading.Tasks;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Task = System.Threading.Tasks.Task;

class Test {
Task T() {
IVsTask t = null;
t.Wait();
return Task.FromResult(1);
}
}
";

var withFix = @"
using System.Threading.Tasks;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Task = System.Threading.Tasks.Task;

class Test {
async Task T() {
IVsTask t = null;
await t;
}
}
";
this.expect = this.CreateDiagnostic(10, 11, 4);
this.VerifyCSharpDiagnostic(test, this.expect);
this.VerifyCSharpFix(test, withFix);
}

[Fact]
public void IVsTaskGetResultInTaskReturningMethodGeneratesWarning()
{
var test = @"
using System.Threading.Tasks;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Task = System.Threading.Tasks.Task;

class Test {
Task T() {
IVsTask t = null;
object result = t.GetResult();
return Task.FromResult(1);
}
}
";

var withFix = @"
using System.Threading.Tasks;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Task = System.Threading.Tasks.Task;

class Test {
async Task T() {
IVsTask t = null;
object result = await t;
}
}
";
this.expect = this.CreateDiagnostic(10, 27, 9);
this.VerifyCSharpDiagnostic(test, this.expect);
this.VerifyCSharpFix(test, withFix);
}

/// <summary>
/// Ensures we don't offer a code fix when the required using directive is not already present.
/// </summary>
[Fact]
public void IVsTaskGetResultInTaskReturningMethod_WithoutUsing_OffersNoFix()
{
var test = @"
using System.Threading.Tasks;
using Microsoft.VisualStudio.Shell.Interop;

class Test {
Task T() {
IVsTask t = null;
object result = t.GetResult();
return Task.FromResult(1);
}
}
";

string withFix = null;
//// var withFix = @"
//// using System.Threading.Tasks;
//// using Microsoft.VisualStudio.Shell;
//// using Microsoft.VisualStudio.Shell.Interop;
//// using Task = System.Threading.Tasks.Task;
////
//// class Test {
//// async Task T() {
//// IVsTask t = null;
//// object result = await t;
//// }
//// }
//// ";
this.expect = this.CreateDiagnostic(8, 27, 9);
this.VerifyCSharpDiagnostic(test, this.expect);
this.VerifyCSharpFix(test, withFix);
}

[Fact]
public void TaskOfTResultInTaskReturningMethodGeneratesWarning()
{
Expand Down Expand Up @@ -319,7 +429,7 @@ void T() {
}
";

this.expect.Locations = new[] { new DiagnosticResultLocation("Test0.cs", 9, 28, 9, 34) };
this.expect = this.CreateDiagnostic(9, 28, 6);
this.VerifyCSharpDiagnostic(test, this.expect);
this.VerifyCSharpFix(test, withFix);
}
Expand Down Expand Up @@ -874,7 +984,7 @@ public void NoDiagnosticAndNoExceptionForProperties()

class Test {
string Foo => string.Empty;
string Bar => string.Join(""a"", string.Empty);
string Bar => string.Join(""a"", string.Empty);
}
";

Expand Down Expand Up @@ -1078,5 +1188,14 @@ Task MethodAsync()

this.VerifyCSharpDiagnostic(test);
}

private DiagnosticResult CreateDiagnostic(int line, int column, int length, string messagePattern = null) =>
new DiagnosticResult
{
Id = this.expect.Id,
MessagePattern = messagePattern ?? this.expect.MessagePattern,
Severity = this.expect.Severity,
Locations = new[] { new DiagnosticResultLocation("Test0.cs", line, column, line, column + length) },
};
}
}
11 changes: 9 additions & 2 deletions src/Microsoft.VisualStudio.Threading.Analyzers/CommonInterest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ internal static class CommonInterest
new SyncBlockingMethod(new QualifiedMember(new QualifiedType(Namespaces.SystemRuntimeCompilerServices, nameof(TaskAwaiter)), nameof(TaskAwaiter.GetResult)), null),
};

internal static readonly IEnumerable<SyncBlockingMethod> SyncBlockingMethods = JTFSyncBlockers.Concat(ProblematicSyncBlockingMethods);
internal static readonly IEnumerable<SyncBlockingMethod> SyncBlockingMethods = JTFSyncBlockers.Concat(ProblematicSyncBlockingMethods).Concat(new[]
{
new SyncBlockingMethod(new QualifiedMember(new QualifiedType(Namespaces.MicrosoftVisualStudioShellInterop, "IVsTask"), "Wait"), extensionMethodNamespace: Namespaces.MicrosoftVisualStudioShell),
new SyncBlockingMethod(new QualifiedMember(new QualifiedType(Namespaces.MicrosoftVisualStudioShellInterop, "IVsTask"), "GetResult"), extensionMethodNamespace: Namespaces.MicrosoftVisualStudioShell),
});

internal static readonly IEnumerable<QualifiedMember> LegacyThreadSwitchingMethods = new[]
{
Expand Down Expand Up @@ -381,15 +385,18 @@ public bool IsMatch(ISymbol symbol)
[DebuggerDisplay("{" + nameof(Method) + "} -> {" + nameof(AsyncAlternativeMethodName) + "}")]
internal struct SyncBlockingMethod
{
public SyncBlockingMethod(QualifiedMember method, string asyncAlternativeMethodName)
public SyncBlockingMethod(QualifiedMember method, string asyncAlternativeMethodName = null, IReadOnlyList<string> extensionMethodNamespace = null)
{
this.Method = method;
this.AsyncAlternativeMethodName = asyncAlternativeMethodName;
this.ExtensionMethodNamespace = extensionMethodNamespace;
}

public QualifiedMember Method { get; }

public string AsyncAlternativeMethodName { get; }

public IReadOnlyList<string> ExtensionMethodNamespace { get; }
}
}
}
8 changes: 8 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/Namespaces.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,13 @@ internal static class Namespaces
"VisualStudio",
"Shell",
};

internal static readonly IReadOnlyList<string> MicrosoftVisualStudioShellInterop = new[]
{
"Microsoft",
"VisualStudio",
"Shell",
"Interop",
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ private static bool InspectMemberAccess(SyntaxNodeAnalysisContext context, Membe
if (item.Method.IsMatch(memberSymbol))
{
var location = memberAccessSyntax.Name.GetLocation();
var properties = ImmutableDictionary<string, string>.Empty;
var properties = ImmutableDictionary<string, string>.Empty
.Add(VSTHRD103UseAsyncOptionCodeFix.ExtensionMethodNamespaceKeyName, item.ExtensionMethodNamespace != null ? string.Join(".", item.ExtensionMethodNamespace) : string.Empty);
DiagnosticDescriptor descriptor;
var messageArgs = new List<object>(2);
messageArgs.Add(item.Method.Name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,50 @@ public class VSTHRD103UseAsyncOptionCodeFix : CodeFixProvider
{
internal const string AsyncMethodKeyName = "AsyncMethodName";

internal const string ExtensionMethodNamespaceKeyName = "ExtensionMethodNamespace";

private static readonly ImmutableArray<string> ReusableFixableDiagnosticIds = ImmutableArray.Create(
VSTHRD103UseAsyncOptionAnalyzer.Id);

/// <inheritdoc />
public override ImmutableArray<string> FixableDiagnosticIds => ReusableFixableDiagnosticIds;

/// <inheritdoc />
public override Task RegisterCodeFixesAsync(CodeFixContext context)
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var diagnostic = context.Diagnostics.FirstOrDefault(d => d.Properties.ContainsKey(AsyncMethodKeyName));
if (diagnostic != null)
{
context.RegisterCodeFix(new ReplaceSyncMethodCallWithAwaitAsync(context.Document, diagnostic), diagnostic);
}
// Check that the method we're replacing the sync blocking call with actually exists.
// This is particularly useful when the method is an extension method, since the using directive
// would need to be present (or the namespace imply it) and we don't yet add missing using directives.
bool asyncAlternativeExists = false;
string asyncMethodName = diagnostic.Properties[AsyncMethodKeyName];
if (string.IsNullOrEmpty(asyncMethodName))
{
asyncMethodName = "GetAwaiter";
}

return Task.FromResult<object>(null);
var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
var syntaxRoot = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
var blockingIdentifier = syntaxRoot.FindNode(diagnostic.Location.SourceSpan) as IdentifierNameSyntax;
var memberAccessExpression = blockingIdentifier?.Parent as MemberAccessExpressionSyntax;

// Check whether this code was already calling the awaiter (in a synchronous fashion).
asyncAlternativeExists |= memberAccessExpression?.Expression is InvocationExpressionSyntax invoke && invoke.Expression is MemberAccessExpressionSyntax parentMemberAccess && parentMemberAccess.Name.Identifier.Text == nameof(Task.GetAwaiter);

if (!asyncAlternativeExists)
{
// If we fail to recognize the container, assume it exists since the analyzer thought it would.
var container = memberAccessExpression != null ? semanticModel.GetTypeInfo(memberAccessExpression.Expression, context.CancellationToken).ConvertedType : null;
asyncAlternativeExists = container == null || semanticModel.LookupSymbols(diagnostic.Location.SourceSpan.Start, name: asyncMethodName, container: container, includeReducedExtensionMethods: true).Any();
}

if (asyncAlternativeExists)
{
context.RegisterCodeFix(new ReplaceSyncMethodCallWithAwaitAsync(context.Document, diagnostic), diagnostic);
}
}
}

/// <inheritdoc />
Expand Down Expand Up @@ -80,6 +108,8 @@ public override string Title

private string AlternativeAsyncMethod => this.diagnostic.Properties[AsyncMethodKeyName];

private string ExtensionMethodNamespace => this.diagnostic.Properties[ExtensionMethodNamespaceKeyName];

protected override async Task<Solution> GetChangedSolutionAsync(CancellationToken cancellationToken)
{
var document = this.document;
Expand Down