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

Add USP0022, UnityObjectIfNullCoalescingSuppressor #324

Merged
Show file tree
Hide file tree
Changes from all 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
32 changes: 32 additions & 0 deletions doc/USP0022.md
Original file line number Diff line number Diff line change
@@ -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.
1 change: 1 addition & 0 deletions doc/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ public void Update()
await VerifyCSharpDiagnosticAsync(test, suppressor);
}


[Fact]
public async Task UseIsNullFromObjectSuppressed()
{
Expand All @@ -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()
{
Expand Down
9 changes: 9 additions & 0 deletions src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/Microsoft.Unity.Analyzers/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@
<data name="UnityObjectNullCoalescingSuppressorJustification" xml:space="preserve">
<value>Unity objects should not use null coalescing.</value>
</data>
<data name="UnityObjectIfNullCoalescingSuppressorJustification" xml:space="preserve">
<value>Unity objects should not use if null coalescing.</value>
</data>
<data name="UnityObjectNullPropagationSuppressorJustification" xml:space="preserve">
<value>Unity objects should not use null propagation.</value>
</data>
Expand Down Expand Up @@ -611,4 +614,4 @@
<data name="GetLocalPositionAndRotationDiagnosticTitle" xml:space="preserve">
<value>Inefficient localPosition/localRotation read</value>
</data>
</root>
</root>
7 changes: 6 additions & 1 deletion src/Microsoft.Unity.Analyzers/UnityObjectNullHandling.cs
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,12 @@ public class UnityObjectNullHandlingSuppressor : DiagnosticSuppressor
suppressedDiagnosticId: "IDE0074",
justification: Strings.UnityObjectCoalescingAssignmentSuppressorJustification);

public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions => ImmutableArray.Create(NullCoalescingRule, NullPropagationRule, CoalescingAssignmentRule, UseIsNullRule);
internal static readonly SuppressionDescriptor IfNullCoalescingRule = new(
id: "USP0022",
suppressedDiagnosticId: "IDE0270",
justification: Strings.UnityObjectIfNullCoalescingSuppressorJustification);

public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions => ImmutableArray.Create(NullCoalescingRule, NullPropagationRule, CoalescingAssignmentRule, UseIsNullRule, IfNullCoalescingRule);

public override void ReportSuppressions(SuppressionAnalysisContext context)
{
Expand Down