New rule S6960 for C#: Controllers should not have too many responsib…

…ilities (#9111)

Co-authored-by: mary-georgiou-sonarsource <mary.georgiou@sonarsource.com>

analyzers/rspec/cs/S6960.json

@@ -0,0 +1,24 @@
+{
+  "title": "Controllers should not have mixed responsibilities",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Linear",
+    "linearDesc": "responsibilities",
+    "linearFactor": "15min"
+  },
+  "tags": [
+    "asp.net"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6960",
+  "sqKey": "S6960",
+  "scope": "Main",
+  "quickfix": "partial",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "HIGH"
+    },
+    "attribute": "MODULAR"
+  }
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -316,6 +316,7 @@
     "S6930",
     "S6931",
     "S6934",
+    "S6960",
     "S6961",
     "S6962",
     "S6964",

analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveMixedResponsibilities.cs

@@ -0,0 +1,203 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2024 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+using System.Collections.Concurrent;
+
+namespace SonarAnalyzer.Rules.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class ControllersHaveMixedResponsibilities : SonarDiagnosticAnalyzer
+{
+    private const string DiagnosticId = "S6960";
+    private const string MessageFormat = "This controller has multiple responsibilities and could be split into {0} smaller controllers.";
+    private const string UnspeakableIndexerName = "<indexer>I"; // All indexers are considered part of the same group
+
+    public enum MemberType
+    {
+        Service,
+        Action,
+    }
+
+    private static readonly HashSet<string> ExcludedWellKnownServices =
+    [
+        "ILogger",
+        "IMediator",
+        "IMapper",
+        "IConfiguration",
+        "IBus",
+        "IMessageBus",
+        "IHttpClientFactory"
+    ];
+
+    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterCompilationStartAction(compilationStartContext =>
+        {
+            if (!compilationStartContext.Compilation.ReferencesControllers())
+            {
+                return;
+            }
+
+            compilationStartContext.RegisterSymbolStartAction(symbolStartContext =>
+            {
+                var symbol = (INamedTypeSymbol)symbolStartContext.Symbol;
+                if (symbol.IsCoreApiController()
+                    && symbol.BaseType.Is(KnownType.Microsoft_AspNetCore_Mvc_ControllerBase)
+                    && !symbol.IsAbstract)
+                {
+                    var relevantMembers = RelevantMembers(symbol);
+
+                    if (relevantMembers.Count < 2)
+                    {
+                        return;
+                    }
+
+                    var dependencies = new ConcurrentStack<Dependency>();
+                    symbolStartContext.RegisterCodeBlockStartAction(PopulateDependencies(relevantMembers, dependencies));
+                    symbolStartContext.RegisterSymbolEndAction(CalculateAndReportOnResponsibilities(symbol, relevantMembers, dependencies));
+                }
+                }, SymbolKind.NamedType);
+        });
+
+    private static Action<SonarCodeBlockStartAnalysisContext<SyntaxKind>> PopulateDependencies(
+        ImmutableDictionary<string, MemberType> relevantMembers,
+        ConcurrentStack<Dependency> dependencies) =>
+        codeBlockStartContext =>
+        {
+            if (BlockName(codeBlockStartContext.CodeBlock) is { } blockName)
+            {
+                codeBlockStartContext.RegisterNodeAction(c =>
+                {
+                    if (c.Node.GetName() is { } dependencyName && relevantMembers.ContainsKey(blockName) && relevantMembers.ContainsKey(dependencyName))
+                    {
+                        dependencies.Push(new(blockName, dependencyName));
+                    }
+                }, SyntaxKind.IdentifierName);
+            }
+        };
+
+    private static Action<SonarSymbolReportingContext> CalculateAndReportOnResponsibilities(
+        INamedTypeSymbol controllerSymbol,
+        ImmutableDictionary<string, MemberType> relevantMembers,
+        ConcurrentStack<Dependency> dependencies) =>
+        symbolEndContext =>
+        {
+            if (ResponsibilityGroups(relevantMembers, dependencies) is { Count: > 1 } responsibilityGroups)
+            {
+                var secondaryLocations = SecondaryLocations(controllerSymbol, responsibilityGroups).ToList();
+                foreach (var primaryLocation in IdentifierLocations<ClassDeclarationSyntax>(controllerSymbol))
+                {
+                    symbolEndContext.ReportIssue(Diagnostic.Create(
+                        Rule,
+                        primaryLocation,
+                        secondaryLocations.ToAdditionalLocations(),
+                        secondaryLocations.ToProperties(),
+                        responsibilityGroups.Count));
+                }
+            }
+        };
+
+    private static List<List<string>> ResponsibilityGroups(
+        ImmutableDictionary<string, MemberType> relevantMembers,
+        ConcurrentStack<Dependency> dependencies)
+    {
+        var dependencySets = new DisjointSets(relevantMembers.Keys);
+        foreach (var dependency in dependencies)
+        {
+            dependencySets.Union(dependency.From, dependency.To);
+        }
+
+        return dependencySets
+            .GetAllSets()
+            // Filter out sets of only actions or only services
+            .Where(x => x.Exists(x => relevantMembers[x] == MemberType.Service) && x.Exists(x => relevantMembers[x] == MemberType.Action))
+            .ToList();
+    }
+
+    private static string BlockName(SyntaxNode block) =>
+        block switch
+        {
+            AccessorDeclarationSyntax { Parent.Parent: PropertyDeclarationSyntax property } => property.GetName(),
+            AccessorDeclarationSyntax { Parent.Parent: IndexerDeclarationSyntax } => UnspeakableIndexerName,
+            ArrowExpressionClauseSyntax { Parent: PropertyDeclarationSyntax property } => property.GetName(),
+            ArrowExpressionClauseSyntax { Parent: IndexerDeclarationSyntax } => UnspeakableIndexerName,
+            MethodDeclarationSyntax method => method.GetName(),
+            PropertyDeclarationSyntax property => property.GetName(),
+            _ => null
+        };
+
+    private static ImmutableDictionary<string, MemberType> RelevantMembers(INamedTypeSymbol symbol)
+    {
+        var builder = ImmutableDictionary.CreateBuilder<string, MemberType>();
+        foreach (var member in symbol.GetMembers().Where(x => !x.IsStatic))
+        {
+            switch (member)
+            {
+                // Constructors are not considered because they have to be split anyway
+                // Accessors are not considered because they are part of properties, that are considered as a whole
+                case IMethodSymbol method when !method.IsConstructor() && method.MethodKind != MethodKind.StaticConstructor && method.AssociatedSymbol is not IPropertySymbol:
+                    builder.Add(method.Name, MemberType.Action);
+                    break;
+                // Indexers are treated as methods with an unspeakable name
+                case IPropertySymbol { IsIndexer: true }:
+                    builder.Add(UnspeakableIndexerName, MemberType.Action);
+                    break;
+                // Primary constructor parameters may or may not generate fields, and must be considered
+                case IMethodSymbol method when method.IsPrimaryConstructor():
+                    builder.AddRange(method.Parameters.Where(IsService).Select(x => new KeyValuePair<string, MemberType>(x.Name, MemberType.Service)));
+                    break;
+                // Backing fields are excluded for auto-properties, since they are considered part of the property
+                case IFieldSymbol field when IsService(field) && !field.IsImplicitlyDeclared:
+                    builder.Add(field.Name, MemberType.Service);
+                    break;
+                case IPropertySymbol property when IsService(property):
+                    builder.Add(property.Name, MemberType.Service);
+                    break;
+            }
+        }
+
+        return builder.ToImmutable();
+    }
+
+    private static bool IsService(ISymbol symbol) =>
+        !ExcludedWellKnownServices.Contains(symbol.GetSymbolType().Name);
+
+    private static IEnumerable<SecondaryLocation> SecondaryLocations(INamedTypeSymbol controllerSymbol, List<List<string>> sets)
+    {
+        for (var setIndex = 0; setIndex < sets.Count; setIndex++)
+        {
+            foreach (var memberLocation in sets[setIndex].SelectMany(MemberLocations))
+            {
+                yield return new SecondaryLocation(memberLocation, $"May belong to responsibility #{setIndex + 1}.");
+            }
+        }
+
+        IEnumerable<Location> MemberLocations(string memberName) =>
+            controllerSymbol.GetMembers(memberName).OfType<IMethodSymbol>().SelectMany(IdentifierLocations<MethodDeclarationSyntax>);
+    }
+
+    private static IEnumerable<Location> IdentifierLocations<T>(ISymbol symbol) where T : SyntaxNode =>
+        symbol.DeclaringSyntaxReferences.Select(x => x.GetSyntax()).OfType<T>().Select(x => x.GetIdentifier()?.GetLocation());
+
+    private record struct Dependency(string From, string To);
+}

