From dc701c95cfe2b4480ed29e2abe1e416f36e938c2 Mon Sep 17 00:00:00 2001 From: Jeff Corcoran Date: Fri, 12 Nov 2021 08:26:13 -0500 Subject: [PATCH 1/5] Initial checkin for Ternary null check analysis --- .../NullTestTernariesAnalyzer.cs | 53 +++++++++++++ .../NullTestTernariesTests.cs | 75 +++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs create mode 100644 tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs diff --git a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs new file mode 100644 index 0000000..b585d81 --- /dev/null +++ b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs @@ -0,0 +1,53 @@ +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); + }); + } + + private static void AnalyzeSyntax(SyntaxNodeAnalysisContext context) + { + var expression = (ConditionalExpressionSyntax) context.Node; + var falseValue = expression.WhenFalse; + var trueValue = expression.WhenTrue; + + var eExpression = (BinaryExpressionSyntax) expression.Condition; + + var lExpression = eExpression.Left; + var rExpression = eExpression.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())); + } + } + + public override ImmutableArray 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", + category: "Usage", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: $"https://github.com/Faithlife/FaithlifeAnalyzers/wiki/{DiagnosticId}" + ); + } +} diff --git a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs new file mode 100644 index 0000000..e43ce8c --- /dev/null +++ b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs @@ -0,0 +1,75 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; + +namespace Faithlife.Analyzers.Tests +{ + [TestFixture] + public class NullTestTernariesTests : CodeFixVerifier + { + [Test] + public void ValidUsage() + { + 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 != null ? person.FirstName : null;")] + [TestCase("var firstName = person == null ? null : person.FirstName;")] + public void InvalidUsage(string badExample) + { + var brokenProgram = 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; + {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 = @""; + + private static readonly int s_preambleLength = c_preamble.Split('\n').Length; + } +} From 01f90e51fa7e0dcca876f28427d7edac44ad1e01 Mon Sep 17 00:00:00 2001 From: Jeff Corcoran Date: Fri, 12 Nov 2021 14:09:11 -0500 Subject: [PATCH 2/5] Final checks added for HasValue --- .../NullTestTernariesAnalyzer.cs | 20 +++++++++++++------ .../NullTestTernariesTests.cs | 12 ++++++----- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs index b585d81..1f47e94 100644 --- a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs +++ b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs @@ -25,14 +25,22 @@ private static void AnalyzeSyntax(SyntaxNodeAnalysisContext context) var falseValue = expression.WhenFalse; var trueValue = expression.WhenTrue; - var eExpression = (BinaryExpressionSyntax) expression.Condition; - - var lExpression = eExpression.Left; - var rExpression = eExpression.Right; + 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)) + 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) { - context.ReportDiagnostic(Diagnostic.Create(s_rule, expression.GetLocation())); + if (memberAccessExpression.Kind() == SyntaxKind.SimpleMemberAccessExpression && memberAccessExpression.Name.Identifier.Text == "HasValue") + { + context.ReportDiagnostic(Diagnostic.Create(s_rule, expression.GetLocation())); + } } } diff --git a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs index e43ce8c..6b324f3 100644 --- a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs +++ b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs @@ -32,8 +32,9 @@ public static void UtilityMethod() VerifyCSharpDiagnostic(validProgram); } - [TestCase("var firstName = person != null ? person.FirstName : null;")] - [TestCase("var firstName = person == null ? null : person.FirstName;")] + [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;")] public void InvalidUsage(string badExample) { var brokenProgram = c_preamble + $@" @@ -41,15 +42,16 @@ namespace TestApplication {{ public class Person {{ - public string? FirstName {{ get; set; }} - public string? LastName {{ get; set; }} + public string FirstName {{ get; set; }} + public string LastName {{ get; set; }} + public int? Age {{ get; set; }} }} internal static class TestClass {{ public static void UtilityMethod() {{ - Person person = null; + var person = new Person {{ FirstName = ""Bob"", LastName = ""Dole"" }}; {badExample} }} }} From cc54c9e1b6f5e44accf686809da5798295aa89fe Mon Sep 17 00:00:00 2001 From: jeffcorcoran Date: Mon, 15 Nov 2021 08:56:12 -0500 Subject: [PATCH 3/5] Update src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs Co-authored-by: bagley2014 --- src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs index 1f47e94..cab4fe3 100644 --- a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs +++ b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs @@ -15,7 +15,7 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationStartAction(compilationStartAnalysisContext => { - compilationStartAnalysisContext.RegisterSyntaxNodeAction(c => AnalyzeSyntax(c), SyntaxKind.ConditionalExpression); + compilationStartAnalysisContext.RegisterSyntaxNodeAction(AnalyzeSyntax, SyntaxKind.ConditionalExpression); }); } From 7293055ab84269f3cbda6d1e4cd6ddc34737acf5 Mon Sep 17 00:00:00 2001 From: Jeff Corcoran Date: Mon, 15 Nov 2021 09:54:04 -0500 Subject: [PATCH 4/5] Multiple updates for code cleanup. Added viable / reasonable tests --- .../NullTestTernariesAnalyzer.cs | 9 ++- .../NullTestTernariesTests.cs | 69 +++++++++++++------ 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs index 1f47e94..de1de40 100644 --- a/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs +++ b/src/Faithlife.Analyzers/NullTestTernariesAnalyzer.cs @@ -15,7 +15,7 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationStartAction(compilationStartAnalysisContext => { - compilationStartAnalysisContext.RegisterSyntaxNodeAction(c => AnalyzeSyntax(c), SyntaxKind.ConditionalExpression); + compilationStartAnalysisContext.RegisterSyntaxNodeAction(AnalyzeSyntax, SyntaxKind.ConditionalExpression); }); } @@ -42,6 +42,13 @@ private static void AnalyzeSyntax(SyntaxNodeAnalysisContext context) context.ReportDiagnostic(Diagnostic.Create(s_rule, expression.GetLocation())); } } + else if (expression.Condition is PrefixUnaryExpressionSyntax prefixUnaryExpression && expression.Condition.Kind() == SyntaxKind.LogicalNotExpression) + { + if (((MemberAccessExpressionSyntax) prefixUnaryExpression.Operand).Name.Identifier.Text == "HasValue") + { + context.ReportDiagnostic(Diagnostic.Create(s_rule, expression.GetLocation())); + } + } } public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(s_rule); diff --git a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs index 6b324f3..7d4c6c1 100644 --- a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs +++ b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs @@ -7,34 +7,50 @@ namespace Faithlife.Analyzers.Tests [TestFixture] public class NullTestTernariesTests : CodeFixVerifier { - [Test] - public void ValidUsage() + [TestCase("Person person = null;", "var firstName = person?.FirstName;")] + [TestCase("var person = new Person { FirstName = \"Bob\", LastName = \"Dole\" };", "var firstName = person.Age?.BirthYear;")] + public void ValidUsage(string declaration, string ternary) { - const string validProgram = c_preamble + @" + string validProgram = c_preamble + $@" namespace TestApplication -{ +{{ public class Person - { - public string FirstName { get; set; } - public string LastName { get; set; } - } + {{ + public string FirstName {{ get; set; }} + public string LastName {{ get; set; }} + public Age? Age {{ get; set; }} + }} + + public struct Age + {{ + public int? InYears {{ get; set; }} + + public int? BirthYear + {{ + get + {{ + return System.DateTime.Now.Year - InYears; + }} + }} + }} internal static class TestClass - { + {{ public static void UtilityMethod() - { - Person person = null; - var firstName = person?.FirstName; - } - } -}"; + {{ + {declaration} + {ternary} + }} + }} +}}"; 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;")] + [TestCase("var firstName = person != null ? person.FirstName : null;")] + [TestCase("var firstName = person == null ? null : person.FirstName;")] + [TestCase("var firstName = person.Age.HasValue ? person.Age.Value.BirthYear : null;")] + [TestCase("var firstName = !person.Age.HasValue ? null : person.Age.Value.BirthYear;")] public void InvalidUsage(string badExample) { var brokenProgram = c_preamble + $@" @@ -44,7 +60,20 @@ public class Person {{ public string FirstName {{ get; set; }} public string LastName {{ get; set; }} - public int? Age {{ get; set; }} + public Age? Age {{ get; set; }} + }} + + public struct Age + {{ + public int? InYears {{ get; set; }} + + public int? BirthYear + {{ + get + {{ + return System.DateTime.Now.Year - InYears; + }} + }} }} internal static class TestClass @@ -62,7 +91,7 @@ public static void UtilityMethod() 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) }, + Locations = new[] { new DiagnosticResultLocation("Test0.cs", s_preambleLength + 28, 20) }, }; VerifyCSharpDiagnostic(brokenProgram, expected); From 247751a7244318b2b610d7fc9ec8bc904d2e7546 Mon Sep 17 00:00:00 2001 From: Jeff Corcoran Date: Mon, 15 Nov 2021 10:00:11 -0500 Subject: [PATCH 5/5] Cleaned up tests by removing unneeded preamble --- .../NullTestTernariesTests.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs index 7d4c6c1..875f8c2 100644 --- a/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs +++ b/tests/Faithlife.Analyzers.Tests/NullTestTernariesTests.cs @@ -11,7 +11,7 @@ public class NullTestTernariesTests : CodeFixVerifier [TestCase("var person = new Person { FirstName = \"Bob\", LastName = \"Dole\" };", "var firstName = person.Age?.BirthYear;")] public void ValidUsage(string declaration, string ternary) { - string validProgram = c_preamble + $@" + string validProgram = $@" namespace TestApplication {{ public class Person @@ -53,7 +53,7 @@ public static void UtilityMethod() [TestCase("var firstName = !person.Age.HasValue ? null : person.Age.Value.BirthYear;")] public void InvalidUsage(string badExample) { - var brokenProgram = c_preamble + $@" + var brokenProgram = $@" namespace TestApplication {{ public class Person @@ -91,16 +91,12 @@ public static void UtilityMethod() 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 + 28, 20) }, + Locations = new[] { new DiagnosticResultLocation("Test0.cs", 29, 20) }, }; VerifyCSharpDiagnostic(brokenProgram, expected); } protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() => new NullTestTernariesAnalyzer(); - - private const string c_preamble = @""; - - private static readonly int s_preambleLength = c_preamble.Split('\n').Length; } }