-
Notifications
You must be signed in to change notification settings - Fork 222
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
New rule S6960 for C#: Controllers should not have too many responsib…
…ilities
- Loading branch information
1 parent
c83dc72
commit e8ca4df
Showing
13 changed files
with
1,628 additions
and
9 deletions.
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -316,6 +316,7 @@ | |
"S6930", | ||
"S6931", | ||
"S6934", | ||
"S6960", | ||
"S6961", | ||
"S6962", | ||
"S6965", | ||
|
192 changes: 192 additions & 0 deletions
192
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveTooManyResponsibilities.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,192 @@ | ||
/* | ||
* 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 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" | ||
]; | ||
|
||
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 memberNames = RelevantMemberNames(symbol); | ||
if (memberNames.Count < 2) | ||
{ | ||
return; | ||
} | ||
var dependencies = new ConcurrentStack<Dependency>(); | ||
symbolStartContext.RegisterCodeBlockStartAction(PopulateDependencies(memberNames, dependencies)); | ||
symbolStartContext.RegisterSymbolEndAction(CalculateAndReportOnResponsibilities(symbol, memberNames, dependencies)); | ||
} | ||
}, SymbolKind.NamedType); | ||
} | ||
}); | ||
|
||
private static Action<SonarCodeBlockStartAnalysisContext<SyntaxKind>> PopulateDependencies(ImmutableHashSet<string> memberNames, ConcurrentStack<Dependency> dependencies) => | ||
codeBlockStartContext => | ||
{ | ||
if (BlockName(codeBlockStartContext.CodeBlock) is { } blockName) | ||
{ | ||
codeBlockStartContext.RegisterNodeAction(c => | ||
{ | ||
if (c.Node.GetName() is { } dependencyName && memberNames.Contains(dependencyName)) | ||
{ | ||
dependencies.Push(new(blockName, dependencyName)); | ||
} | ||
}, SyntaxKind.IdentifierName); | ||
} | ||
}; | ||
|
||
private static Action<SonarSymbolReportingContext> CalculateAndReportOnResponsibilities( | ||
INamedTypeSymbol controllerSymbol, | ||
ImmutableHashSet<string> memberNames, | ||
ConcurrentStack<Dependency> dependencies) => | ||
symbolEndContext => | ||
{ | ||
var dependencySets = new DisjointSets(memberNames); | ||
foreach (var dependency in dependencies) | ||
{ | ||
dependencySets.Union(dependency.From, dependency.To); | ||
} | ||
var responsibilityGroups = dependencySets.GetAllSets(mergeSingletons: true); // Actions with no dependencies don't form new responsibilities | ||
if (responsibilityGroups.Count > 1) | ||
{ | ||
var secondaryLocations = SecondaryLocations(controllerSymbol, responsibilityGroups); | ||
foreach (var primaryLocation in IdentifierLocations<ClassDeclarationSyntax>(controllerSymbol)) | ||
{ | ||
symbolEndContext.ReportIssue(Diagnostic.Create( | ||
Rule, | ||
primaryLocation, | ||
secondaryLocations.ToAdditionalLocations(), | ||
secondaryLocations.ToProperties(), | ||
responsibilityGroups.Count)); | ||
} | ||
} | ||
}; | ||
|
||
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 indexer } => UnspeakableIndexerName, | ||
MethodDeclarationSyntax method => method.GetName(), | ||
PropertyDeclarationSyntax property => property.GetName(), | ||
_ => null | ||
}; | ||
|
||
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; | ||
// 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); | ||
break; | ||
case IPropertySymbol property when IsService(property): | ||
builder.Add(property.Name); | ||
break; | ||
// Indexers are treated as methods with an unspeakable name | ||
case IPropertySymbol { IsIndexer: true } indexer: | ||
builder.Add(UnspeakableIndexerName); | ||
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) | ||
{ | ||
for (var setIndex = 0; setIndex < sets.Count; setIndex++) | ||
{ | ||
foreach (var memberLocation in sets[setIndex].SelectMany(MemberLocations)) | ||
{ | ||
yield return new SecondaryLocation(memberLocation, $"Belongs 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 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); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.