analyzers/src/SonarAnalyzer.Common/Helpers/AspNetMvcHelper.cs

@@ -59,8 +59,7 @@ public static class AspNetMvcHelper
             && IsControllerType(methodSymbol.ContainingType);
 
         /// <summary>
-        /// Returns a value indicating whether the provided type symbol is a ASP.NET MVC
-        /// controller.
+        /// Whether the provided type symbol is a ASP.NET MVC controller.
         /// </summary>
         public static bool IsControllerType(this INamedTypeSymbol namedType) =>
             namedType is not null
@@ -69,6 +68,15 @@ public static class AspNetMvcHelper
                 || namedType.GetAttributes(ControllerAttributeTypes).Any())
             && !namedType.GetAttributes(NonControllerAttributeTypes).Any();
 
+        /// <summary>
+        /// Whether the provided type symbol is an ASP.NET Core API controller.
+        /// Considers as API controllers also controllers deriving from ControllerBase but not Controller.
+        /// </summary>
+        public static bool IsCoreApiController(this INamedTypeSymbol namedType) =>
+            namedType.IsControllerType()
+            && (namedType.GetAttributesWithInherited().Any(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiControllerAttribute))
+                || (namedType.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ControllerBase) && !namedType.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_Controller)));
+
         public static bool ReferencesControllers(this Compilation compilation) =>
             compilation.GetTypeByMetadataName(KnownType.System_Web_Mvc_Controller) is not null
             || compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_Controller) is not null

