This repository has been archived by the owner on Nov 8, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 18
Don't use Thread.Sleep() analyzers - initial implementation #50
Open
tmaczynski
wants to merge
22
commits into
DotNetAnalyzers:master
Choose a base branch
from
tmaczynski:dont_use_thread_sleep_pr
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
133c5f2
AsyncUsageAnalyzers.sln: update version of VisualStudio from 14.0.247…
tmaczynski 969c1f3
update version of Microsoft.CodeAnalysis.Analyzers and dependent pack…
tmaczynski 466b00b
Initial implementation of DontUseThreadSleep and DontUseThreadSleepIn…
tmaczynski ddc42f4
add test which checks if `Thread.Sleep(0)` is changed to` await Task.…
tmaczynski f4b7d8b
update DontUseThreadSleepCodeUniversalCodeFixProvider CodeAction's title
tmaczynski 9982504
basic syntax-based heuristic to handle Thread.Sleep(0) correctly
tmaczynski e64d692
add additional test cases when 'Thread.Sleep()' which should be conve…
tmaczynski f637170
DontUseThreadSleepTestsBase: add more InlineData which are semanticly…
tmaczynski 3fecffa
DontUseThreadSleepCodeUniversalCodeFixProvider: correctly handle valu…
tmaczynski 76f050a
move extension methods from InvocationExpressionSyntaxExtensions to E…
tmaczynski e9cd752
ExpressionSyntaxExtensions: simplify if statements to boolean expresion
tmaczynski 875fa4f
first working implementation checking if System.TimeSpan.Zero is used…
tmaczynski f0e2c3d
DontUseThreadSleepInAsyncCodeAnalyzer: reorder a method and a class
tmaczynski 7ca2925
delete comments with contributor name
tmaczynski b896415
fix typo in RegisterCodeFixForDiagnostic method name
tmaczynski 9272f06
DontUseThreadSleepCodeUniversalCodeFixProvider: don't export code fix…
tmaczynski f173b05
update documentation for Thread.Sleep-related analyzers
tmaczynski afe96a5
ExpressionSyntaxExtensions.cs: refactor TryGet... methods. Now they r…
tmaczynski 2cdb5a0
move DontUseThreadSleep.md and DontUseThreadSleepInAsyncCode.md to a …
tmaczynski 28c4e56
add new line in DontUseThreadSleepInAsyncCode.md
tmaczynski 3b30f21
DontUseThreadSleep.md: add note that using reset event might be an al…
tmaczynski a49bd3a
use backticks to format code in md files
tmaczynski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
160 changes: 160 additions & 0 deletions
160
...ers/AsyncUsageAnalyzers.CodeFixes/Usage/DontUseThreadSleepCodeUniversalCodeFixProvider.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,160 @@ | ||
// Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. | ||
// Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information. | ||
|
||
namespace AsyncUsageAnalyzers.Usage | ||
{ | ||
using System.Collections.Immutable; | ||
using System.Composition; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Helpers; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CodeActions; | ||
using Microsoft.CodeAnalysis.CodeFixes; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Text; | ||
|
||
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(DontUseThreadSleepCodeUniversalCodeFixProvider))] | ||
[Shared] | ||
internal class DontUseThreadSleepCodeUniversalCodeFixProvider : CodeFixProvider | ||
{ | ||
private static readonly ImmutableArray<string> FixableDiagnostics = | ||
ImmutableArray.Create(DontUseThreadSleepAnalyzer.DiagnosticId, DontUseThreadSleepInAsyncCodeAnalyzer.DiagnosticId); | ||
|
||
public override ImmutableArray<string> FixableDiagnosticIds => FixableDiagnostics; | ||
|
||
/// <inheritdoc/> | ||
public override FixAllProvider GetFixAllProvider() | ||
{ | ||
return CustomFixAllProviders.BatchFixer; | ||
} | ||
|
||
public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
foreach (var diagnostic in context.Diagnostics) | ||
{ | ||
if (diagnostic.Id == DontUseThreadSleepInAsyncCodeAnalyzer.DiagnosticId) | ||
{ | ||
RegisterCodeFixForDiagnostic(context, diagnostic); | ||
} | ||
else if (diagnostic.Id == DontUseThreadSleepAnalyzer.DiagnosticId) | ||
{ | ||
var document = context.Document; | ||
|
||
var root = await document.GetSyntaxRootAsync().ConfigureAwait(false); | ||
var invocationExpression = root.FindNode(TextSpan.FromBounds(diagnostic.Location.SourceSpan.Start, diagnostic.Location.SourceSpan.End), getInnermostNodeForTie: true) as InvocationExpressionSyntax; | ||
|
||
if (invocationExpression == null) | ||
{ | ||
return; | ||
} | ||
|
||
if (invocationExpression.IsInsideAsyncCode()) | ||
{ | ||
RegisterCodeFixForDiagnostic(context, diagnostic); | ||
} | ||
} | ||
} | ||
} | ||
|
||
private static void RegisterCodeFixForDiagnostic(CodeFixContext context, Diagnostic diagnostic) | ||
{ | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
"Use `await Task.Delay(...)` or `await Task.Yield()`", | ||
cancellationToken => GetTransformedDocumentAsync(context.Document, diagnostic, cancellationToken)), | ||
diagnostic); | ||
} | ||
|
||
private static async Task<Document> GetTransformedDocumentAsync(Document document, Diagnostic diagnostic, CancellationToken cancellationToken) | ||
{ | ||
var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
var firstNodeWithCorrectSpan = root | ||
.FindNode(diagnostic.Location.SourceSpan); | ||
InvocationExpressionSyntax expression = firstNodeWithCorrectSpan | ||
.DescendantNodesAndSelf() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know Roslyn well enough to know if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know Roslyn very well either, but I think that I can justify that
|
||
.OfType<InvocationExpressionSyntax>() | ||
.First(); | ||
|
||
var arguments = expression.ArgumentList; | ||
|
||
var newExpression = await IsArgumentListWithZeroValueAsync(document, arguments).ConfigureAwait(true) | ||
? GenerateTaskYieldExpression() | ||
: GenerateTaskDelayExpression(arguments); | ||
|
||
SyntaxNode newRoot = root.ReplaceNode(expression, newExpression.WithTriviaFrom(expression)); | ||
var newDocument = document.WithSyntaxRoot(newRoot); | ||
return newDocument; | ||
} | ||
|
||
private static async Task<bool> IsArgumentListWithZeroValueAsync(Document document, ArgumentListSyntax argumentListSyntax) | ||
{ | ||
// all valid overloads of Thread.Sleep() method take exactly one argument | ||
if (argumentListSyntax.Arguments.Count != 1) | ||
{ | ||
return false; | ||
} | ||
|
||
var argumentExpression = argumentListSyntax.Arguments.First().Expression; | ||
|
||
var argumentString = argumentExpression.ToString().Trim(); | ||
if (argumentString == "0" || argumentString == "TimeSpan.Zero") | ||
{ | ||
return true; | ||
} | ||
|
||
var semanticModel = await document.GetSemanticModelAsync().ConfigureAwait(false); | ||
var optionalValue = semanticModel.GetConstantValue(argumentExpression); | ||
if (optionalValue.HasValue && optionalValue.Value.Equals(0)) | ||
{ | ||
return true; | ||
} | ||
|
||
var memberAccessExpression = argumentExpression as MemberAccessExpressionSyntax; | ||
if (memberAccessExpression != null) | ||
{ | ||
IFieldSymbol propertySymbol = null; | ||
return memberAccessExpression.TryGetFieldSymbolByTypeNameAndMethodName(semanticModel, "System.TimeSpan", "Zero", out propertySymbol); | ||
} | ||
|
||
return false; | ||
} | ||
|
||
private static AwaitExpressionSyntax GenerateTaskDelayExpression(ArgumentListSyntax methodArgumentList) => | ||
SyntaxFactory.AwaitExpression( | ||
SyntaxFactory.InvocationExpression( | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.IdentifierName("System"), | ||
SyntaxFactory.IdentifierName("Threading")), | ||
SyntaxFactory.IdentifierName("Tasks")), | ||
SyntaxFactory.IdentifierName("Task")), | ||
SyntaxFactory.IdentifierName("Delay"))) | ||
.WithArgumentList(methodArgumentList)); | ||
|
||
private static AwaitExpressionSyntax GenerateTaskYieldExpression() => | ||
SyntaxFactory.AwaitExpression( | ||
SyntaxFactory.InvocationExpression( | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.MemberAccessExpression( | ||
SyntaxKind.SimpleMemberAccessExpression, | ||
SyntaxFactory.IdentifierName("System"), | ||
SyntaxFactory.IdentifierName("Threading")), | ||
SyntaxFactory.IdentifierName("Tasks")), | ||
SyntaxFactory.IdentifierName("Task")), | ||
SyntaxFactory.IdentifierName("Yield")))); | ||
} | ||
} |
14 changes: 7 additions & 7 deletions
14
AsyncUsageAnalyzers/AsyncUsageAnalyzers.CodeFixes/packages.config
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,15 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<packages> | ||
<package id="AsyncUsageAnalyzers" version="1.0.0-alpha003" targetFramework="portable45-net45+win8" developmentDependency="true" /> | ||
<package id="Microsoft.CodeAnalysis.Analyzers" version="1.0.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.Common" version="1.0.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.CSharp" version="1.0.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.CSharp.Workspaces" version="1.0.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.Workspaces.Common" version="1.0.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.Analyzers" version="1.1.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.Common" version="1.3.2" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.CSharp" version="1.3.2" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.CSharp.Workspaces" version="1.3.2" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.CodeAnalysis.Workspaces.Common" version="1.3.2" targetFramework="portable45-net45+win8" /> | ||
<package id="Microsoft.Composition" version="1.0.27" targetFramework="portable45-net45+win8" /> | ||
<package id="NuGet.CommandLine" version="2.8.3" targetFramework="portable45-net45+win8" /> | ||
<package id="StyleCop.Analyzers" version="1.0.0-rc3" targetFramework="portable45-net45+win8" developmentDependency="true" /> | ||
<package id="System.Collections.Immutable" version="1.1.36" targetFramework="portable45-net45+win8" /> | ||
<package id="System.Reflection.Metadata" version="1.0.21" targetFramework="portable45-net45+win8" /> | ||
<package id="System.Collections.Immutable" version="1.1.37" targetFramework="portable45-net45+win8" /> | ||
<package id="System.Reflection.Metadata" version="1.2.0" targetFramework="portable45-net45+win8" /> | ||
<package id="Tvl.NuGet.BuildTasks" version="1.0.0-alpha002" targetFramework="portable45-net45+win8" /> | ||
</packages> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for having the same message for both here? How does VS render the backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there were two separate messages for code fixes, semantic model would had to be awaited when the code fix is registered which would degrade performance. Currently, semantic model is awaited only if a user applies code fix. I don't think that user experience will be much better if there are two separate message.
I can change the code to include two separate code fix messages if you do think that it's worth it.
Backticks are displayed literally in VS2015 Update 2.
I believe that either quotes or backticks should be used to quote code - IMHO it improves readability. I used backticks because I wanted to be consistent with the AvoidAsyncSuffixCodeFixProvider. On the other hand no quotation characters are used in UseConfigureAwaitCodeFixProvider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea if it is worth it, just looked a bit strange with a roulette type code fix :)