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

Prefer null conditional over ternaries #75

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
61 changes: 61 additions & 0 deletions src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;

namespace Faithlife.Analyzers
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class NullTestTernariesAnalyzer : DiagnosticAnalyzer
{
public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);

context.RegisterCompilationStartAction(compilationStartAnalysisContext =>
{
compilationStartAnalysisContext.RegisterSyntaxNodeAction(c => AnalyzeSyntax(c), SyntaxKind.ConditionalExpression);
jeffcorcoran marked this conversation as resolved.
Show resolved Hide resolved
});
}

private static void AnalyzeSyntax(SyntaxNodeAnalysisContext context)
{
var expression = (ConditionalExpressionSyntax) context.Node;
var falseValue = expression.WhenFalse;
var trueValue = expression.WhenTrue;

if (expression.Condition is BinaryExpressionSyntax binaryExpression)
{
var lExpression = binaryExpression.Left;
var rExpression = binaryExpression.Right;

if ((falseValue.Kind() == SyntaxKind.NullLiteralExpression || trueValue.Kind() == SyntaxKind.NullLiteralExpression) && (lExpression.Kind() == SyntaxKind.NullLiteralExpression || rExpression.Kind() == SyntaxKind.NullLiteralExpression))
{
context.ReportDiagnostic(Diagnostic.Create(s_rule, expression.GetLocation()));
}
}
else if (expression.Condition is MemberAccessExpressionSyntax memberAccessExpression)
{
if (memberAccessExpression.Kind() == SyntaxKind.SimpleMemberAccessExpression && memberAccessExpression.Name.Identifier.Text == "HasValue")
{
context.ReportDiagnostic(Diagnostic.Create(s_rule, expression.GetLocation()));
}
}
}

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(s_rule);

public const string DiagnosticId = "FL0015";

private static readonly DiagnosticDescriptor s_rule = new(
id: DiagnosticId,
title: "Null Checking Ternaries Usage",
messageFormat: "Prefer null conditional operators over ternaries explicitly checking for null",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
messageFormat: "Prefer null conditional operators over ternaries explicitly checking for null",
messageFormat: "Use null propagation",

The message from IDE0031 is simpler and appropriate.

category: "Usage",
defaultSeverity: DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: $"https://github.com/Faithlife/FaithlifeAnalyzers/wiki/{DiagnosticId}"
);
}
}
77 changes: 77 additions & 0 deletions tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using NUnit.Framework;

namespace Faithlife.Analyzers.Tests
{
[TestFixture]
public class NullTestTernariesTests : CodeFixVerifier
{
[Test]
public void ValidUsage()
jeffcorcoran marked this conversation as resolved.
Show resolved Hide resolved
{
const string validProgram = c_preamble + @"
namespace TestApplication
{
public class Person
{
public string FirstName { get; set; }
public string LastName { get; set; }
}

internal static class TestClass
{
public static void UtilityMethod()
{
Person person = null;
var firstName = person?.FirstName;
}
}
}";

VerifyCSharpDiagnostic(validProgram);
}

[TestCase("var firstName = person.Age != null ? person.FirstName : null;")]
[TestCase("var firstName = person.Age == null ? null : person.FirstName;")]
[TestCase("var firstName = person.Age.HasValue ? person.FirstName : null;")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a test for the case where the ternary checks !person.Age.HasValue?

Copy link
Member

Choose a reason for hiding this comment

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

Are these examples of code that would trigger the diagnostic and should be changed? What's the desired replacement? (I don't see how the null-coalescing operator would be used here; what am I missing?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added better tests here. These were certainly sloppy, tried to get them in quick before I ran out of time at the end of the day, no real excuse.

var firstName = person.Age?.BirthYear; <-- This should better catch all the HasValue cases.

I have not removed the == null and != null because I still see some value in having them (ease of deployment), but I'm happy to rip those checks out for sure, as they can lead to issues down the road in terms of maintenance or even bugs.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, you can set a PR to Draft status if you know it's not fully-baked.

public void InvalidUsage(string badExample)
{
var brokenProgram = c_preamble + $@"
namespace TestApplication
{{
public class Person
{{
public string FirstName {{ get; set; }}
public string LastName {{ get; set; }}
public int? Age {{ get; set; }}
}}

internal static class TestClass
{{
public static void UtilityMethod()
{{
var person = new Person {{ FirstName = ""Bob"", LastName = ""Dole"" }};
{badExample}
}}
}}
}}";

var expected = new DiagnosticResult
{
Id = NullTestTernariesAnalyzer.DiagnosticId,
Message = "Prefer null conditional operators over ternaries explicitly checking for null",
Severity = DiagnosticSeverity.Warning,
Locations = new[] { new DiagnosticResultLocation("Test0.cs", s_preambleLength + 15, 20) },
};

VerifyCSharpDiagnostic(brokenProgram, expected);
}

protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() => new NullTestTernariesAnalyzer();

private const string c_preamble = @"";
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that tis preamble is empty, you should probably delete it and all its uses. At the very least, we no longer allow unnecessary verbatim strings ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned this up.


private static readonly int s_preambleLength = c_preamble.Split('\n').Length;
}
}