analyzers/tests/SonarAnalyzer.Test/Common/DisjointSetsTest.cs

@@ -1,4 +1,24 @@
-namespace SonarAnalyzer.Test.Common;
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2024 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+namespace SonarAnalyzer.Test.Common;
 
 [TestClass]
 public class DisjointSetsTest

analyzers/tests/SonarAnalyzer.Test/Extensions/SyntaxNodeExtensionsTest.cs

@@ -98,7 +98,7 @@ public void ArrowExpressionBody_WithNotExpectedNode_ReturnsNull()
     [TestMethod]
     public void GetDeclarationTypeName_UnknownType() =>
 #if DEBUG
-        Assert.ThrowsException<System.ArgumentException>(() => SyntaxNodeExtensionsCSharp.GetDeclarationTypeName(SyntaxFactory.Block()), "Unexpected type Block\r\nParameter name: kind");
+        Assert.ThrowsException<UnexpectedValueException>(() => SyntaxNodeExtensionsCSharp.GetDeclarationTypeName(SyntaxFactory.Block()), "Unexpected type Block\r\nParameter name: kind");
 #else
         SyntaxNodeExtensionsCSharp.GetDeclarationTypeName(SyntaxFactory.Block()).Should().Be("type");
 #endif

analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs

@@ -6884,7 +6884,7 @@ internal static class RuleTypeMappingCS
         // ["S6957"],
         // ["S6958"],
         // ["S6959"],
-        // ["S6960"],
+        ["S6960"] = "CODE_SMELL",
         ["S6961"] = "CODE_SMELL",
         ["S6962"] = "CODE_SMELL",
         // ["S6963"],

analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveMixedResponsibilitiesTest.cs

@@ -0,0 +1,51 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2024 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+using Microsoft.CodeAnalysis.CSharp;
+using SonarAnalyzer.Rules.CSharp;
+
+namespace SonarAnalyzer.Test.Rules;
+
+#if NET
+
+[TestClass]
+public class ControllersHaveMixedResponsibilitiesTest
+{
+    private readonly VerifierBuilder builder =
+        new VerifierBuilder<ControllersHaveMixedResponsibilities>().AddReferences(References).WithBasePath("AspNet");
+
+    private static IEnumerable<MetadataReference> References =>
+    [
+        AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions,
+        AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore,
+        AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures,
+        CoreMetadataReference.SystemComponentModel, // For IServiceProvider
+        .. NuGetMetadataReference.MicrosoftExtensionsDependencyInjectionAbstractions("8.0.1"), // For IServiceProvider extensions
+    ];
+
+    [TestMethod]
+    public void ControllersHaveMixedResponsibilities_CS() =>
+        builder
+            .AddPaths("ControllersHaveMixedResponsibilities.Latest.cs")
+            .WithLanguageVersion(LanguageVersion.Latest)
+            .Verify();
+}
+
+#endif