diff --git a/doc/index.md b/doc/index.md index a405b86a..9e74824b 100644 --- a/doc/index.md +++ b/doc/index.md @@ -42,6 +42,7 @@ ID | Title | Category [UNT0038](UNT0038.md) | Cache `WaitForSeconds` invocations | Performance [UNT0039](UNT0039.md) | Use `RequireComponent` attribute when self-invoking `GetComponent` | Type Safety [UNT0040](UNT0040.md) | GameObject.isStatic is editor-only | Correctness +[UNT0041](UNT0041.md) | Use Animator.StringToHash for repeated Animator method calls | # Diagnostic Suppressors @@ -70,3 +71,4 @@ ID | Suppressed ID | Justification [USP0021](USP0021.md) | IDE0041 | Prefer reference equality [USP0022](USP0022.md) | IDE0270 | Unity objects should not use if null coalescing [USP0023](USP0023.md) | IDE1006 | The Unity runtime invokes Unity messages + diff --git a/src/Microsoft.Unity.Analyzers.Tests/AnimatorStringToHashTests.cs b/src/Microsoft.Unity.Analyzers.Tests/AnimatorStringToHashTests.cs new file mode 100644 index 00000000..1dd041be --- /dev/null +++ b/src/Microsoft.Unity.Analyzers.Tests/AnimatorStringToHashTests.cs @@ -0,0 +1,438 @@ +/*-------------------------------------------------------------------------------------------- + * 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 AnimatorStringToHashTests : BaseCodeFixVerifierTest +{ + [Fact] + public async Task AnimatorPlayWithStringLiteral() // stateName vs stateNameHash + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void Start() + { + _animator.Play(""Attack""); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(10, 9) + .WithArguments("Play", "Attack"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int AttackHash = Animator.StringToHash(""Attack""); + private Animator _animator = null; + + void Start() + { + _animator.Play(AttackHash); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task AnimatorPlayWithVariable_NoDiagnostic() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + private string _stateName = ""Attack""; + + void Start() + { + _animator.Play(_stateName); + } +} +"; + + await VerifyCSharpDiagnosticAsync(test); + } + + [Fact] + public async Task NonAnimatorType_NoDiagnostic() + { + const string test = @" +using UnityEngine; + +class OtherClass +{ + public void Play(string name) { } +} + +class Test : MonoBehaviour +{ + private OtherClass _other = null; + + void Start() + { + _other.Play(""Attack""); + } +} +"; + + await VerifyCSharpDiagnosticAsync(test); + } + + [Fact] + public async Task AnimatorSetBoolWithStringLiteral() // name vs id + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void Update() + { + _animator.SetBool(""IsRunning"", true); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(10, 9) + .WithArguments("SetBool", "IsRunning"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int IsRunningHash = Animator.StringToHash(""IsRunning""); + private Animator _animator = null; + + void Update() + { + _animator.SetBool(IsRunningHash, true); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task AnimatorSetTriggerWithStringLiteral() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void OnTriggerEnter(Collider other) + { + _animator.SetTrigger(""Jump""); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(10, 9) + .WithArguments("SetTrigger", "Jump"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int JumpHash = Animator.StringToHash(""Jump""); + private Animator _animator = null; + + void OnTriggerEnter(Collider other) + { + _animator.SetTrigger(JumpHash); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task AnimatorGetFloatWithStringLiteral() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void Update() + { + float speed = _animator.GetFloat(""Speed""); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(10, 23) + .WithArguments("GetFloat", "Speed"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int SpeedHash = Animator.StringToHash(""Speed""); + private Animator _animator = null; + + void Update() + { + float speed = _animator.GetFloat(SpeedHash); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + + [Fact] + public async Task AnimatorCrossFadeWithStringLiteral() // stateName vs stateNameHash + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void Start() + { + _animator.CrossFade(""Idle"", 0.5f); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(10, 9) + .WithArguments("CrossFade", "Idle"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int IdleHash = Animator.StringToHash(""Idle""); + private Animator _animator = null; + + void Start() + { + _animator.CrossFade(IdleHash, 0.5f); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task AnimatorWithAlreadyHashedCall_NoDiagnostic() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int AttackHash = Animator.StringToHash(""Attack""); + private Animator _animator = null; + + void Start() + { + _animator.Play(AttackHash); + } +} +"; + + await VerifyCSharpDiagnosticAsync(test); + } + + [Fact] + public async Task AnimatorWithExistingHashField() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int AttackHash = Animator.StringToHash(""Attack""); + private Animator _animator = null; + + void Start() + { + _animator.Play(""Attack""); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(11, 9) + .WithArguments("Play", "Attack"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int AttackHash = Animator.StringToHash(""Attack""); + private Animator _animator = null; + + void Start() + { + _animator.Play(AttackHash); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task AnimatorWithConstString_NoDiagnostic() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private const string AttackState = ""Attack""; + private Animator _animator = null; + + void Start() + { + _animator.Play(AttackState); + } +} +"; + + await VerifyCSharpDiagnosticAsync(test); + } + + [Fact] + public async Task AnimatorWithMethodParameter_NoDiagnostic() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void PlayAnimation(string stateName) + { + _animator.Play(stateName); + } +} +"; + + await VerifyCSharpDiagnosticAsync(test); + } + + [Fact] + public async Task AnimatorWithSpecialCharactersInStateName() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void Start() + { + _animator.Play(""Attack-Heavy 01""); + } +} +"; + + var diagnostic = ExpectDiagnostic() + .WithLocation(10, 9) + .WithArguments("Play", "Attack-Heavy 01"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic); + + const string fixedTest = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private static readonly int AttackHeavy01Hash = Animator.StringToHash(""Attack-Heavy 01""); + private Animator _animator = null; + + void Start() + { + _animator.Play(AttackHeavy01Hash); + } +} +"; + + await VerifyCSharpFixAsync(test, fixedTest); + } + + [Fact] + public async Task AnimatorMultipleCalls() + { + const string test = @" +using UnityEngine; + +class Test : MonoBehaviour +{ + private Animator _animator = null; + + void Start() + { + _animator.Play(""Attack""); + _animator.SetBool(""IsRunning"", true); + } +} +"; + + var diagnostic1 = ExpectDiagnostic() + .WithLocation(10, 9) + .WithArguments("Play", "Attack"); + + var diagnostic2 = ExpectDiagnostic() + .WithLocation(11, 9) + .WithArguments("SetBool", "IsRunning"); + + await VerifyCSharpDiagnosticAsync(test, diagnostic1, diagnostic2); + } +} diff --git a/src/Microsoft.Unity.Analyzers/AnimatorStringToHash.cs b/src/Microsoft.Unity.Analyzers/AnimatorStringToHash.cs new file mode 100644 index 00000000..e4bd1495 --- /dev/null +++ b/src/Microsoft.Unity.Analyzers/AnimatorStringToHash.cs @@ -0,0 +1,212 @@ +/*-------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *-------------------------------------------------------------------------------------------*/ + +using System; +using System.Collections.Immutable; +using System.Linq; +using System.Text.RegularExpressions; +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.CodeAnalysis.Editing; +using Microsoft.Unity.Analyzers.Resources; + +namespace Microsoft.Unity.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class AnimatorStringToHashAnalyzer : DiagnosticAnalyzer +{ + private const string RuleId = "UNT0041"; + + internal static readonly DiagnosticDescriptor Rule = new( + id: RuleId, + title: Strings.AnimatorStringToHashDiagnosticTitle, + messageFormat: Strings.AnimatorStringToHashDiagnosticMessageFormat, + category: DiagnosticCategory.Performance, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + helpLinkUri: HelpLink.ForDiagnosticId(RuleId), + description: Strings.AnimatorStringToHashDiagnosticDescription); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(AnalyzeInvocation, SyntaxKind.InvocationExpression); + } + + private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context) + { + var invocation = (InvocationExpressionSyntax)context.Node; + + if (context.SemanticModel.GetSymbolInfo(invocation).Symbol is not IMethodSymbol methodSymbol) + return; + + var containingType = methodSymbol.ContainingType; + + if (containingType == null || !containingType.Matches(typeof(UnityEngine.Animator))) + return; + + var stringArgument = FindStringArgumentWithHashOverload(invocation, methodSymbol); + if (stringArgument?.Expression is not LiteralExpressionSyntax literal || !literal.IsKind(SyntaxKind.StringLiteralExpression)) + return; + + context.ReportDiagnostic(Diagnostic.Create( + Rule, + invocation.GetLocation(), + methodSymbol.Name, + literal.Token.ValueText)); + } + + internal static ArgumentSyntax? FindStringArgumentWithHashOverload(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) + { + var arguments = invocation.ArgumentList.Arguments; + + for (var i = 0; i < methodSymbol.Parameters.Length && i < arguments.Count; i++) + { + var parameter = methodSymbol.Parameters[i]; + if (!parameter.Type.Matches(typeof(string))) + continue; + + if (HasIntOverloadAtPosition(methodSymbol, i)) + return arguments[i]; + } + + return null; + } + + internal static bool HasIntOverloadAtPosition(IMethodSymbol methodSymbol, int stringParameterIndex) + { + var containingType = methodSymbol.ContainingType; + var overloads = containingType.GetMembers(methodSymbol.Name) + .OfType() + .Where(m => m.Parameters.Length == methodSymbol.Parameters.Length); + + return overloads + .Where(overload => !SymbolEqualityComparer.Default.Equals(overload, methodSymbol)) + .Any(overload => overload.Parameters.Select((param, i) => i == stringParameterIndex + ? param.Type.SpecialType == SpecialType.System_Int32 + : SymbolEqualityComparer.Default.Equals(methodSymbol.Parameters[i].Type, param.Type)) + .All(match => match)); + } +} + +[ExportCodeFixProvider(LanguageNames.CSharp)] +public class AnimatorStringToHashCodeFix : CodeFixProvider +{ + public sealed override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(AnimatorStringToHashAnalyzer.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; + + context.RegisterCodeFix( + CodeAction.Create( + Strings.AnimatorStringToHashCodeFixTitle, + ct => ExtractToHashFieldAsync(context.Document, invocation, ct), + FixableDiagnosticIds.Single()), + context.Diagnostics); + } + + private static async Task ExtractToHashFieldAsync( + Document document, + InvocationExpressionSyntax invocation, + CancellationToken cancellationToken) + { + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + if (semanticModel?.GetSymbolInfo(invocation).Symbol is not IMethodSymbol methodSymbol) + return document; + + var stringArgument = AnimatorStringToHashAnalyzer.FindStringArgumentWithHashOverload(invocation, methodSymbol); + if (stringArgument == null) + return document; + + var literalValue = (stringArgument.Expression as LiteralExpressionSyntax)?.Token.ValueText; + if (literalValue == null) + return document; + + var classDecl = invocation.FirstAncestorOrSelf(); + if (classDecl == null) + return document; + + var fieldName = GenerateFieldName(literalValue); + + var fieldExists = classDecl.Members + .OfType() + .SelectMany(f => f.Declaration.Variables) + .Any(v => v.Identifier.Text == fieldName); + + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + + if (!fieldExists) + { + // Create: private static readonly int FieldName = Animator.StringToHash("value"); + var hashInvocation = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + SyntaxFactory.IdentifierName(nameof(UnityEngine.Animator)), + SyntaxFactory.IdentifierName("StringToHash"))) + .WithArgumentList(SyntaxFactory.ArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument( + SyntaxFactory.LiteralExpression( + SyntaxKind.StringLiteralExpression, + SyntaxFactory.Literal(literalValue)))))); + + var fieldDecl = SyntaxFactory.FieldDeclaration( + SyntaxFactory.VariableDeclaration( + SyntaxFactory.PredefinedType(SyntaxFactory.Token(SyntaxKind.IntKeyword)), + SyntaxFactory.SeparatedList([ + SyntaxFactory.VariableDeclarator( + SyntaxFactory.Identifier(fieldName), + null, + SyntaxFactory.EqualsValueClause(hashInvocation)) + ]))) + .AddModifiers( + SyntaxFactory.Token(SyntaxKind.PrivateKeyword), + SyntaxFactory.Token(SyntaxKind.StaticKeyword), + SyntaxFactory.Token(SyntaxKind.ReadOnlyKeyword)); + + editor.InsertMembers(classDecl, 0, [fieldDecl]); + } + + var newArgument = stringArgument.WithExpression(SyntaxFactory.IdentifierName(fieldName)); + editor.ReplaceNode(stringArgument, newArgument); + + return editor.GetChangedDocument(); + } + + private static string GenerateFieldName(string literalValue) + { + var cleaned = Regex.Replace(literalValue, @"[^a-zA-Z0-9]", " "); + + var words = cleaned.Split([' '], StringSplitOptions.RemoveEmptyEntries); + var pascalCase = string.Concat(words.Select(ToPascalCaseWord)); + + if (!string.IsNullOrEmpty(pascalCase) && char.IsDigit(pascalCase[0])) + pascalCase = "_" + pascalCase; + + return pascalCase + "Hash"; + } + + private static string ToPascalCaseWord(string word) + { + if (string.IsNullOrEmpty(word)) + return ""; + + return char.ToUpperInvariant(word[0]) + word.Substring(1); + } +} diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs index 913fd31e..81dfca6a 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs @@ -60,6 +60,42 @@ internal Strings() { } } + /// + /// Looks up a localized string similar to Use Animator.StringToHash. + /// + internal static string AnimatorStringToHashCodeFixTitle { + get { + return ResourceManager.GetString("AnimatorStringToHashCodeFixTitle", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to String parameters in Animator methods are converted to hashes at runtime. Pre-computing the hash using Animator.StringToHash and caching the result improves performance.. + /// + internal static string AnimatorStringToHashDiagnosticDescription { + get { + return ResourceManager.GetString("AnimatorStringToHashDiagnosticDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use Animator.StringToHash for repeated Animator method calls with string parameter '{1}' in '{0}'.. + /// + internal static string AnimatorStringToHashDiagnosticMessageFormat { + get { + return ResourceManager.GetString("AnimatorStringToHashDiagnosticMessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use Animator.StringToHash instead of strings. + /// + internal static string AnimatorStringToHashDiagnosticTitle { + get { + return ResourceManager.GetString("AnimatorStringToHashDiagnosticTitle", resourceCulture); + } + } + /// /// Looks up a localized string similar to Asset operations such as asset loading should be avoided in LoadAttribute method.. /// diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx index b26ff4df..e25e80e1 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx @@ -650,4 +650,17 @@ GameObject.isStatic is editor-only - \ No newline at end of file + + Use Animator.StringToHash + + + String parameters in Animator methods are converted to hashes at runtime. Pre-computing the hash using Animator.StringToHash and caching the result improves performance. + + + Use Animator.StringToHash for repeated Animator method calls with string parameter '{1}' in '{0}'. + {0} is a method name, {1} is a string literal value + + + Use Animator.StringToHash instead of strings + + diff --git a/src/doc/UNT0041.md b/src/doc/UNT0041.md new file mode 100644 index 00000000..067e8933 --- /dev/null +++ b/src/doc/UNT0041.md @@ -0,0 +1,48 @@ +# UNT0041 Use Animator.StringToHash for repeated Animator method calls + +`Animator` methods have overloads that accept either a string or an int (hash) parameter. Using the string-based overload repeatedly causes Unity to compute the hash every time, which impacts performance. + +## Examples of patterns that are flagged by this analyzer + +```csharp +using UnityEngine; + +class Example : MonoBehaviour +{ + private Animator _animator; + + void Update() + { + // UNT0041: Use Animator.StringToHash for repeated Animator method calls + _animator.Play("Attack"); + _animator.SetBool("IsRunning", true); + _animator.SetFloat("Speed", 1.5f); + } +} +``` + +## Solution + +Pre-compute the hash using `Animator.StringToHash` and store it in a static readonly field: + +```csharp +using UnityEngine; + +class Example : MonoBehaviour +{ + private static readonly int AttackHash = Animator.StringToHash("Attack"); + private static readonly int IsRunningHash = Animator.StringToHash("IsRunning"); + private static readonly int SpeedHash = Animator.StringToHash("Speed"); + + private Animator _animator; + + void Update() + { + _animator.Play(AttackHash); + _animator.SetBool(IsRunningHash, true); + _animator.SetFloat(SpeedHash, 1.5f); + } +} +``` + +A code fix is offered for this diagnostic to automatically extract the string literal to a cached hash field.