Skip to content

Commit

Permalink
Merge pull request #575 from AArnott/fixVSTHRD003
Browse files Browse the repository at this point in the history 
VSTHRD003 fixes: more good and less bad
  • Loading branch information
@AArnott
AArnott authored Dec 19, 2019
2 parents c442248 + e559b71 commit 68a6089
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 29 deletions.
68 changes: 61 additions & 7 deletions src/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,38 @@ public Task GetTask()
await Verify.VerifyAnalyzerAsync(test, expected);
}

[Fact]
public async Task ReportWarningWhenTaskIsReturnedDirectlyFromMethodViaExpressionBody()
{
var test = @"
using System.Threading.Tasks;
class Tests
{
private Task task;
public Task GetTask() => task;
}
";
var expected = this.CreateDiagnostic(8, 30, 4);
await Verify.VerifyAnalyzerAsync(test, expected);
}

[Fact]
public async Task ReportWarningWhenTaskParameterIsReturnedDirectlyFromMethodViaExpressionBody()
{
var test = @"
using System.Threading.Tasks;
class Tests
{
public Task GetTask(Task task) => task;
}
";
var expected = this.CreateDiagnostic(6, 39, 4);
await Verify.VerifyAnalyzerAsync(test, expected);
}

[Fact]
public async Task ReportWarningWhenTaskIsReturnedAwaitedFromMethod()
{
Expand Down Expand Up @@ -989,19 +1021,41 @@ public void Test() {
public async Task DoNotReportWarningWhenCompletedTaskIsReturnedDirectlyFromMethod()
{
var test = @"
using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.Threading;
class Tests
{
public Task GetTask()
{
return Task.CompletedTask;
}
public Task GetTask2()
static readonly Task MyCompletedTask = Task.CompletedTask;
static readonly Task MyCompletedTask1 = TplExtensions.CompletedTask;
static readonly Task MyCompletedTask2 = TplExtensions.CanceledTask;
static readonly Task MyCompletedTask3 = TplExtensions.TrueTask;
static readonly Task MyCompletedTask4 = TplExtensions.FalseTask;
static readonly Task MyCompletedTask5 = Task.FromCanceled(new CancellationToken(true));
static readonly Task MyCompletedTask6 = Task.FromException(new Exception());
public Task GetTask(int i)
{
return TplExtensions.CompletedTask;
switch (i)
{
case 1: return Task.CompletedTask;
case 2: return TplExtensions.CompletedTask;
case 3: return TplExtensions.CanceledTask;
case 4: return TplExtensions.TrueTask;
case 5: return TplExtensions.FalseTask;
case 6: return MyCompletedTask;
case 7: return MyCompletedTask1;
case 8: return MyCompletedTask2;
case 9: return MyCompletedTask3;
case 10: return MyCompletedTask4;
case 11: return MyCompletedTask5;
case 12: return MyCompletedTask6;
case 13: return Task.FromCanceled(new CancellationToken(true));
case 14: return Task.FromException(new Exception());
default: return null;
}
}
}
";
Expand Down
17 changes: 17 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,21 @@ internal static class TplExtensions
/// </summary>
internal const string CompletedTask = "CompletedTask";

/// <summary>
/// The name of the CanceledTask field.
/// </summary>
internal const string CanceledTask = "CanceledTask";

/// <summary>
/// The name of the TrueTask field.
/// </summary>
internal const string TrueTask = "TrueTask";

/// <summary>
/// The name of the FalseTask field.
/// </summary>
internal const string FalseTask = "FalseTask";

internal static readonly IReadOnlyList<string> Namespace = Namespaces.MicrosoftVisualStudioThreading;
}

Expand Down Expand Up @@ -155,6 +170,8 @@ internal static class Task

internal const string FullName = "System.Threading.Tasks." + TypeName;

internal const string CompletedTask = nameof(System.Threading.Tasks.Task.CompletedTask);

internal static readonly IReadOnlyList<string> Namespace = Namespaces.SystemThreadingTasks;
}

Expand Down
8 changes: 6 additions & 2 deletions src/Microsoft.VisualStudio.Threading.Analyzers/Utils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,14 @@ internal static ExpressionSyntax IsolateMethodName(InvocationExpressionSyntax in
/// <summary>
/// Gets a semantic model for the given <see cref="SyntaxTree"/>.
/// </summary>
internal static SemanticModel GetNewOrExistingSemanticModel(this SyntaxNodeAnalysisContext context, SyntaxTree syntaxTree)
internal static bool TryGetNewOrExistingSemanticModel(this SyntaxNodeAnalysisContext context, SyntaxTree syntaxTree, out SemanticModel semanticModel)
{
// Avoid calling GetSemanticModel unless we need it since it's much more expensive to create a new one than to reuse one.
return context.Node.SyntaxTree != syntaxTree ? context.Compilation.GetSemanticModel(syntaxTree) : context.SemanticModel;
semanticModel =
context.Node.SyntaxTree == syntaxTree ? context.SemanticModel :
context.Compilation.ContainsSyntaxTree(syntaxTree) ? context.Compilation.GetSemanticModel(syntaxTree) :
null;
return semanticModel is object;
}

internal static bool IsEqualToOrDerivedFrom(ITypeSymbol type, ITypeSymbol expectedType)
Expand Down
86 changes: 66 additions & 20 deletions src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD003UseJtfRunAsyncAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,24 @@ public override void Initialize(AnalysisContext context)

context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeAwaitExpression), SyntaxKind.AwaitExpression);
context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeReturnStatement), SyntaxKind.ReturnStatement);
context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeArrowExpressionClause), SyntaxKind.ArrowExpressionClause);
context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeLambdaExpression), SyntaxKind.SimpleLambdaExpression);
context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeLambdaExpression), SyntaxKind.ParenthesizedLambdaExpression);
}

