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

VSTHRD114: Do not return null from non-async Task method #596

Merged
merged 14 commits into from
Apr 19, 2020
Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
namespace Microsoft.VisualStudio.Threading.Analyzers.Tests
{
using System.Threading.Tasks;
using Xunit;
using Verify = CSharpCodeFixVerifier<VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzer, CodeAnalysis.Testing.EmptyCodeFixProvider>;

public class VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzerTests
{
[Fact]
public async Task TaskOfTReturnsNull_Diagnostic()
{
var test = @"
using System.Threading.Tasks;

class Test
{
public Task<object> GetTaskObj()
{
return null;
}
}
";
await new Verify.Test
{
TestCode = test,
ExpectedDiagnostics = { Verify.Diagnostic().WithSpan(8, 9, 8, 21), },
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
}.RunAsync();
}

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

class Test
{
public Task GetTask()
{
return null;
}
}
";
await new Verify.Test
{
TestCode = test,
ExpectedDiagnostics = { Verify.Diagnostic().WithSpan(8, 9, 8, 21), },
}.RunAsync();
}

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

class Test
{
public async Task<object> GetTaskObj()
{
return null;
}
}
";
await new Verify.Test
{
TestCode = test,
}.RunAsync();
}

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

class Test
{
public Task<object> GetTaskObj()
{
Task<object> o = null;
return o;
AArnott marked this conversation as resolved.
Show resolved Hide resolved
}
}
";
await new Verify.Test
{
TestCode = test,
}.RunAsync();
}

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

class Test
{
public Task<object> GetTaskObj(bool b)
{
return b ? default(Task<object>) : null;
AArnott marked this conversation as resolved.
Show resolved Hide resolved
}
}
";
await new Verify.Test
{
TestCode = test,
}.RunAsync();
}

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

class Test
{
public Task<object> GetTaskObj(string s)
{
if (string.IsNullOrEmpty(s))
{
{|VSTHRD112:return null;|}
}

{|VSTHRD112:return null;|}
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
}
}
";
await new Verify.Test
{
TestCode = test,
}.RunAsync();
}
}
}
18 changes: 18 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -309,4 +309,10 @@ Use AsyncLazy&lt;T&gt; instead.</value>
<value>Call ThrowIfCancellationRequested()</value>
<comment>"ThrowIfCancellationRequested" is a method name and should not be translated.</comment>
</data>
</root>
<data name="VSTHRD112_MessageFormat" xml:space="preserve">
<value>Avoid returning null from a non-async awaitable method</value>
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="VSTHRD112_Title" xml:space="preserve">
<value>Avoid returning null from a non-async awaitable method</value>
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
</data>
</root>
30 changes: 30 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/Utils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,36 @@ internal static Action<OperationAnalysisContext> DebuggableWrapper(Action<Operat
};
}

