Skip to content

Commit

Permalink
Merge pull request #343 from manfred-brands/SuppressionCauseOutsideAs…
Browse files Browse the repository at this point in the history 
…sertMultiple

Fix NullReference Suppressor for more situations.
  • Loading branch information
@mikkelbu
mikkelbu authored Jan 25, 2021
2 parents 6e4e51e + 8c2b230 commit 268b0cb
Show file tree
Hide file tree
Showing 3 changed files with 227 additions and 23 deletions.
174 changes: 170 additions & 4 deletions ....analyzers.tests/DiagnosticSuppressors/DereferencePossiblyNullReferenceSuppressorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,16 +309,35 @@ await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
.ConfigureAwait(false);
}

[TestCase("var")]
[TestCase("Exception")]
public async Task ThrowsLocalDeclaration(string type)
[TestCase("var", "Throws")]
[TestCase("Exception", "Catch")]
public async Task ThrowsLocalDeclaration(string type, string assert)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
#nullable enable
[Test]
public void Test()
{{
{type} ex = Assert.{assert}<Exception>(() => throw new InvalidOperationException());
string m = ex.Message;
}}
");

await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
DereferencePossiblyNullReferenceSuppressor.SuppressionDescriptors["CS8602"], testCode)
.ConfigureAwait(false);
}

[TestCase("var", "CatchAsync")]
[TestCase("Exception", "ThrowsAsync")]
public async Task ThrowsAsyncLocalDeclaration(string type, string assert)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
#nullable enable
[Test]
public void Test()
{{
{type} ex = Assert.Throws<Exception>(() => throw new InvalidOperationException());
{type} ex = Assert.{assert}<Exception>(() => Task.Delay(0));
string m = ex.Message;
}}
");
Expand Down Expand Up @@ -406,5 +425,152 @@ await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
DereferencePossiblyNullReferenceSuppressor.SuppressionDescriptors["CS8604"], testCode)
.ConfigureAwait(false);
}

[Test]
public async Task ThrowAssignedOutsideAssertMultipleUsedInside()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
#nullable enable
[Test]
public void Test()
{
var e = Assert.Throws<Exception>(() => throw new Exception(""Test""));
Assert.Multiple(() =>
{
Assert.That(e.Message, Is.EqualTo(""Test""));
Assert.That(e.InnerException, Is.Null);
Assert.That(e.StackTrace, Is.Not.Empty);
});
}");

await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
DereferencePossiblyNullReferenceSuppressor.SuppressionDescriptors["CS8602"], testCode)
.ConfigureAwait(false);
}

[Test]
public async Task VariableAssertedOutsideAssertMultipleUsedInside()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
#nullable enable
[Test]
public void Test()
{
var e = GetPossibleException();
Assert.That(e, Is.Not.Null);
Assert.Multiple(() =>
{
Assert.That(e.Message, Is.EqualTo(""Test""));
Assert.That(e.InnerException, Is.Null);
Assert.That(e.StackTrace, Is.Not.Empty);
});
}
private Exception? GetPossibleException() => new Exception();
");

await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
DereferencePossiblyNullReferenceSuppressor.SuppressionDescriptors["CS8602"], testCode)
.ConfigureAwait(false);
}

[Test]
public async Task VariableAssignedOutsideAssertMultipleUsedInside()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
#nullable enable
[Test]
public void Test()
{
var e = GetPossibleException();
Assert.Multiple(() =>
{
Assert.That(e.Message, Is.EqualTo(""Test""));
Assert.That(e.InnerException, Is.Null);
Assert.That(e.StackTrace, Is.Not.Empty);
});
}
private Exception? GetPossibleException() => new Exception();
");

await DiagnosticsSuppressorAnalyzer.EnsureNotSuppressed(suppressor, testCode)
.ConfigureAwait(false);
}

[Test]
public async Task VariableAssignedUsedInsideLambda()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
#nullable enable
[Test]
public void Test()
{
var e = GetPossibleException();
Assert.That(() => e.Message, Is.EqualTo(""Test""));
}
private Exception? GetPossibleException() => new Exception();
");

await DiagnosticsSuppressorAnalyzer.EnsureNotSuppressed(suppressor, testCode)
.ConfigureAwait(false);
}

[Test]
public async Task VariableAssertedUsedInsideLambda()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
#nullable enable
[Test]
public void Test()
{
var e = GetPossibleException();
Assert.That(e, Is.Not.Null);
Assert.That(() => e.Message, Is.EqualTo(""Test""));
}
private Exception? GetPossibleException() => new Exception();
");

await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
DereferencePossiblyNullReferenceSuppressor.SuppressionDescriptors["CS8602"], testCode)
.ConfigureAwait(false);
}

[Test]
public async Task NestedStatements()
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
#nullable enable
[Test]
public void Test()
{
Assert.Multiple(() =>
{
Assert.DoesNotThrow(() =>
{
var e = Assert.Throws<Exception>(() => new Exception(""Test""));
if (e.InnerException != null)
{
Assert.That(e.InnerException.Message, Is.EqualTo(""Test""));
}
else
{
Assert.That(e.Message, Is.EqualTo(""Test""));
}
});
});
}");

