From 5c9436c0f9a3f878f742df804152485f285de9f6 Mon Sep 17 00:00:00 2001 From: Joris Voermans Date: Thu, 18 Apr 2024 10:09:00 +0200 Subject: [PATCH] Add USP0022, UnityObjectIfNullCoalescingSuppressor --- doc/USP0022.md | 32 +++++++++++++++++++ doc/index.md | 1 + .../UnityObjectNullHandlingSuppressorTests.cs | 25 +++++++++++++++ .../Resources/Strings.Designer.cs | 9 ++++++ .../Resources/Strings.resx | 5 ++- .../UnityObjectNullHandling.cs | 7 +++- 6 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 doc/USP0022.md diff --git a/doc/USP0022.md b/doc/USP0022.md new file mode 100644 index 00000000..35d08cd2 --- /dev/null +++ b/doc/USP0022.md @@ -0,0 +1,32 @@ +# USP0022 Prefer reference equality + +We have a dedicated diagnostic `UNT0029` to prevent is-null check with `UnityEngine.Object`. But `IDE0270` will suggest to use is-null check over reference equality method. + +## Suppressed Diagnostic ID + +IDE0270 - Null check can be simplified (if null check) + +## Examples of code that produces a suppressed diagnostic +```csharp +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void M() + { + Camera item = FindItem() as Camera; + if (item == null) + throw new System.InvalidOperationException(); + } + + MonoBehaviour FindItem() => null; +} +``` + +## Why is the diagnostic reported? + +Under normal circumstances, `if (item == null) throw new System.InvalidOperationException()` can be simplified to `Camera item = FindItem() as Camera ?? throw new System.InvalidOperationException();`. + +## Why do we suppress this diagnostic? + +Unity has overridden the `==` operator for `UnityEngine.Object`. If you use the `==` operator to compare a `UnityEngine.Object` to null, it will return true if the `UnityEngine.Object` is destroyed, even if the object itself isn't actually null. The `??` operator cannot be overridden in this way, and therefore behaves inconsistently with the `==` operator, because it checks for null in a different way. \ No newline at end of file diff --git a/doc/index.md b/doc/index.md index 50306a6f..1e81a830 100644 --- a/doc/index.md +++ b/doc/index.md @@ -65,3 +65,4 @@ ID | Suppressed ID | Justification [USP0019](USP0012.md) | IDE0051 | Don't flag private methods with PreserveAttribute or UsedImplicitlyAttribute as unused [USP0020](USP0020.md) | IDE0052 | The Unity runtime invokes Unity messages [USP0021](USP0021.md) | IDE0041 | Prefer reference equality +[USP0022](USP0022.md) | IDE0270 | Unity objects should not use if null coalescing diff --git a/src/Microsoft.Unity.Analyzers.Tests/UnityObjectNullHandlingSuppressorTests.cs b/src/Microsoft.Unity.Analyzers.Tests/UnityObjectNullHandlingSuppressorTests.cs index cd8d0dc2..81319bd1 100644 --- a/src/Microsoft.Unity.Analyzers.Tests/UnityObjectNullHandlingSuppressorTests.cs +++ b/src/Microsoft.Unity.Analyzers.Tests/UnityObjectNullHandlingSuppressorTests.cs @@ -219,6 +219,7 @@ public void Update() await VerifyCSharpDiagnosticAsync(test, suppressor); } + [Fact] public async Task UseIsNullFromObjectSuppressed() { @@ -241,6 +242,30 @@ public void Update() await VerifyCSharpDiagnosticAsync(test, suppressor); } + [Fact] + public async Task IfNullCoalescingSuppressed() + { + const string test = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void M() + { + Camera item = FindItem() as Camera; + if (item == null) + throw new System.InvalidOperationException(); + } + + MonoBehaviour FindItem() => null; +}"; + + var suppressor = ExpectSuppressor(UnityObjectNullHandlingSuppressor.IfNullCoalescingRule) + .WithLocation(9, 3); + + await VerifyCSharpDiagnosticAsync(test, suppressor); + } + [Fact] public async Task LegitUseIsNullNotSuppressed() { diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs index 5a03326e..ee2d3b32 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs @@ -1149,6 +1149,15 @@ internal static string UnityObjectCoalescingAssignmentSuppressorJustification { } } + /// + /// Looks up a localized string similar to Unity objects should not use if null coalescing.. + /// + internal static string UnityObjectIfNullCoalescingSuppressorJustification { + get { + return ResourceManager.GetString("UnityObjectIfNullCoalescingSuppressorJustification", resourceCulture); + } + } + /// /// Looks up a localized string similar to Use null comparison. /// diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx index 88445d4b..bdcee202 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx @@ -232,6 +232,9 @@ Unity objects should not use null coalescing. + + Unity objects should not use if null coalescing. + Unity objects should not use null propagation. @@ -611,4 +614,4 @@ Inefficient localPosition/localRotation read - \ No newline at end of file + diff --git a/src/Microsoft.Unity.Analyzers/UnityObjectNullHandling.cs b/src/Microsoft.Unity.Analyzers/UnityObjectNullHandling.cs index bdf66386..94a7becf 100644 --- a/src/Microsoft.Unity.Analyzers/UnityObjectNullHandling.cs +++ b/src/Microsoft.Unity.Analyzers/UnityObjectNullHandling.cs @@ -289,7 +289,12 @@ public class UnityObjectNullHandlingSuppressor : DiagnosticSuppressor suppressedDiagnosticId: "IDE0074", justification: Strings.UnityObjectCoalescingAssignmentSuppressorJustification); - public override ImmutableArray SupportedSuppressions => ImmutableArray.Create(NullCoalescingRule, NullPropagationRule, CoalescingAssignmentRule, UseIsNullRule); + internal static readonly SuppressionDescriptor IfNullCoalescingRule = new( + id: "USP0022", + suppressedDiagnosticId: "IDE0270", + justification: Strings.UnityObjectIfNullCoalescingSuppressorJustification); + + public override ImmutableArray SupportedSuppressions => ImmutableArray.Create(NullCoalescingRule, NullPropagationRule, CoalescingAssignmentRule, UseIsNullRule, IfNullCoalescingRule); public override void ReportSuppressions(SuppressionAnalysisContext context) {