diff --git a/doc/USP0024.md b/doc/USP0024.md new file mode 100644 index 0000000..4f9d147 --- /dev/null +++ b/doc/USP0024.md @@ -0,0 +1,31 @@ +# USP0024 The Unity Input System invokes input messages + +Private methods with input message signatures are not unused. + +## Suppressed Diagnostic ID + +IDE0051 - Remove unused private members + +## Examples of code that produces a suppressed diagnostic +```csharp +using UnityEngine; +using UnityEngine.InputSystem; + +class Player : MonoBehaviour +{ + public Vector2 rawInput; + + private void OnMove(InputValue value) + { + rawInput = value.Get(); + } +} +``` + +## Why is the diagnostic reported? + +The IDE cannot detect any calls to `OnMove` in the class. + +## Why do we suppress this diagnostic? + +With the `SendMessages` or `BroadcastMessages` notification behavior, `PlayerInput` invokes `On` + action name methods through `SendMessage`, which is undetectable by the IDE. The same applies to notification messages taking a `PlayerInput` argument (like `OnDeviceLost` or `OnPlayerJoined`) and to callbacks taking an `InputAction.CallbackContext` argument, invoked through serialized `UnityEvents`. diff --git a/doc/USP0025.md b/doc/USP0025.md new file mode 100644 index 0000000..7880d69 --- /dev/null +++ b/doc/USP0025.md @@ -0,0 +1,29 @@ +# USP0025 The Unity Input System invokes input messages + +Unused parameters should not be removed from input messages. + +## Suppressed Diagnostic ID + +IDE0060 - Remove unused parameters + +## Examples of code that produces a suppressed diagnostic +```csharp +using UnityEngine; +using UnityEngine.InputSystem; + +class Player : MonoBehaviour +{ + private void OnFire(InputAction.CallbackContext context) + { + // do stuff, but context remains unused + } +} +``` + +## Why is the diagnostic reported? + +The IDE does not detect that you've used `context`, and under normal circumstances, it would be reasonable to remove the unused parameter. + +## Why do we suppress this diagnostic? + +The IDE doesn't realize this method is invoked by the Input System, and therefore has no way of determining that it needs to have a specific signature to function correctly. diff --git a/doc/index.md b/doc/index.md index 92bf50b..6f0c0ff 100644 --- a/doc/index.md +++ b/doc/index.md @@ -73,4 +73,6 @@ 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 +[USP0024](USP0024.md) | IDE0051 | The Unity Input System invokes input messages +[USP0025](USP0025.md) | IDE0060 | The Unity Input System invokes input messages diff --git a/src/Microsoft.Unity.Analyzers.Tests/InputSystemMessageSuppressorTests.cs b/src/Microsoft.Unity.Analyzers.Tests/InputSystemMessageSuppressorTests.cs new file mode 100644 index 0000000..909849e --- /dev/null +++ b/src/Microsoft.Unity.Analyzers.Tests/InputSystemMessageSuppressorTests.cs @@ -0,0 +1,137 @@ +/*-------------------------------------------------------------------------------------------- + * 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 Microsoft.CodeAnalysis; +using Xunit; + +namespace Microsoft.Unity.Analyzers.Tests; + +public class InputSystemMessageSuppressorTests : BaseSuppressorVerifierTest +{ + // The Input System package is not available in test infrastructure, so tests declare matching types. + + [Fact] + public async Task InputValueMessageSuppressed() + { + const string test = @" +using UnityEngine; +using UnityEngine.InputSystem; + +namespace UnityEngine.InputSystem +{ + public class InputValue + { + public T Get() { return default(T); } + } +} + +class Player : MonoBehaviour +{ + public Vector2 rawInput; + + private void OnMove(InputValue value) + { + rawInput = value.Get(); + } +}"; + + var suppressor = ExpectSuppressor(InputSystemMessageSuppressor.MethodRule) + .WithLocation(17, 18); + + await VerifyCSharpDiagnosticAsync(test, suppressor); + } + + [Fact] + public async Task CallbackContextMessageAndParameterSuppressed() + { + const string test = @" +using UnityEngine; +using UnityEngine.InputSystem; + +namespace UnityEngine.InputSystem +{ + public class InputAction + { + public struct CallbackContext { } + } +} + +class Player : MonoBehaviour +{ + private void OnFire(InputAction.CallbackContext context) + { + } +}"; + + var suppressors = new[] { + ExpectSuppressor(InputSystemMessageSuppressor.MethodRule).WithLocation(15, 18), + ExpectSuppressor(InputSystemMessageSuppressor.ParameterRule).WithLocation(15, 53), + }; + + await VerifyCSharpDiagnosticAsync(test, suppressors); + } + + [Fact] + public async Task PlayerInputNotificationMessageAndParameterSuppressed() + { + const string test = @" +using UnityEngine; +using UnityEngine.InputSystem; + +namespace UnityEngine.InputSystem +{ + public class PlayerInput { } +} + +class Player : MonoBehaviour +{ + private void OnDeviceLost(PlayerInput input) + { + } +}"; + + var suppressors = new[] { + ExpectSuppressor(InputSystemMessageSuppressor.MethodRule).WithLocation(12, 18), + ExpectSuppressor(InputSystemMessageSuppressor.ParameterRule).WithLocation(12, 43), + }; + + await VerifyCSharpDiagnosticAsync(test, suppressors); + } + + [Fact] + public async Task MethodWithoutOnPrefixNotSuppressed() + { + const string test = @" +using UnityEngine; +using UnityEngine.InputSystem; + +namespace UnityEngine.InputSystem +{ + public class InputValue + { + public T Get() { return default(T); } + } +} + +class Player : MonoBehaviour +{ + public Vector2 rawInput; + + private void HandleMove(InputValue value) + { + rawInput = value.Get(); + } +}"; + + var not = ExpectNotSuppressed(InputSystemMessageSuppressor.MethodRule) + .WithLocation(17, 18) + .WithSeverity(DiagnosticSeverity.Info) + .WithMessageFormat("Private member '{0}' is unused") + .WithArguments("Player.HandleMove"); + + await VerifyCSharpDiagnosticAsync(test, not); + } +} diff --git a/src/Microsoft.Unity.Analyzers/InputSystemMessageSuppressor.cs b/src/Microsoft.Unity.Analyzers/InputSystemMessageSuppressor.cs new file mode 100644 index 0000000..c93426d --- /dev/null +++ b/src/Microsoft.Unity.Analyzers/InputSystemMessageSuppressor.cs @@ -0,0 +1,98 @@ +/*-------------------------------------------------------------------------------------------- + * 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 Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.Unity.Analyzers.Resources; + +namespace Microsoft.Unity.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class InputSystemMessageSuppressor : DiagnosticSuppressor +{ + internal static readonly SuppressionDescriptor MethodRule = new( + id: "USP0024", + suppressedDiagnosticId: "IDE0051", + justification: Strings.InputSystemMessageSuppressorJustification); + + internal static readonly SuppressionDescriptor ParameterRule = new( + id: "USP0025", + suppressedDiagnosticId: "IDE0060", + justification: Strings.InputSystemMessageSuppressorJustification); + + public override ImmutableArray SupportedSuppressions => ImmutableArray.Create( + MethodRule, + ParameterRule + ); + + public override void ReportSuppressions(SuppressionAnalysisContext context) + { + foreach (var diagnostic in context.ReportedDiagnostics) + { + AnalyzeDiagnostic(diagnostic, context); + } + } + + private void AnalyzeDiagnostic(Diagnostic diagnostic, SuppressionAnalysisContext context) + { + var node = context.GetSuppressibleNode(diagnostic, n => n is ParameterSyntax or MethodDeclarationSyntax); + + if (node is ParameterSyntax) + { + node = node + .Ancestors() + .OfType() + .FirstOrDefault(); + } + + if (node == null) + return; + + if (diagnostic.Location.SourceTree is not { } syntaxTree) + return; + + var model = context.GetSemanticModel(syntaxTree); + if (model.GetDeclaredSymbol(node) is not IMethodSymbol methodSymbol) + return; + + if (!IsInputSystemMessage(methodSymbol)) + return; + + foreach (var suppression in SupportedSuppressions) + { + if (suppression.SuppressedDiagnosticId == diagnostic.Id) + context.ReportSuppression(Suppression.Create(suppression, diagnostic)); + } + } + + private static bool IsInputSystemMessage(IMethodSymbol methodSymbol) + { + // With the SendMessages/BroadcastMessages notification behavior, PlayerInput invokes + // 'On' + action name methods taking an optional InputValue argument, and device/player + // notifications (OnDeviceLost, OnPlayerJoined, ...) taking a PlayerInput argument. + // With the UnityEvents notification behavior, callbacks take an InputAction.CallbackContext + // argument and are invoked through serialized events. + if (!methodSymbol.ReturnsVoid) + return false; + + if (!methodSymbol.Name.StartsWith("On", StringComparison.Ordinal)) + return false; + + if (!methodSymbol.ContainingType.Extends(typeof(UnityEngine.MonoBehaviour))) + return false; + + if (methodSymbol.Parameters.Length != 1) + return false; + + var parameterType = methodSymbol.Parameters[0].Type; + return parameterType.Matches(typeof(UnityEngine.InputSystem.InputValue)) + || parameterType.Matches(typeof(UnityEngine.InputSystem.InputAction.CallbackContext)) + || parameterType.Matches(typeof(UnityEngine.InputSystem.PlayerInput)); + } +} diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs index 690d9ed..8f3848f 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.Designer.cs @@ -735,6 +735,15 @@ internal static string InputGetKeyDiagnosticTitle { } } + /// + /// Looks up a localized string similar to The Unity Input System invokes input messages.. + /// + internal static string InputSystemMessageSuppressorJustification { + get { + return ResourceManager.GetString("InputSystemMessageSuppressorJustification", resourceCulture); + } + } + /// /// Looks up a localized string similar to Fix method signature. /// diff --git a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx index c21c2d2..dc9cd7d 100644 --- a/src/Microsoft.Unity.Analyzers/Resources/Strings.resx +++ b/src/Microsoft.Unity.Analyzers/Resources/Strings.resx @@ -466,6 +466,9 @@ Input.GetKey overloads with KeyCode argument + + The Unity Input System invokes input messages. + Use TryGetComponent instead diff --git a/src/Microsoft.Unity.Analyzers/UnityStubs.cs b/src/Microsoft.Unity.Analyzers/UnityStubs.cs index 218f748..b5044d2 100644 --- a/src/Microsoft.Unity.Analyzers/UnityStubs.cs +++ b/src/Microsoft.Unity.Analyzers/UnityStubs.cs @@ -628,6 +628,18 @@ protected virtual void OnCanvasHierarchyChanged() { } } } +namespace UnityEngine.InputSystem +{ + class InputValue { } + + class InputAction + { + public struct CallbackContext { } + } + + class PlayerInput : MonoBehaviour { } +} + namespace UnityEngine.Networking { class NetworkReader { }