Fix S1172 FP: Unused parameter cannot be removed

[bart-vmware]

Description

Rule S1172 suggests removing unused parameters from a method that is used as an event handler. Therefore the unused parameters cannot be removed without breaking the code.

Repro steps

The code below produces two warnings for unused parameters. While the parameters are unused, they cannot be removed:

#nullable enable

using System.Reflection;

namespace SonarParameterBug;

internal static class Program
{
    public static void Main()
    {
        Console.WriteLine(typeof(AssemblyLoader));
    }
}

internal static class AssemblyLoader
{
    static AssemblyLoader()
    {
        AppDomain.CurrentDomain.AssemblyResolve += AssemblyResolver.LoadAnyVersion;
    }

    private static class AssemblyResolver
    {
        // S1172: Remove this unused method parameter 'sender'.
        // S1172: Remove this unused method parameter 'args'.
        public static Assembly? LoadAnyVersion(object? sender, ResolveEventArgs args)
        {
            throw new NotSupportedException();
        }
    }
}

Expected behavior

No warnings.

Actual behavior

Incorrect warnings. The parameters cannot be removed.

Known workarounds

Moving the method LoadAnyVersion outside of the nested class makes the warnings go away.

Related information

• C#/VB.NET Plugins version: N/A
• Visual Studio version: Visual Studio Community 2022 v17.8.0
• MSBuild / dotnet version: .NET 6
• SonarScanner for .NET version: 9.12.0.78982
• Operating System: Windows 11

[martin-strecker-sonarsource]

Thank you @bart-vmware for reporting the issue. The event AssemblyResolve does not follow the event delegate signature recommendation and is therefore not detected as an eventhandler (see the methodSymbol.ReturnsVoid check):

sonar-dotnet/analyzers/src/SonarAnalyzer.Common/Helpers/KnownMethods.cs

Lines 221 to 230 in 9f6edc4

	public static bool IsEventHandler(this IMethodSymbol methodSymbol) =>
	methodSymbol != null
	&& methodSymbol.ReturnsVoid
	&& methodSymbol.Parameters.Length == 2
	&& (methodSymbol.Parameters[0].Name.Equals("sender", StringComparison.OrdinalIgnoreCase) || methodSymbol.Parameters[0].Type.Is(KnownType.System_Object))
	&& (
	// Inheritance from EventArgs is not enough for UWP or Xamarin as it uses other kind of event args (e.g. ILeavingBackgroundEventArgs)
	methodSymbol.Parameters[1].Type.ToString().EndsWith("EventArgs", StringComparison.Ordinal) ||
	methodSymbol.Parameters[1].Type.DerivesFrom(KnownType.System_EventArgs)
	);

As this event is part of almost all runtimes, we will add support for it in our IsEventHandler logic.

[bart-vmware]

Good point, I didn't realize the divergent signature. But then why does the warning not appear when the handler method is moved outside of the nested class?

[martin-strecker-sonarsource]

But then why does the warning not appear when the handler method is moved outside of the nested class?

We do detect the event handler registration, if it happens in the same class. If the registration happens there, the method is recognized as being unchangeable regardless of the method signature.

[bart-vmware]

Ah, another case where analyzers peek into the implementation of other methods, very unintuitive. We're falling off the cliff all the time while refactoring code for single responsibility. I get that this gets synthetic/simplistic unit tests to pass, but it makes Sonar very unreliable in general. Please consider turning off that "feature". There are good reasons as to why compilers consider method bodies black boxes (jit inlining aside). It also explains why .NET has attributes like [Pure], [NotNullIfNotNull] etc.