internal static Action<OperationBlockStartAnalysisContext> DebuggableWrapper(Action<OperationBlockStartAnalysisContext> handler)
{
return ctxt =>
{
try
{
handler(ctxt);
}
catch (OperationCanceledException)
{
throw;
}
catch (Exception ex) when (LaunchDebuggerExceptionFilter())
{
var messageBuilder = new StringBuilder();
messageBuilder.AppendLine("Analyzer failure while processing syntax at");
Copy link
Member

Choose a reason for hiding this comment

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

Do multi-line exception messages get printed alright at all the interesting points? (e.g. command line builds, VS error list, infobar, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I don't know and I don't know all contexts I will need to test so I will move to a single-line message. From my tests it is ok with infobar and error list.


foreach (var operation in ctxt.OperationBlocks)
{
var lineSpan = operation.Syntax.GetLocation()?.GetLineSpan();
messageBuilder.AppendLine($"- {operation.Syntax.SyntaxTree.FilePath}({lineSpan?.StartLinePosition.Line + 1},{lineSpan?.StartLinePosition.Character + 1}). Syntax: {operation.Syntax}");
}

messageBuilder.AppendLine($"{ex.GetType()} {ex.Message}");

throw new Exception(messageBuilder.ToString(), ex);
}
};
}

internal static ExpressionSyntax IsolateMethodName(InvocationExpressionSyntax invocation)
{
if (invocation == null)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
namespace Microsoft.VisualStudio.Threading.Analyzers
{
using System.Collections.Immutable;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

/// <summary>
/// Finds await expressions on <see cref="Task"/> that do not use <see cref="Task.ConfigureAwait(bool)"/>.
/// Also works on <see cref="ValueTask"/>.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add VB as a supported language here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want me to introduce vbnet tests?

Copy link
Member

Choose a reason for hiding this comment

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

Just one or two sanity tests in VB would be great, yes.

public class VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzer : DiagnosticAnalyzer
{
public const string Id = "VSTHRD112";

internal static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor(
id: Id,
title: new LocalizableResourceString(nameof(Strings.VSTHRD112_Title), Strings.ResourceManager, typeof(Strings)),
messageFormat: new LocalizableResourceString(nameof(Strings.VSTHRD112_MessageFormat), Strings.ResourceManager, typeof(Strings)),
helpLinkUri: Utils.GetHelpLink(Id),
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
category: "Usage",
defaultSeverity: DiagnosticSeverity.Warning,
isEnabledByDefault: true);

/// <inheritdoc />
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Descriptor);

/// <inheritdoc />
public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze);

context.RegisterOperationBlockStartAction(Utils.DebuggableWrapper(context => AnalyzeOperationBlockStart(context)));
}

private static void AnalyzeOperationBlockStart(OperationBlockStartAnalysisContext context)
{
if (context.OwningSymbol is IMethodSymbol method &&
!method.IsAsync &&
Utils.IsTask(method.ReturnType))
{
context.RegisterOperationAction(Utils.DebuggableWrapper(context => AnalyzerReturnOperation(context)), OperationKind.Return);
}
}

private static void AnalyzerReturnOperation(OperationAnalysisContext context)
{
var returnOperation = (IReturnOperation)context.Operation;

if (returnOperation.ReturnedValue.ConstantValue.HasValue &&
returnOperation.ReturnedValue.ConstantValue.Value == null)
{
context.ReportDiagnostic(Diagnostic.Create(Descriptor, returnOperation.Syntax.GetLocation()));
Evangelink marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.cs.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ Použijte místo toho AsyncLazy&lt;T&gt;.</target>
<target state="translated">Používat ConfigureAwait(bool)</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">V názvech metod, které vrací typ awaitable, používejte příponu „Async“.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.de.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ Verwenden Sie stattdessen "AsyncLazy&lt;T&gt;".</target>
<target state="translated">ConfigureAwait(bool) verwenden</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">Verwenden Sie das Suffix "Async" in Namen von Methoden, die einen Typ zurückgeben, der "await" unterstützt.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.es.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ Use AsyncLazy&lt;T&gt; en su lugar.</target>
<target state="translated">Usar ConfigureAwait(bool)</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">Use el sufijo "Async" en nombres de métodos que devuelven un tipo que admite await.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.fr.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ Utilisez AsyncLazy&lt;T&gt; à la place.</target>
<target state="translated">Utiliser ConfigureAwait(bool)</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">Utilisez le suffixe "Async" dans les noms des méthodes qui retournent un type awaitable.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.it.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ In alternativa, usare AsyncLazy&lt;T&gt;.</target>
<target state="translated">Usa ConfigureAwait(bool)</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">Usare il suffisso "Async" in nomi di metodi che restituiscono un tipo awaitable.</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.ja.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ Use AsyncLazy&lt;T&gt; instead.</source>
<target state="translated">ConfigureAwait(bool) を使用する</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">待機可能な型を戻すメソッドの名前に "Async" サフィックスを使用します。</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.VisualStudio.Threading.Analyzers/xlf/Strings.ko.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ Use AsyncLazy&lt;T&gt; instead.</source>
<target state="translated">ConfigureAwait(bool)를 사용합니다.</target>
<note from="MultilingualBuild" annotates="source" priority="2">"ConfigureAwait(bool)" is a reference and should NOT be translated.</note>
</trans-unit>
<trans-unit id="VSTHRD112_MessageFormat">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD112_Title">
<source>Avoid returning null from a non-async awaitable method</source>
<target state="new">Avoid returning null from a non-async awaitable method</target>
<note />
</trans-unit>
<trans-unit id="VSTHRD200_AddAsync_MessageFormat" translate="yes" xml:space="preserve">
<source>Use "Async" suffix in names of methods that return an awaitable type.</source>
<target state="translated">대기할 수 있는 형식을 반환하는 메서드의 이름에 "Async" 접미사를 사용합니다.</target>
Expand Down
Loading