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

@@ -317,6 +317,7 @@
     "S6930",
     "S6931",
     "S6934",
+    "S6960",
     "S6961"
   ]
 }

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

@@ -0,0 +1,171 @@
+/*
+ * 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
+{
+    private const string DiagnosticId = "S6960";
+    private const string MessageFormat = "This controller has multiple responsibilities and could be split into {0} smaller units.";
+
+    private static readonly HashSet<string> ExcludedWellKnownServices =
+    [
+        "ILogger",
+        "IMediator",
+        "IMapper",
+        "IConfiguration",
+        "IBus",
+        "IMessageBus"
+    ];
+
+    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.IsControllerType()
+                        && symbol.GetAttributesWithInherited().Any(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiControllerAttribute)))
+                    {
+                        CheckApiController(symbolStartContext, symbol);
+                    }
+                }, SymbolKind.NamedType);
+            }
+        });
+
+    private static void CheckApiController(SonarSymbolStartAnalysisContext symbolStartContext, INamedTypeSymbol symbol)
+    {
+        var memberNames = RelevantMemberNames(symbol);
+
+        if (memberNames.Count < 2)
+        {
+            return;
+        }
+
+        var dependencies = new ConcurrentStack<Dependency>();
+
+        symbolStartContext.RegisterCodeBlockStartAction<SyntaxKind>(codeBlockStartContext =>
+        {
+            var methodName = codeBlockStartContext.CodeBlock switch
+            {
+                MethodDeclarationSyntax method => method.GetName(),
+                AccessorDeclarationSyntax { Parent.Parent: PropertyDeclarationSyntax property } => property.GetName(),
+                _ => null
+            };
+
+            if (methodName is not null)
+            {
+                codeBlockStartContext.RegisterNodeAction(c =>
+                {
+                    if (c.Node.GetName() is { } dependencyName && memberNames.Contains(dependencyName))
+                    {
+                        dependencies.Push(new(methodName, dependencyName));
+                    }
+                }, SyntaxKind.IdentifierName);
+            }
+        });
+
+        symbolStartContext.RegisterSymbolEndAction(symbolEndContext =>
+        {
+            var parents = memberNames.ToDictionary(x => x, x => x); // Start with singleton sets
+            foreach (var dependency in dependencies)
+            {
+                Union(parents, dependency.From, dependency.To);
+            }
+
+            var disjointSets = DisjointSets(parents);
+            if (disjointSets.Count > 1)
+            {
+                var secondaryLocations = SecondaryLocations(symbol, disjointSets);
+                foreach (var primaryLocation in LocationIdentifiers<ClassDeclarationSyntax>(symbol))
+                {
+                    var diagnostic = Diagnostic.Create(Rule, primaryLocation, secondaryLocations.ToAdditionalLocations(), secondaryLocations.ToProperties(), disjointSets.Count);
+                    symbolEndContext.ReportIssue(CSharpFacade.Instance.GeneratedCodeRecognizer, diagnostic);
+                }
+            }
+        });
+    }
+
+    private static void Union(IDictionary<string, string> parents, string from, string to) =>
+        parents[FindRoot(parents, from)] = FindRoot(parents, to);
+
+    private static string FindRoot(IDictionary<string, string> parents, string element) =>
+        parents[element] is var root && root != element ? FindRoot(parents, root) : root;
+
+    private static List<List<string>> DisjointSets(IDictionary<string, string> parents) =>
+        parents.GroupBy(x => FindRoot(parents, x.Key), x => x.Key).Select(x => x.OrderBy(x => x).ToList()).OrderBy(x => x[0]).ToList();
+
+    private static IEnumerable<Location> LocationIdentifiers<T>(ISymbol symbol) where T : SyntaxNode =>
+        symbol.DeclaringSyntaxReferences.Select(x => x.GetSyntax()).OfType<T>().Select(x => x.GetIdentifier()?.GetLocation());
+
+    private static ImmutableHashSet<string> RelevantMemberNames(INamedTypeSymbol symbol)
+    {
+        var builder = ImmutableHashSet.CreateBuilder<string>();
+        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);
+                    break;
+                // Primary constructor parameters may or may not generate fields, and must be considered
+                case IMethodSymbol method when method.IsPrimaryConstructor():
+                    builder.UnionWith(method.Parameters.Where(IsService).Select(x => x.Name));
+                    break;
+                case IFieldSymbol field when IsService(field) && !field.IsImplicitlyDeclared:
+                    builder.Add(field.Name);
+                    break;
+                case IPropertySymbol property when IsService(property):
+                    builder.Add(property.Name);
+                    break;
+            }
+        }
+
+        return builder.ToImmutable();
+    }
+
+    private static bool IsService(ISymbol symbol) =>
+        symbol.GetSymbolType() is { TypeKind: TypeKind.Interface, Name: var name } && !ExcludedWellKnownServices.Contains(name);
+
+    private static IEnumerable<SecondaryLocation> SecondaryLocations(INamedTypeSymbol controllerSymbol, List<List<string>> sets)
+    {
+        var methods = controllerSymbol.GetMembers().OfType<IMethodSymbol>();
+        return
+            from set in sets.Zip(Enumerable.Range(1, sets.Count), (set, setIndex) => new { Set = set, SetIndex = setIndex })
+            let setIndex = set.SetIndex
+            from memberName in set.Set
+            from method in methods.Where(x => x.Name == memberName)
+            from location in LocationIdentifiers<MethodDeclarationSyntax>(method)
+            select new SecondaryLocation(location, $"Belongs to responsibility #{setIndex}.");
+    }
+
+    private record struct Dependency(string From, string To);
+}

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 && symbol.Name == ".cctor";
+
         public static bool IsDestructor(this IMethodSymbol method) =>
             method.MethodKind == MethodKind.Destructor;
 

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

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

analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveTooManyResponsibilitiesTest.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;
+
+[TestClass]
+public class ControllersHaveTooManyResponsibilitiesTest
+{
+    private readonly VerifierBuilder builder =
+        new VerifierBuilder<ControllersHaveTooManyResponsibilities>();
+
+#if NET
+    private static IEnumerable<MetadataReference> AspNetCoreReferences =>
+    [
+        AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions,
+        AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore,
+        AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures,
+    ];
+
+    [TestMethod]
+    public void ControllersHaveTooManyResponsibilities_CS() =>
+        builder
+            .AddReferences(AspNetCoreReferences)
+            .AddReferences([CoreMetadataReference.SystemComponentModel]) // For IServiceProvider
+            .AddReferences(NuGetMetadataReference.MicrosoftExtensionsDependencyInjectionAbstractions("8.0.1")) // For IServiceProvider extensions
+            .WithBasePath("AspNet")
+            .AddPaths("ControllersHaveTooManyResponsibilities.CSharp12.cs")
+            .WithLanguageVersion(LanguageVersion.CSharp12)
+            .Verify();
+#endif
+}