From 2faed96faf6719f1246b89f1b2c4a0cadec571e5 Mon Sep 17 00:00:00 2001 From: Sebastien Lebreton Date: Fri, 15 Jul 2022 17:35:00 +0200 Subject: [PATCH] Add UNT0030, Calling `Destroy` or `DestroyImmediate` on a `Transform` (#231) * Add UNT0030, Calling Destroy or DestroyImmediate on a Transform * Remove extra spaces --- doc/UNT0030.md | 33 ++++ doc/index.md | 1 + .../DestroyTransformTests.cs | 162 ++++++++++++++++++ .../DestroyTransform.cs | 139 +++++++++++++++ .../Resources/Strings.Designer.cs | 36 ++++ .../Resources/Strings.resx | 12 ++ 6 files changed, 383 insertions(+) create mode 100644 doc/UNT0030.md create mode 100644 src/Microsoft.Unity.Analyzers.Tests/DestroyTransformTests.cs create mode 100644 src/Microsoft.Unity.Analyzers/DestroyTransform.cs diff --git a/doc/UNT0030.md b/doc/UNT0030.md new file mode 100644 index 00000000..662eb165 --- /dev/null +++ b/doc/UNT0030.md @@ -0,0 +1,33 @@ +# UNT0030 Calling Destroy or DestroyImmediate on a Transform + +Calling `Object.Destroy` or `Object.DestroyImmediate` using a `Transform` argument is not allowed and will produce an error message at runtime. + +## Examples of patterns that are flagged by this analyzer + +```csharp +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Destroy(transform); + } +} +``` + +## Solution + +Destroy the related `GameObject` instead: + +```csharp +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Destroy(transform.gameObject); + } +} +``` + +A code fix is offered for this diagnostic to automatically apply this change. diff --git a/doc/index.md b/doc/index.md index ff64b880..1a162289 100644 --- a/doc/index.md +++ b/doc/index.md @@ -31,6 +31,7 @@ ID | Title | Category [UNT0027](UNT0027.md) | Do not call PropertyDrawer.OnGUI() | Correctness [UNT0028](UNT0028.md) | Use non-allocating physics APIs | Performance [UNT0029](UNT0029.md) | Pattern matching with null on Unity objects | Correctness +[UNT0030](UNT0030.md) | Calling Destroy or DestroyImmediate on a Transform | Correctness # Diagnostic Suppressors diff --git a/src/Microsoft.Unity.Analyzers.Tests/DestroyTransformTests.cs b/src/Microsoft.Unity.Analyzers.Tests/DestroyTransformTests.cs new file mode 100644 index 00000000..d7175c71 --- /dev/null +++ b/src/Microsoft.Unity.Analyzers.Tests/DestroyTransformTests.cs @@ -0,0 +1,162 @@ +/*-------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *-------------------------------------------------------------------------------------------*/ + +using System.Threading.Tasks; +using Xunit; + +namespace Microsoft.Unity.Analyzers.Tests; + +public class DestroyTransformTests : BaseCodeFixVerifierTest +{ + [Fact] + public async Task TestValidDestroy() + { + const string test = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Destroy(gameObject); + } +} +"; + + await VerifyCSharpDiagnosticAsync(test); + } + + [Fact] + public async Task TestDestroy() + { + const string test = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Destroy(transform); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(7, 9); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Destroy(transform.gameObject); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task TestObjectDestroy() + { + const string test = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Object.Destroy(transform, 5); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(7, 9); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Object.Destroy(transform.gameObject, 5); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task TestDestroyImmediate() + { + const string test = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + DestroyImmediate(transform); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(7, 9); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + DestroyImmediate(transform.gameObject); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task TestObjectDestroyImmediate() + { + const string test = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Object.DestroyImmediate(transform, true); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(7, 9); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Camera : MonoBehaviour +{ + public void Update() { + Object.DestroyImmediate(transform.gameObject, true); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + +} diff --git a/src/Microsoft.Unity.Analyzers/DestroyTransform.cs b/src/Microsoft.Unity.Analyzers/DestroyTransform.cs new file mode 100644 index 00000000..c8950f61 --- /dev/null +++ b/src/Microsoft.Unity.Analyzers/DestroyTransform.cs @@ -0,0 +1,139 @@ +/*-------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *-------------------------------------------------------------------------------------------*/ + +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.Unity.Analyzers.Resources; + +namespace Microsoft.Unity.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class DestroyTransformAnalyzer : DiagnosticAnalyzer +{ + internal static readonly DiagnosticDescriptor Rule = new( + id: "UNT0030", + title: Strings.DestroyTransformDiagnosticTitle, + messageFormat: Strings.DestroyTransformDiagnosticMessageFormat, + category: DiagnosticCategory.Correctness, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: Strings.DestroyTransformDiagnosticDescription); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(AnalyzeInvocation, SyntaxKind.InvocationExpression); + } + + internal static readonly HashSet DestroyMethodNames = new(new[] {"Destroy", "DestroyImmediate"}); + + private static bool InvocationMatches(SyntaxNode node) + { + switch (node) + { + case InvocationExpressionSyntax ies: + return InvocationMatches(ies.Expression); + case MemberAccessExpressionSyntax maes: + return InvocationMatches(maes.Name); + case IdentifierNameSyntax ins: + var text = ins.Identifier.Text; + return DestroyMethodNames.Contains(text); + default: + return false; + } + } + + internal static bool InvocationMatches(InvocationExpressionSyntax invocation, [NotNullWhen(true)] out ExpressionSyntax? argument) + { + argument = null; + + if (!InvocationMatches(invocation)) + return false; + + argument = invocation + .ArgumentList + .Arguments + .FirstOrDefault()? + .Expression; + + return argument != null; + } + + private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context) + { + if (context.Node is not InvocationExpressionSyntax invocation) + return; + + if (!InvocationMatches(invocation, out var argument)) + return; + + var model = context.SemanticModel; + if (model.GetSymbolInfo(invocation.Expression).Symbol is not IMethodSymbol methodSymbol) + return; + + var typeSymbol = methodSymbol.ContainingType; + if (!typeSymbol.Matches(typeof(UnityEngine.Object))) + return; + + var transformTypeSymbol = model + .GetTypeInfo(argument) + .Type; + + if (!transformTypeSymbol.Extends(typeof(UnityEngine.Transform))) + return; + + context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.GetLocation())); + } +} + +[ExportCodeFixProvider(LanguageNames.CSharp)] +public class DestroyTransformCodeFix : CodeFixProvider +{ + public sealed override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(DestroyTransformAnalyzer.Rule.Id); + + public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var invocation = await context.GetFixableNodeAsync(); + if (invocation == null) + return; + + if (!DestroyTransformAnalyzer.InvocationMatches(invocation, out var argument)) + return; + + context.RegisterCodeFix( + CodeAction.Create( + Strings.DestroyTransformCodeFixTitle, + ct => UseGameObjectAsync(context.Document, argument, ct), + invocation.ToFullString()), + context.Diagnostics); + } + + private async Task UseGameObjectAsync(Document document, ExpressionSyntax argument, CancellationToken cancellationToken) + { + var gameObject = SyntaxFactory.IdentifierName("gameObject"); + var memberAccess = SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, argument, gameObject); + + var root = await document + .GetSyntaxRootAsync(cancellationToken) + .ConfigureAwait(false); + + var newRoot = root?.ReplaceNode(argument, memberAccess); + return newRoot == null ? document : document.WithSyntaxRoot(newRoot); + } +} diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs index 49487bcc..a8fc2999 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs @@ -159,6 +159,42 @@ internal static string CreateScriptableObjectInstanceDiagnosticTitle { } } + /// + /// Looks up a localized string similar to Use gameObject instead. + /// + internal static string DestroyTransformCodeFixTitle { + get { + return ResourceManager.GetString("DestroyTransformCodeFixTitle", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to You cannot destroy a Transform component of a GameObject, but you can destroy the whole GameObject.. + /// + internal static string DestroyTransformDiagnosticDescription { + get { + return ResourceManager.GetString("DestroyTransformDiagnosticDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Calling Destroy or DestroyImmediate on a Transform is not allowed.. + /// + internal static string DestroyTransformDiagnosticMessageFormat { + get { + return ResourceManager.GetString("DestroyTransformDiagnosticMessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Calling Destroy or DestroyImmediate on a Transform. + /// + internal static string DestroyTransformDiagnosticTitle { + get { + return ResourceManager.GetString("DestroyTransformDiagnosticTitle", resourceCulture); + } + } + /// /// Looks up a localized string similar to Remove empty Unity message. /// diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx index b3fb7b4a..eb797e6f 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx @@ -522,4 +522,16 @@ Pattern matching with null on Unity objects + + Use gameObject instead + + + You cannot destroy a Transform component of a GameObject, but you can destroy the whole GameObject. + + + Calling Destroy or DestroyImmediate on a Transform is not allowed. + + + Calling Destroy or DestroyImmediate on a Transform + \ No newline at end of file