Skip to content

Add code fix provider for VSTHRD109 #397

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

Merged
AArnott merged 1 commit into from
Oct 9, 2018
+578 −94
Merged

Add code fix provider for VSTHRD109 #397

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
177 changes: 172 additions & 5 deletions ...VisualStudio.Threading.Analyzers.Tests/VSTHRD109AvoidAssertInAsyncMethodsAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
using System.Threading.Tasks;
using Xunit;
using Verify = CSharpCodeFixVerifier<VSTHRD109AvoidAssertInAsyncMethodsAnalyzer, CodeAnalysis.Testing.EmptyCodeFixProvider>;
using Verify = CSharpCodeFixVerifier<VSTHRD109AvoidAssertInAsyncMethodsAnalyzer, VSTHRD109AvoidAssertInAsyncMethodsCodeFix>;

public class VSTHRD109AvoidAssertInAsyncMethodsAnalyzerTests
{
Expand All @@ -20,10 +20,58 @@ async Task FooAsync() {
await Task.Yield();
}
}
";

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

class Test {
async Task FooAsync() {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
await Task.Yield();
}
}
";

var expected = Verify.Diagnostic().WithSpan(8, 22, 8, 42);
await Verify.VerifyAnalyzerAsync(test, expected);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

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

class Test {
async Task FooAsync(CancellationToken ct) {
ThreadHelper.ThrowIfNotOnUIThread();
await Task.Yield();
}
}
";

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

class Test {
async Task FooAsync(CancellationToken ct) {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(ct);
await Task.Yield();
}
}
";

var expected = Verify.Diagnostic().WithSpan(9, 22, 9, 42);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

[Fact]
Expand All @@ -40,10 +88,23 @@ Task<int> FooAsync() {
return Task.FromResult(1);
}
}
";

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

class Test {
async Task<int> FooAsync() {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
return 1;
}
}
";

var expected = Verify.Diagnostic().WithSpan(8, 22, 8, 42);
await Verify.VerifyAnalyzerAsync(test, expected);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

[Fact]
Expand Down Expand Up @@ -98,10 +159,64 @@ void Foo() {
});
}
}
";

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

class Test {
void Foo() {
Task.Run(async delegate {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
await Task.Yield();
});
}
}
";

var expected = Verify.Diagnostic().WithSpan(9, 26, 9, 46);
await Verify.VerifyAnalyzerAsync(test, expected);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

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

class Test {
void Foo(CancellationToken ct) {
Task.Run(async delegate {
ThreadHelper.ThrowIfNotOnUIThread();
await Task.Yield();
});
}
}
";

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

class Test {
void Foo(CancellationToken ct) {
Task.Run(async delegate {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(ct);
await Task.Yield();
});
}
}
";

var expected = Verify.Diagnostic().WithSpan(10, 26, 10, 46);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

[Fact]
Expand All @@ -112,6 +227,43 @@ public async Task TaskReturningLambdaInsideVoidMethod_GeneratesDiagnostic()
using Microsoft.VisualStudio.Shell;
using Task = System.Threading.Tasks.Task;

class Test {
void Foo() {
Task.Run<int>(() => {
ThreadHelper.ThrowIfNotOnUIThread();
return Task.FromResult(1);
});
}
}
";

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

class Test {
void Foo() {
Task.Run<int>(async () => {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
return 1;
});
}
}
";

var expected = Verify.Diagnostic().WithSpan(9, 26, 9, 46);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

[Fact(Skip = "Fails because the semantic model claims the anonymous func returns Task instead of Task<int>. We should probably skip that check and always preserve return statements.")]

@sharwell sharwell Oct 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 This may work in newer versions of Roslyn with later language versions (bestest betterness). If the underlying issue is the inability to distinguish between Task.Run(Func<Task>) and Task.Run<TResult>(Func<Task<TResult>>), you can force a disambiguation in the test by calling Task.Run<int> instead of just Task.Run.