await DiagnosticsSuppressorAnalyzer.EnsureNotSuppressed(suppressor, testCode)
.ConfigureAwait(false);
}
}
}
2 changes: 0 additions & 2 deletions src/nunit.analyzers.tests/DiagnosticSuppressors/DiagnosticsSuppressorAnalyzer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
Expand Down
74 changes: 57 additions & 17 deletions src/nunit.analyzers/DiagnosticSuppressors/DereferencePossiblyNullReferenceSuppressor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,37 +39,48 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
{
SyntaxNode? node = diagnostic.Location.SourceTree?.GetRoot(context.CancellationToken)
.FindNode(diagnostic.Location.SourceSpan);
BlockSyntax? parent = node?.Ancestors().OfType<BlockSyntax>().FirstOrDefault();

if (node is null || parent is null)
if (node is null)
{
continue;
}

if (IsInsideAssertMultiple(parent))
{
// NUnit doesn't throw on failures and therefore the compiler is correct.
continue;
}

if (ShouldBeSuppressed(node, parent))
// Was the offending variable assigned or verified to be not null inside an Assert.Multiple?
// NUnit doesn't throw on failures and therefore the compiler is correct.
if (ShouldBeSuppressed(node, out SyntaxNode? suppressionCause) && !IsInsideAssertMultiple(suppressionCause))
{
context.ReportSuppression(Suppression.Create(SuppressionDescriptors[diagnostic.Id], diagnostic));
}
}
}

private static bool IsInsideAssertMultiple(SyntaxNode parent)
private static bool IsInsideAssertMultiple(SyntaxNode node)
{
var possibleAssertMultiple = parent.AncestorsAndSelf().OfType<InvocationExpressionSyntax>().FirstOrDefault();
return IsAssert("Multiple", possibleAssertMultiple);
InvocationExpressionSyntax? possibleAssertMultiple;

while ((possibleAssertMultiple = node.Ancestors().OfType<InvocationExpressionSyntax>().FirstOrDefault()) != null)
{
// Is the statement inside a Block which is part of an Assert.Multiple.
if (IsAssert(possibleAssertMultiple, "Multiple"))
{
return true;
}

// Keep looking at possible parent nested expression.
node = possibleAssertMultiple;
}

return false;
}

private static bool ShouldBeSuppressed(SyntaxNode node, BlockSyntax parent)
private static bool ShouldBeSuppressed(SyntaxNode node, [NotNullWhen(true)] out SyntaxNode? suppressionCause)
{
suppressionCause = default(SyntaxNode);

if (IsKnownToBeNotNull(node))
{
// Known to be not null value assigned or passed to non-nullable type.
suppressionCause = node;
return true;
}

Expand All @@ -80,15 +91,44 @@ private static bool ShouldBeSuppressed(SyntaxNode node, BlockSyntax parent)
possibleNullReference = castExpression.Expression.ToString();
}

StatementSyntax? statement = node?.AncestorsAndSelf().OfType<StatementSyntax>().FirstOrDefault();
bool validatedNotNull;

do
{
BlockSyntax? parent = node.Ancestors().OfType<BlockSyntax>().FirstOrDefault();

if (parent is null)
{
return false;
}

validatedNotNull = IsValidatedNotNull(possibleNullReference, parent, node, out suppressionCause);
if (parent.Parent is null)
{
return false;
}

node = parent.Parent;
}
while (!validatedNotNull);

return validatedNotNull;
}

private static bool IsValidatedNotNull(string possibleNullReference, BlockSyntax parent, SyntaxNode node,
[NotNullWhen(true)] out SyntaxNode? suppressionCause)
{
suppressionCause = default(SyntaxNode);

StatementSyntax? statement = node?.AncestorsAndSelf().OfType<StatementSyntax>().FirstOrDefault();
var siblings = parent.ChildNodes().ToList();

// Look in earlier statements to see if the variable was previously checked for null.
for (int nodeIndex = siblings.FindIndex(x => x == statement); --nodeIndex >= 0;)
{
SyntaxNode previous = siblings[nodeIndex];

suppressionCause = previous;
if (previous is ExpressionStatementSyntax expressionStatement)
{
if (expressionStatement.Expression is AssignmentExpressionSyntax assignmentExpression)
Expand Down Expand Up @@ -149,12 +189,12 @@ private static bool IsKnownToBeNotNull(SyntaxNode? node)
private static bool IsKnownToBeNotNull(ExpressionSyntax? expression)
{
// For now, we only know that Assert.Throws either returns not-null or throws
return IsAssert("Throws", expression);
return IsAssert(expression, "Throws", "Catch", "ThrowsAsync", "CatchAsync");
}

private static bool IsAssert(string requestedMember, ExpressionSyntax? expression)
private static bool IsAssert(ExpressionSyntax? expression, params string[] requestedMembers)
{
return IsAssert(expression, out string member, out _) && member == requestedMember;
return IsAssert(expression, out string member, out _) && requestedMembers.Contains(member);
}

private static bool IsAssert(ExpressionSyntax? expression,
Expand Down

0 comments on commit 268b0cb

Please sign in to comment.