private void AnalyzeArrowExpressionClause(SyntaxNodeAnalysisContext context)
{
var arrowExpressionClause = (ArrowExpressionClauseSyntax)context.Node;
if (arrowExpressionClause.Parent is MethodDeclarationSyntax)
{
var diagnostic = this.AnalyzeAwaitedOrReturnedExpression(arrowExpressionClause.Expression, context, context.CancellationToken);
if (diagnostic is object)
{
context.ReportDiagnostic(diagnostic);
}
}
}

private void AnalyzeLambdaExpression(SyntaxNodeAnalysisContext context)
{
var lambdaExpression = (LambdaExpressionSyntax)context.Node;
Expand Down Expand Up @@ -120,7 +134,11 @@ private Diagnostic AnalyzeAwaitedOrReturnedExpression(ExpressionSyntax expressio

// Get the semantic model for the SyntaxTree for the given ExpressionSyntax, since it *may* not be in the same syntax tree
// as the original context.Node.
var semanticModel = context.GetNewOrExistingSemanticModel(expressionSyntax.SyntaxTree);
if (!context.TryGetNewOrExistingSemanticModel(expressionSyntax.SyntaxTree, out var semanticModel))
{
return null;
}

SymbolInfo symbolToConsider = semanticModel.GetSymbolInfo(expressionSyntax, cancellationToken);
if (CommonInterest.TaskConfigureAwait.Any(configureAwait => configureAwait.IsMatch(symbolToConsider.Symbol)))
{
Expand Down Expand Up @@ -148,12 +166,6 @@ private Diagnostic AnalyzeAwaitedOrReturnedExpression(ExpressionSyntax expressio
// If the field is readonly and initialized with Task.FromResult, it's OK.
if (fieldSymbol.IsReadOnly)
{
// Whitelist the TplExtensions.CompletedTask field.
if (fieldSymbol.Name == Types.TplExtensions.CompletedTask && fieldSymbol.ContainingType.Name == Types.TplExtensions.TypeName && fieldSymbol.BelongsToNamespace(Types.TplExtensions.Namespace))
{
return null;
}

// If we can find the source code for the field, we can check whether it has a field initializer
// that stores the result of a Task.FromResult invocation.
if (!fieldSymbol.DeclaringSyntaxReferences.Any())
Expand All @@ -164,23 +176,57 @@ private Diagnostic AnalyzeAwaitedOrReturnedExpression(ExpressionSyntax expressio

foreach (var syntaxReference in fieldSymbol.DeclaringSyntaxReferences)
{
if (syntaxReference.GetSyntax(cancellationToken) is VariableDeclaratorSyntax declarationSyntax &&
declarationSyntax.Initializer?.Value is InvocationExpressionSyntax invocationSyntax &&
invocationSyntax.Expression != null)
if (syntaxReference.GetSyntax(cancellationToken) is VariableDeclaratorSyntax declarationSyntax)
{
if (!context.Compilation.ContainsSyntaxTree(invocationSyntax.SyntaxTree))
if (declarationSyntax.Initializer?.Value is InvocationExpressionSyntax invocationSyntax &&
invocationSyntax.Expression != null)
{
// We can't look up the definition of the field. It *probably* is a precompleted cached task, so don't create a diagnostic.
return null;
}
if (!context.Compilation.ContainsSyntaxTree(invocationSyntax.SyntaxTree))
{
// We can't look up the definition of the field. It *probably* is a precompleted cached task, so don't create a diagnostic.
return null;
}

// Whitelist Task.From*() methods.
if (!context.TryGetNewOrExistingSemanticModel(invocationSyntax.SyntaxTree, out var declarationSemanticModel))
{
return null;
}

var declarationSemanticModel = context.GetNewOrExistingSemanticModel(invocationSyntax.SyntaxTree);
if (declarationSemanticModel.GetSymbolInfo(invocationSyntax.Expression, cancellationToken).Symbol is IMethodSymbol invokedMethod &&
invokedMethod.Name == nameof(Task.FromResult) &&
invokedMethod.ContainingType.Name == nameof(Task) &&
invokedMethod.ContainingType.BelongsToNamespace(Types.Task.Namespace))
if (declarationSemanticModel.GetSymbolInfo(invocationSyntax.Expression, cancellationToken).Symbol is IMethodSymbol invokedMethod &&
invokedMethod.ContainingType.Name == nameof(Task) &&
invokedMethod.ContainingType.BelongsToNamespace(Types.Task.Namespace) &&
(invokedMethod.Name == nameof(Task.FromResult) || invokedMethod.Name == nameof(Task.FromCanceled) || invokedMethod.Name == nameof(Task.FromException)))
{
return null;
}
}
else if (declarationSyntax.Initializer?.Value is MemberAccessExpressionSyntax memberAccessSyntax && memberAccessSyntax.Expression is object)
{
return null;
if (!context.TryGetNewOrExistingSemanticModel(memberAccessSyntax.SyntaxTree, out var declarationSemanticModel))
{
return null;
}

var definition = declarationSemanticModel.GetSymbolInfo(memberAccessSyntax, cancellationToken).Symbol;
if (definition is IFieldSymbol field)
{
// Whitelist the TplExtensions.CompletedTask and related fields.
if (field.ContainingType.Name == Types.TplExtensions.TypeName && field.BelongsToNamespace(Types.TplExtensions.Namespace) &&
(field.Name == Types.TplExtensions.CompletedTask || field.Name == Types.TplExtensions.CanceledTask || field.Name == Types.TplExtensions.TrueTask || field.Name == Types.TplExtensions.FalseTask))
{
return null;
}
}
else if (definition is IPropertySymbol property)
{
// Explicitly allow Task.CompletedTask
if (property.ContainingType.Name == Types.Task.TypeName && property.BelongsToNamespace(Types.Task.Namespace) &&
property.Name == Types.Task.CompletedTask)
{
return null;
}
}
}
}
}
Expand Down

0 comments on commit 68a6089

Please sign in to comment.