@AArnott AArnott Oct 8, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, using Task.Run<int> does resolve the issue (see the test directly above this one, on which I applied that workaround).

Interestingly though, although this compiles:

Task<int> t = Task.Run(() => Task.FromResult(1));

Even in that case, which compiles, Roslyn still claims that the delegate returns Task. Yet if it really did, the result couldn't be assigned to Task<int> as I did. So what's up with that? Would 'bestest betterness' actually solve this?

@sharwell sharwell Oct 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even in that case, which compiles,

Are you compiling with the same version of Roslyn as you are using to run the tests?

@AArnott AArnott Oct 8, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No. I compile the analyzers against Roslyn 1.2, and the tests run with 2.3. Evidently the dependency on the new Roslyn test framework bumped up the version of Roslyn I'm testing with.
How does that affect this investigation?

@sharwell sharwell Oct 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Evidently the dependency on the new Roslyn test framework bumped up the version of Roslyn I'm testing with.

This shouldn't be the case. The test framework is compiled against Roslyn 1.0.1. The version of Roslyn used for testing is controlled by the following line:

https://github.com/Microsoft/vs-threading/blob/9df4cbf729555494c4a3b2ef9902fbb7dfa81100/src/Microsoft.VisualStudio.Threading.Analyzers.Tests/Microsoft.VisualStudio.Threading.Analyzers.Tests.csproj#L23

Git blame indicates this was updated to Roslyn 2.3.2 in #190.

@AArnott AArnott Oct 9, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see. I can certainly try to match the versions up (presumably by rolling back the version in the tests). What do you predict will happen? Will it resolve the issue? If so, why?

