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.

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

Jump to bottom

Issue 385: Detect indirect assignment of non-nullable fields. #386

Merged
merged 1 commit into from
Aug 28, 2021
Merged

Issue 385: Detect indirect assignment of non-nullable fields. #386

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
2 changes: 2 additions & 0 deletions documentation/NUnit3002.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ This rule check diagnostics reported by the CS8618 compiler error:
If the violating field/property is set in the `SetUp` or `OneTimeSetUp` method. The rule suppresses the error.
This allows for non-nullable fields/properties to be used in a `TestFixture`.

The rule does detect indirect calls, when the field is set in a method called by the `SetUp` or `OneTimeSetUp` methods.

## Motivation

```csharp
Expand Down
124 changes: 124 additions & 0 deletions ...s.tests/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,5 +155,129 @@ await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
NonNullableFieldOrPropertyIsUninitializedSuppressor.NullableFieldOrPropertyInitializedInSetUp, testCode)
.ConfigureAwait(false);
}

[TestCase("SetUp", "")]
[TestCase("OneTimeSetUp", "this.")]
public async Task FieldAssignedInCalledMethod(string attribute, string prefix)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
#nullable enable

private string field;

[{attribute}]
public void Initialize()
{{
{prefix}SetFields();
}}

[Test]
public void Test()
{{
Assert.That({prefix}field, Is.Not.Null);
}}

private void SetFields()
{{
{prefix}field = string.Empty;
}}
");

await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
NonNullableFieldOrPropertyIsUninitializedSuppressor.NullableFieldOrPropertyInitializedInSetUp, testCode)
.ConfigureAwait(false);
}

[TestCase("SetUp", "")]
[TestCase("OneTimeSetUp", "this.")]
public async Task FieldAssignedInCalledMethods(string attribute, string prefix)
{
var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@$"
#nullable enable

private double numericField;
private string textField;

[{attribute}]
public void Initialize()
{{
{prefix}SetFields();
}}

[Test]
public void Test()
{{
Assert.That({prefix}numericField, Is.Not.Zero);
Assert.That({prefix}textField, Is.Not.Null);
}}

public void SetFields()
{{
{prefix}SetFields(3, 4);
{prefix}SetFields(""Seven"");
}}

private void SetFields(params double[] numbers)
{{
double sum = 0;
for (int i = 0; i < numbers.Length; i++)
sum += numbers[i];
{prefix}numericField = sum;
}}

private void SetFields(string? text)
{{
{prefix}textField = text ?? string.Empty;
}}
");

await DiagnosticsSuppressorAnalyzer.EnsureSuppressed(suppressor,
NonNullableFieldOrPropertyIsUninitializedSuppressor.NullableFieldOrPropertyInitializedInSetUp, testCode)
.ConfigureAwait(false);
}

[TestCase("SetUp", "")]
[TestCase("OneTimeSetUp", "this.")]
public async Task FieldIndirectAssignedThroughBaseClassSetupMethod(string attribute, string prefix)
{
var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@$"
#nullable enable

public class BaseClass
{{
[{attribute}]
public void Initialize()
{{
SetFields();
}}

protected virtual void SetFields(string text = ""Default"")
{{
}}
}}

[TestFixture]
public class TestClass : BaseClass
{{
private string field;

[Test]
public void Test()
{{
Assert.That({prefix}field, Is.EqualTo(""Default""));
}}

protected override void SetFields(string text)
{{
{prefix}field = text;
}}
}}
");

// This is not supported as the BaseClass could be in an external library.
// We therefore cannot validate that its SetUp method calls into our overridden method.
await DiagnosticsSuppressorAnalyzer.EnsureNotSuppressed(suppressor, testCode)
.ConfigureAwait(false);
}
}
}
87 changes: 70 additions & 17 deletions ...it.analyzers/DiagnosticSuppressors/NonNullableFieldOrPropertyIsUninitializedSuppressor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using NUnit.Analyzers.Constants;

using BindingFlags = System.Reflection.BindingFlags;

namespace NUnit.Analyzers.DiagnosticSuppressors
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
Expand All @@ -26,17 +27,15 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
{
foreach (var diagnostic in context.ReportedDiagnostics)
{
SyntaxNode? node = diagnostic.Location.SourceTree?.GetRoot(context.CancellationToken)
.FindNode(diagnostic.Location.SourceSpan);
SyntaxTree? sourceTree = diagnostic.Location.SourceTree;

if (node is null)
if (sourceTree is null)
{
continue;
}

var classDeclaration = node.Ancestors().OfType<ClassDeclarationSyntax>().First();
var fieldDeclarations = classDeclaration.Members.OfType<FieldDeclarationSyntax>().ToArray();
var propertyDeclarations = classDeclaration.Members.OfType<PropertyDeclarationSyntax>().ToArray();
SyntaxNode node = sourceTree.GetRoot(context.CancellationToken)
.FindNode(diagnostic.Location.SourceSpan);

string fieldOrPropertyName;

Expand Down Expand Up @@ -72,22 +71,27 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
}

// Verify that the name found is actually a field or a property name.
var classDeclaration = node.Ancestors().OfType<ClassDeclarationSyntax>().First();
var fieldDeclarations = classDeclaration.Members.OfType<FieldDeclarationSyntax>();
var propertyDeclarations = classDeclaration.Members.OfType<PropertyDeclarationSyntax>();

if (!fieldDeclarations.SelectMany(x => x.Declaration.Variables).Any(x => x.Identifier.Text == fieldOrPropertyName) &&
!propertyDeclarations.Any(x => x.Identifier.Text == fieldOrPropertyName))
{
continue;
}

var methods = classDeclaration.Members.OfType<MethodDeclarationSyntax>().ToArray();
SemanticModel model = context.GetSemanticModel(sourceTree);

var methods = classDeclaration.Members.OfType<MethodDeclarationSyntax>();
foreach (var method in methods)
{
var allAttributes = method.AttributeLists.SelectMany(list => list.Attributes.Select(a => a.Name.ToString()))
.ToImmutableHashSet();
if (allAttributes.Contains("SetUp") || allAttributes.Contains("OneTimeSetUp"))
var isSetup = method.AttributeLists.SelectMany(list => list.Attributes.Select(a => a.Name.ToString()))
.Any(name => name == "SetUp" || name == "OneTimeSetUp");
if (isSetup)
{
// Find (OneTime)SetUps method and check for assignment to this field.
if (IsAssignedIn(method, fieldOrPropertyName))
if (IsAssignedIn(model, classDeclaration, method, fieldOrPropertyName))
{
context.ReportSuppression(Suppression.Create(NullableFieldOrPropertyInitializedInSetUp, diagnostic));
}
Expand All @@ -96,11 +100,15 @@ public override void ReportSuppressions(SuppressionAnalysisContext context)
}
}

private static bool IsAssignedIn(MethodDeclarationSyntax method, string fieldOrPropertyName)
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
MethodDeclarationSyntax method,
string fieldOrPropertyName)
{
if (method.ExpressionBody != null)
{
return IsAssignedIn(method.ExpressionBody.Expression, fieldOrPropertyName);
return IsAssignedIn(model, classDeclaration, method.ExpressionBody.Expression, fieldOrPropertyName);
}

if (method.Body is null)
Expand All @@ -111,7 +119,7 @@ private static bool IsAssignedIn(MethodDeclarationSyntax method, string fieldOrP
foreach (var statement in method.Body.Statements)
{
if (statement is ExpressionStatementSyntax expressionStatement &&
IsAssignedIn(expressionStatement.Expression, fieldOrPropertyName))
IsAssignedIn(model, classDeclaration, expressionStatement.Expression, fieldOrPropertyName))
{
return true;
}
Expand All @@ -120,7 +128,32 @@ private static bool IsAssignedIn(MethodDeclarationSyntax method, string fieldOrP
return false;
}

private static bool IsAssignedIn(ExpressionSyntax? expressionStatement, string fieldOrPropertyName)
private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
InvocationExpressionSyntax invocationExpression,
string fieldOrPropertyName)
{
// Check semantic model for actual called method match found by compiler.
IMethodSymbol? calledMethod = model.GetSymbolInfo(invocationExpression).Symbol as IMethodSymbol;

// Find the corresponding declaration
MethodDeclarationSyntax? method = calledMethod?.DeclaringSyntaxReferences.FirstOrDefault()?
.GetSyntax() as MethodDeclarationSyntax;
if (method?.Parent == classDeclaration)
{
// We only get here if the method is in our source code and our class.
return IsAssignedIn(model, classDeclaration, method, fieldOrPropertyName);
}

return false;
}

private static bool IsAssignedIn(
SemanticModel model,
ClassDeclarationSyntax classDeclaration,
ExpressionSyntax? expressionStatement,
string fieldOrPropertyName)
{
if (expressionStatement is AssignmentExpressionSyntax assignmentExpression)
{
Expand All @@ -130,7 +163,27 @@ private static bool IsAssignedIn(ExpressionSyntax? expressionStatement, string f
}
else if (assignmentExpression.Left is MemberAccessExpressionSyntax memberAccessExpression &&
memberAccessExpression.Expression is ThisExpressionSyntax &&
memberAccessExpression.Name.ToString() == fieldOrPropertyName)
memberAccessExpression.Name.Identifier.Text == fieldOrPropertyName)
{
return true;
}
}
else if (expressionStatement is InvocationExpressionSyntax invocationExpression)
{
string? identifier = null;

if (invocationExpression.Expression is MemberAccessExpressionSyntax memberAccessExpression &&
memberAccessExpression.Expression is ThisExpressionSyntax)
{
identifier = memberAccessExpression.Name.Identifier.Text;
}
else if (invocationExpression.Expression is IdentifierNameSyntax identifierName)
{
identifier = identifierName.Identifier.Text;
}

if (!string.IsNullOrEmpty(identifier) &&
IsAssignedIn(model, classDeclaration, invocationExpression, fieldOrPropertyName))
{
return true;
}
Expand Down