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

…ilities

analyzers/rspec/cs/S6960.json

@@ -0,0 +1,24 @@
+{
+  "title": "Controllers should not have too many 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",
     "S6965",

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

@@ -0,0 +1,222 @@
+/*
+ * 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 ControllersHaveTooManyResponsibilities : SonarDiagnosticAnalyzer
+{
+    public enum MemberType
+    {
+        Service,
+        Action,
+    }
+
+    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
+
+    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())
+            {
+                compilationStartContext.RegisterSymbolStartAction(symbolStartContext =>
+                {
+                    var symbol = (INamedTypeSymbol)symbolStartContext.Symbol;
+                    if (symbol.IsCoreApiController())
+                    {
+                        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(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();
+                var responsibilityCount = secondaryLocations.Select(x => x.Message).Distinct().Count();
+                if (responsibilityCount > 1)
+                {
+                    foreach (var primaryLocation in IdentifierLocations<ClassDeclarationSyntax>(controllerSymbol))
+                    {
+                        symbolEndContext.ReportIssue(Diagnostic.Create(
+                            Rule,
+                            primaryLocation,
+                            secondaryLocations.ToAdditionalLocations(),
+                            secondaryLocations.ToProperties(),
+                            responsibilityCount));
+                    }
+                }
+            }
+        };
+
+    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(mergeSingletons: true) // Actions with no dependencies don't form new responsibilities
+            .Where(set => set.Any(item => relevantMembers[item] == MemberType.Service)) // Filter out sets of actions not using any service
+            .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())
+        {
+            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.IsStaticConstructor() && method.AssociatedSymbol is not IPropertySymbol:
+                    builder.Add(method.Name, 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;
+                // Indexers are treated as methods with an unspeakable name
+                case IPropertySymbol { IsIndexer: true } indexer:
+                    builder.Add(UnspeakableIndexerName, MemberType.Action);
+                    break;
+            }
+        }
+
+        return builder.ToImmutable();
+    }
+
+    private static bool IsService(ISymbol symbol) =>
+        symbol.GetSymbolType() switch
+        {
+            { TypeKind: TypeKind.Interface, Name: var name } =>
+                !ExcludedWellKnownServices.Contains(name),
+            INamedTypeSymbol { TypeKind: TypeKind.Delegate or TypeKind.Class } type =>
+                IsServiceWrapper(type)
+                || type.Name.EndsWith("service", StringComparison.OrdinalIgnoreCase)
+                || symbol.Name.EndsWith("service", StringComparison.OrdinalIgnoreCase),
+            _ => false,
+        };
+
+    private static IEnumerable<SecondaryLocation> SecondaryLocations(INamedTypeSymbol controllerSymbol, List<List<string>> sets)
+    {
+        var responsibilityIndex = 1;
+        foreach (var set in sets)
+        {
+            var atLeastOneLocation = false;
+            foreach (var memberLocation in set.SelectMany(MemberLocations))
+            {
+                atLeastOneLocation = true;
+                yield return new SecondaryLocation(memberLocation, $"May belong to responsibility #{responsibilityIndex}.");
+            }
+
+            if (atLeastOneLocation)
+            {
+                responsibilityIndex++;
+            }
+        }
+
+        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 static bool IsServiceWrapper(INamedTypeSymbol type) =>
+        (type.IsAny(KnownType.SystemFuncVariants) || type.Is(KnownType.System_Lazy)) && IsService(type.TypeArguments.Last());
+
+    private record struct Dependency(string From, string To);
+}

analyzers/src/SonarAnalyzer.Common/Common/DisjointSets.cs

@@ -47,6 +47,29 @@ public class DisjointSets
         parents[element] is var root && root != element ? FindRoot(root) : root;
 
     // Set elements are sorted in ascending order. Sets are sorted in ascending order by their first element.
-    public List<List<string>> GetAllSets() =>
-        [.. parents.GroupBy(x => FindRoot(x.Key), x => x.Key).Select(x => x.OrderBy(x => x).ToList()).OrderBy(x => x[0])];
+    public List<List<string>> GetAllSets(bool mergeSingletons)
+    {
+        return [.. GetAllSetsUnordered().OrderBy(x => x.First())];
+
+        IEnumerable<List<string>> GetAllSetsUnordered()
+        {
+            var singletonsUnion = new List<string>();
+            foreach (var group in parents.GroupBy(x => FindRoot(x.Key), x => x.Key))
+            {
+                if (mergeSingletons && group.Count() == 1)
+                {
+                    singletonsUnion.Add(group.Single());
+                }
+                else
+                {
+                    yield return [.. group.OrderBy(x => x)];
+                }
+            }
+
+            if (singletonsUnion.Any())
+            {
+                yield return [.. singletonsUnion.OrderBy(x => x)];
+            }
+        }
+    }
 }

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

@@ -55,8 +55,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 != null
@@ -65,6 +64,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/src/SonarAnalyzer.Common/Helpers/SymbolHelper.cs

@@ -181,6 +181,9 @@ public static Accessibility GetEffectiveAccessibility(this ISymbol symbol)
         public static bool IsConstructor(this ISymbol symbol) =>
             symbol.Kind == SymbolKind.Method && symbol.Name == ".ctor";
 
+        public static bool IsStaticConstructor(this ISymbol symbol) =>
+            symbol.Kind == SymbolKind.Method && ((IMethodSymbol)symbol).MethodKind == MethodKind.StaticConstructor;
+
         public static bool IsDestructor(this IMethodSymbol method) =>
             method.MethodKind == MethodKind.Destructor;
 

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

@@ -1,4 +1,6 @@
-namespace SonarAnalyzer.Test.Common;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace SonarAnalyzer.Test.Common;
 
 [TestClass]
 public class DisjointSetsTest
@@ -77,6 +79,34 @@ public void GetAllSets_ReturnsSortedSets()
         AssertSets([["1", "3"], ["2"]], sets);
     }
 
-    private static void AssertSets(List<List<string>> expected, DisjointSets sets) =>
-        sets.GetAllSets().Should().BeEquivalentTo(expected, options => options.WithStrictOrdering());
+    [TestMethod]
+    public void GetAllSets_MergesSingletons()
+    {
+        var sets = new DisjointSets(FirstSixPositiveInts);
+        AssertSets([["1", "2", "3", "4", "5", "6"]], sets, mergeSingletons: true);
+        sets.Union("1", "1");
+        AssertSets([["1", "2", "3", "4", "5", "6"]], sets, mergeSingletons: true);
+        sets.Union("1", "2");
+        AssertSets([["1", "2"], ["3", "4", "5", "6"]], sets, mergeSingletons: true);
+        sets.Union("3", "4");
+        AssertSets([["1", "2"], ["3", "4"], ["5", "6"]], sets, mergeSingletons: true);
+        sets.Union("1", "4");
+        AssertSets([["1", "2", "3", "4"], ["5", "6"]], sets, mergeSingletons: true);
+        sets.Union("5", "6");
+        AssertSets([["1", "2", "3", "4"], ["5", "6"]], sets, mergeSingletons: true);
+        sets.Union("4", "5");
+        AssertSets([["1", "2", "3", "4", "5", "6"]], sets, mergeSingletons: true);
+    }
+
+    [TestMethod]
+    public void GetAllSets_ReturnsSortedSetOfMergedSingletons()
+    {
+        var sets = new DisjointSets(FirstSixPositiveInts.Reverse());
+        AssertSets([["1", "2", "3", "4", "5", "6"]], sets, mergeSingletons: true);
+        sets.Union("2", "1");
+        AssertSets([["1", "2"], ["3", "4", "5", "6"]], sets, mergeSingletons: true);
+    }
+
+    private static void AssertSets(List<List<string>> expected, DisjointSets sets, bool mergeSingletons = false) =>
+        sets.GetAllSets(mergeSingletons).Should().BeEquivalentTo(expected, options => options.WithStrictOrdering());
 }