public async Task TaskReturningLambdaInsideVoidMethod_NoTypeArg_GeneratesDiagnostic()
{
var test = @"
using System.Threading.Tasks;
using Microsoft.VisualStudio.Shell;
using Task = System.Threading.Tasks.Task;

class Test {
void Foo() {
Task.Run(() => {
Expand All @@ -120,10 +272,25 @@ void Foo() {
});
}
}
";

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

class Test {
void Foo() {
Task.Run(async () => {
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
return 1;
});
}
}
";

var expected = Verify.Diagnostic().WithSpan(9, 26, 9, 46);
await Verify.VerifyAnalyzerAsync(test, expected);
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

[Fact]
Expand Down
96 changes: 74 additions & 22 deletions src/Microsoft.VisualStudio.Threading.Analyzers/CommonInterest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace Microsoft.VisualStudio.Threading.Analyzers
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
Expand Down Expand Up @@ -171,18 +172,19 @@ internal static IEnumerable<QualifiedMember> ReadMethods(AnalyzerOptions analyze
{
foreach (string line in ReadAdditionalFiles(analyzerOptions, fileNamePattern, cancellationToken))
{
Match match = MemberReferenceRegex.Match(line);
if (!match.Success)
{
throw new InvalidOperationException($"Parsing error on line: {line}");
}
yield return ParseAdditionalFileMethodLine(line);
}
}

string methodName = match.Groups["memberName"].Value;
string[] typeNameElements = match.Groups["typeName"].Value.Split(QualifiedIdentifierSeparators);
string typeName = typeNameElements[typeNameElements.Length - 1];
var containingType = new QualifiedType(typeNameElements.Take(typeNameElements.Length - 1).ToImmutableArray(), typeName);
yield return new QualifiedMember(containingType, methodName);
internal static async Task<ImmutableArray<QualifiedMember>> ReadMethodsAsync(CodeFixContext codeFixContext, Regex fileNamePattern, CancellationToken cancellationToken)
{
var result = ImmutableArray.CreateBuilder<QualifiedMember>();
foreach (string line in await ReadAdditionalFilesAsync(codeFixContext.Document.Project.AdditionalDocuments, fileNamePattern, cancellationToken))
{
result.Add(ParseAdditionalFileMethodLine(line));
}

return result.ToImmutable();
}

internal static IEnumerable<TypeMatchSpec> ReadTypesAndMembers(AnalyzerOptions analyzerOptions, Regex fileNamePattern, CancellationToken cancellationToken)
Expand All @@ -205,6 +207,32 @@ internal static IEnumerable<TypeMatchSpec> ReadTypesAndMembers(AnalyzerOptions a
}
}

internal static async Task<ImmutableArray<string>> ReadAdditionalFilesAsync(IEnumerable<TextDocument> additionalFiles, Regex fileNamePattern, CancellationToken cancellationToken)
{
if (additionalFiles == null)
{
throw new ArgumentNullException(nameof(additionalFiles));
}

if (fileNamePattern == null)
{
throw new ArgumentNullException(nameof(fileNamePattern));
}

var docs = from doc in additionalFiles.OrderBy(x => x.FilePath, StringComparer.Ordinal)
let fileName = Path.GetFileName(doc.Name)
where fileNamePattern.IsMatch(fileName)
select doc;
var result = ImmutableArray.CreateBuilder<string>();
foreach (var doc in docs)
{
var text = await doc.GetTextAsync(cancellationToken);
result.AddRange(ReadLinesFromAdditionalFile(text));
}

return result.ToImmutable();
}

internal static IEnumerable<string> ReadAdditionalFiles(AnalyzerOptions analyzerOptions, Regex fileNamePattern, CancellationToken cancellationToken)
{
if (analyzerOptions == null)
Expand All @@ -217,21 +245,12 @@ internal static IEnumerable<string> ReadAdditionalFiles(AnalyzerOptions analyzer
throw new ArgumentNullException(nameof(fileNamePattern));
}

var lines = from file in analyzerOptions.AdditionalFiles.OrderBy(x => x.Path, StringComparer.Ordinal)
var docs = from file in analyzerOptions.AdditionalFiles.OrderBy(x => x.Path, StringComparer.Ordinal)
let fileName = Path.GetFileName(file.Path)
where fileNamePattern.IsMatch(fileName)
let text = file.GetText(cancellationToken)
from line in text.Lines
select line;
foreach (TextLine line in lines)
{
string lineText = line.ToString();

if (!string.IsNullOrWhiteSpace(lineText) && !lineText.StartsWith("#"))
{
yield return lineText;
}
}
select text;
return docs.SelectMany(ReadLinesFromAdditionalFile);
}

internal static bool Contains(this ImmutableArray<QualifiedMember> methods, ISymbol symbol)
Expand Down Expand Up @@ -269,6 +288,39 @@ internal static bool Contains(this ImmutableArray<TypeMatchSpec> types, ITypeSym
return !matching.IsEmpty && !matching.InvertedLogic;
}

private static IEnumerable<string> ReadLinesFromAdditionalFile(SourceText text)
{
if (text == null)
{
throw new ArgumentNullException(nameof(text));
}

foreach (TextLine line in text.Lines)
{
string lineText = line.ToString();

if (!string.IsNullOrWhiteSpace(lineText) && !lineText.StartsWith("#"))
{
yield return lineText;
}
}
}

private static QualifiedMember ParseAdditionalFileMethodLine(string line)
{
Match match = MemberReferenceRegex.Match(line);
if (!match.Success)
{
throw new InvalidOperationException($"Parsing error on line: {line}");
}

string methodName = match.Groups["memberName"].Value;
string[] typeNameElements = match.Groups["typeName"].Value.Split(QualifiedIdentifierSeparators);
string typeName = typeNameElements[typeNameElements.Length - 1];
var containingType = new QualifiedType(typeNameElements.Take(typeNameElements.Length - 1).ToImmutableArray(), typeName);
return new QualifiedMember(containingType, methodName);
}

internal struct TypeMatchSpec
{
internal TypeMatchSpec(QualifiedType type, QualifiedMember member, bool inverted)
Expand Down
Loading