diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S2328.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S2328.json index 97ad40d2d28..66b9ecd19ec 100644 --- a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S2328.json +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S2328.json @@ -2,8 +2,18 @@ "issues": [ { "id": "S2328", -"message": "Remove this use of '_hashCode' from the 'GetHashCode' declaration, or make it 'readonly'.", -"location": { +"message": " Refactor 'GetHashCode' to not reference mutable fields.", +"location": [ +{ +"uri": "Nancy\src\Nancy\TinyIoc\TinyIoC.cs", +"region": { +"startLine": 3055, +"startColumn": 33, +"endLine": 3055, +"endColumn": 44 +} +}, +{ "uri": "Nancy\src\Nancy\TinyIoc\TinyIoC.cs", "region": { "startLine": 3057, @@ -12,6 +22,7 @@ "endColumn": 33 } } +] } ] } diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Tests-{776D244D-BC4D-4BBB-A478-CAF2D04520E1}-S2328.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Tests-{776D244D-BC4D-4BBB-A478-CAF2D04520E1}-S2328.json index e9d5c36b73b..209b60154a1 100644 --- a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Tests-{776D244D-BC4D-4BBB-A478-CAF2D04520E1}-S2328.json +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Tests-{776D244D-BC4D-4BBB-A478-CAF2D04520E1}-S2328.json @@ -2,8 +2,18 @@ "issues": [ { "id": "S2328", -"message": "Remove this use of 'Data' from the 'GetHashCode' declaration, or make it 'readonly'.", -"location": { +"message": " Refactor 'GetHashCode' to not reference mutable fields.", +"location": [ +{ +"uri": "Nancy\src\Nancy.Tests\Unit\Json\TestConverterType.cs", +"region": { +"startLine": 22, +"startColumn": 23, +"endLine": 22, +"endColumn": 34 +} +}, +{ "uri": "Nancy\src\Nancy.Tests\Unit\Json\TestConverterType.cs", "region": { "startLine": 24, @@ -12,11 +22,22 @@ "endColumn": 20 } } +] }, { "id": "S2328", -"message": "Remove this use of 'ConverterData' from the 'GetHashCode' declaration, or make it 'readonly'.", -"location": { +"message": " Refactor 'GetHashCode' to not reference mutable fields.", +"location": [ +{ +"uri": "Nancy\src\Nancy.Tests\Unit\Json\TestData.cs", +"region": { +"startLine": 31, +"startColumn": 23, +"endLine": 31, +"endColumn": 34 +} +}, +{ "uri": "Nancy\src\Nancy.Tests\Unit\Json\TestData.cs", "region": { "startLine": 33, @@ -24,12 +45,8 @@ "endLine": 33, "endColumn": 24 } -} }, { -"id": "S2328", -"message": "Remove this use of 'PrimitiveConverterData' from the 'GetHashCode' declaration, or make it 'readonly'.", -"location": { "uri": "Nancy\src\Nancy.Tests\Unit\Json\TestData.cs", "region": { "startLine": 33, @@ -38,11 +55,22 @@ "endColumn": 63 } } +] }, { "id": "S2328", -"message": "Remove this use of 'Data' from the 'GetHashCode' declaration, or make it 'readonly'.", -"location": { +"message": " Refactor 'GetHashCode' to not reference mutable fields.", +"location": [ +{ +"uri": "Nancy\src\Nancy.Tests\Unit\Json\TestPrimitiveConverterType.cs", +"region": { +"startLine": 22, +"startColumn": 23, +"endLine": 22, +"endColumn": 34 +} +}, +{ "uri": "Nancy\src\Nancy.Tests\Unit\Json\TestPrimitiveConverterType.cs", "region": { "startLine": 24, @@ -51,6 +79,7 @@ "endColumn": 20 } } +] } ] } diff --git a/sonaranalyzer-dotnet/its/expected/akka.net/Akka-{5DEDDF90-37F0-48D3-A0B0-A5CBD8A7E377}-S2328.json b/sonaranalyzer-dotnet/its/expected/akka.net/Akka-{5DEDDF90-37F0-48D3-A0B0-A5CBD8A7E377}-S2328.json index e74e8d85914..d90e3c0e0a8 100644 --- a/sonaranalyzer-dotnet/its/expected/akka.net/Akka-{5DEDDF90-37F0-48D3-A0B0-A5CBD8A7E377}-S2328.json +++ b/sonaranalyzer-dotnet/its/expected/akka.net/Akka-{5DEDDF90-37F0-48D3-A0B0-A5CBD8A7E377}-S2328.json @@ -2,8 +2,18 @@ "issues": [ { "id": "S2328", -"message": "Remove this use of 'inputType' from the 'GetHashCode' declaration, or make it 'readonly'.", -"location": { +"message": " Refactor 'GetHashCode' to not reference mutable fields.", +"location": [ +{ +"uri": "akka.net\src\core\Akka\Actor\Props.cs", +"region": { +"startLine": 111, +"startColumn": 29, +"endLine": 111, +"endColumn": 40 +} +}, +{ "uri": "akka.net\src\core\Akka\Actor\Props.cs", "region": { "startLine": 118, @@ -12,6 +22,7 @@ "endColumn": 55 } } +] } ] } diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/KnownMethods.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/KnownMethods.cs index 1520fa2130c..bae110b96ae 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/KnownMethods.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/KnownMethods.cs @@ -50,7 +50,8 @@ public static bool IsObjectEquals(this IMethodSymbol methodSymbol) public static bool IsObjectGetHashCode(this IMethodSymbol methodSymbol) { - return methodSymbol.IsOverride && + return methodSymbol != null && + methodSymbol.IsOverride && methodSymbol.MethodKind == MethodKind.Ordinary && methodSymbol.Name == nameof(object.GetHashCode) && methodSymbol.Parameters.Length == 0 && diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutable.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutable.cs index 2b7dfc06cad..60d95c19ea6 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutable.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutable.cs @@ -18,6 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; @@ -26,7 +27,6 @@ using Microsoft.CodeAnalysis.Diagnostics; using SonarAnalyzer.Common; using SonarAnalyzer.Helpers; -using System.Collections.Generic; namespace SonarAnalyzer.Rules.CSharp { @@ -35,10 +35,11 @@ namespace SonarAnalyzer.Rules.CSharp public class GetHashCodeMutable : SonarDiagnosticAnalyzer { internal const string DiagnosticId = "S2328"; - private const string MessageFormat = "Remove this use of '{0}' from the 'GetHashCode' declaration, or make it 'readonly'."; + private const string IssueMessage = " Refactor 'GetHashCode' to not reference mutable fields."; + private const string SecondaryMessageFormat = "Remove this use of '{0}' or make it 'readonly'."; private static readonly DiagnosticDescriptor rule = - DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager); + DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, IssueMessage, RspecStrings.ResourceManager); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(rule); @@ -50,9 +51,7 @@ protected sealed override void Initialize(SonarAnalysisContext context) var methodSyntax = (MethodDeclarationSyntax)c.Node; var methodSymbol = c.SemanticModel.GetDeclaredSymbol(methodSyntax); - if (methodSymbol == null || - methodSyntax.Identifier.Text != "GetHashCode" || - !methodSyntax.Modifiers.Any(SyntaxKind.OverrideKeyword)) + if (!methodSymbol.IsObjectGetHashCode()) { return; } @@ -74,15 +73,25 @@ protected sealed override void Initialize(SonarAnalysisContext context) .Where(symbol => symbol != null) .ToImmutableHashSet(); - var identifiers = methodSyntax.DescendantNodes() - .OfType(); + var identifiers = methodSyntax.DescendantNodes().OfType(); + + var secondaryLocations = GetAllFirstMutableFieldsUsed(c, fieldsOfClass, identifiers) + .Select(identifierSyntax => new SecondaryLocation(identifierSyntax.GetLocation(), + string.Format(SecondaryMessageFormat, identifierSyntax.Identifier.Text))) + .ToList(); + if (secondaryLocations.Count == 0) + { + return; + } - ReportOnFirstReferences(c, fieldsOfClass, identifiers); + c.ReportDiagnostic(Diagnostic.Create(rule, methodSyntax.Identifier.GetLocation(), + additionalLocations: secondaryLocations.ToAdditionalLocations(), + properties: secondaryLocations.ToProperties())); }, SyntaxKind.MethodDeclaration); } - private static void ReportOnFirstReferences(SyntaxNodeAnalysisContext context, + private static IEnumerable GetAllFirstMutableFieldsUsed(SyntaxNodeAnalysisContext context, ImmutableHashSet fieldsOfClass, IEnumerable identifiers) { var syntaxNodes = new Dictionary>(); @@ -108,12 +117,9 @@ private static void ReportOnFirstReferences(SyntaxNodeAnalysisContext context, syntaxNodes[identifierSymbol].Add(identifier); } - foreach (var identifierReferences in syntaxNodes.Values) - { - var firstPosition = identifierReferences.Select(id => id.SpanStart).Min(); - var identifier = identifierReferences.First(id => id.SpanStart == firstPosition); - context.ReportDiagnostic(Diagnostic.Create(rule, identifier.GetLocation(), identifier.Identifier.Text)); - } + return syntaxNodes.Values + .Select(identifierReferences => identifierReferences.OrderBy(id => id.SpanStart).FirstOrDefault()) + .Where(identifierSyntax => identifierSyntax != null); } private static bool IsFieldRelevant(IFieldSymbol fieldSymbol, ImmutableHashSet fieldsOfClass) diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutableCodeFixProvider.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutableCodeFixProvider.cs index df207e466d1..23023e8ed49 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutableCodeFixProvider.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutableCodeFixProvider.cs @@ -18,14 +18,17 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using System.Collections.Generic; using System.Collections.Immutable; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CodeFixes; -using System.Threading.Tasks; using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; -using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; using SonarAnalyzer.Helpers; namespace SonarAnalyzer.Rules.CSharp @@ -49,41 +52,70 @@ public sealed override FixAllProvider GetFixAllProvider() protected sealed override async Task RegisterCodeFixesAsync(SyntaxNode root, CodeFixContext context) { var diagnostic = context.Diagnostics.First(); - var diagnosticSpan = diagnostic.Location.SourceSpan; - var identifierName = root.FindNode(diagnosticSpan, getInnermostNodeForTie: true) as IdentifierNameSyntax; - if (identifierName == null) + + var identifiersToFix = diagnostic.AdditionalLocations + .Select(location => location.SourceSpan) + .Select(diagnosticSpan => root.FindNode(diagnosticSpan, getInnermostNodeForTie: true) as IdentifierNameSyntax) + .Where(idenfitierName => idenfitierName != null) + .ToList(); + + if (identifiersToFix.Count == 0) { return; } - var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + var semanticModel = await context.Document + .GetSemanticModelAsync(context.CancellationToken) + .ConfigureAwait(false); + var allFieldDeclarationTasks = identifiersToFix.Select(identifier => + GetFieldDeclarationSyntax(semanticModel, identifier, context.CancellationToken)); + var allFieldDeclarations = await Task.WhenAll(allFieldDeclarationTasks).ConfigureAwait(false); + allFieldDeclarations = allFieldDeclarations.Where(fieldDeclaration => fieldDeclaration != null).ToArray(); + + context.RegisterCodeFix( + CodeAction.Create( + Title, + token => AddReadonlyToFieldDeclarations(context.Document, token, allFieldDeclarations)), + context.Diagnostics); + } + + private async Task GetFieldDeclarationSyntax(SemanticModel semanticModel, + IdentifierNameSyntax identifierName, CancellationToken cancellationToken) + { var fieldSymbol = semanticModel.GetSymbolInfo(identifierName).Symbol as IFieldSymbol; if (fieldSymbol == null || !fieldSymbol.DeclaringSyntaxReferences.Any()) { - return; + return null; } - var reference = await fieldSymbol.DeclaringSyntaxReferences.First().GetSyntaxAsync(context.CancellationToken).ConfigureAwait(false); + var reference = await fieldSymbol.DeclaringSyntaxReferences.First() + .GetSyntaxAsync(cancellationToken) + .ConfigureAwait(false); var fieldDeclaration = (FieldDeclarationSyntax)reference.Parent.Parent; if (fieldDeclaration.Declaration.Variables.Count != 1) { - return; + return null; } - context.RegisterCodeFix( - CodeAction.Create( - Title, - c => - { - var newFieldDeclaration = fieldDeclaration.AddModifiers( - SyntaxFactory.Token(SyntaxKind.ReadOnlyKeyword)); - var newRoot = root.ReplaceNode(fieldDeclaration, newFieldDeclaration); - return Task.FromResult(context.Document.WithSyntaxRoot(newRoot)); - }), - context.Diagnostics); + return fieldDeclaration; + } + + private async Task AddReadonlyToFieldDeclarations(Document document, CancellationToken cancellationToken, + IEnumerable fieldDeclarations) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken); + + foreach (var fieldDeclaration in fieldDeclarations) + { + var readonlyToken = SyntaxFactory.Token(SyntaxKind.ReadOnlyKeyword).WithTrailingTrivia(SyntaxFactory.Space); + var newFieldDeclaration = fieldDeclaration.AddModifiers(readonlyToken); + editor.ReplaceNode(fieldDeclaration, newFieldDeclaration); + } + + return editor.GetChangedDocument(); } } } diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.Fixed.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.Fixed.cs index d661fb99ec7..faedfacfe5a 100644 --- a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.Fixed.cs +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.Fixed.cs @@ -21,15 +21,15 @@ public GetHashCodeMutable() { } - public override int GetHashCode() + public override int GetHashCode() // Fixed { int hash = Zero; - hash += foo.GetHashCode(); // Fixed - hash += age.GetHashCode(); // Fixed - hash += this.name.GetHashCode(); // Fixed + hash += foo.GetHashCode(); + hash += age.GetHashCode(); + hash += this.name.GetHashCode(); hash += name.GetHashCode(); // Compliant, we already reported on this symbol hash += this.birthday.GetHashCode(); - hash += SomeMethod(Field); // Fixed + hash += SomeMethod(Field); return hash; } public int SomeMethod(int value) @@ -48,4 +48,4 @@ public override int GetHashCode() return i; // we don't report on this } } -} +} \ No newline at end of file diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.cs index 2de68a69d05..c50ea0aa4a1 100644 --- a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.cs +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/GetHashCodeMutable.cs @@ -21,16 +21,16 @@ public GetHashCodeMutable() { } - public override int GetHashCode() + public override int GetHashCode() // Noncompliant {{ Refactor 'GetHashCode' to not reference mutable fields.}} { int hash = Zero; - hash += foo.GetHashCode(); // Noncompliant {{Remove this use of 'foo' from the 'GetHashCode' declaration, or make it 'readonly'.}} + hash += foo.GetHashCode(); // Secondary {{Remove this use of 'foo' or make it 'readonly'.}} // ^^^ - hash += age.GetHashCode(); // Noncompliant - hash += this.name.GetHashCode(); // Noncompliant + hash += age.GetHashCode(); // Secondary {{Remove this use of 'age' or make it 'readonly'.}} + hash += this.name.GetHashCode(); // Secondary {{Remove this use of 'name' or make it 'readonly'.}} hash += name.GetHashCode(); // Compliant, we already reported on this symbol hash += this.birthday.GetHashCode(); - hash += SomeMethod(Field); // Noncompliant + hash += SomeMethod(Field); // Secondary {{Remove this use of 'Field' or make it 'readonly'.}} return hash; } public int SomeMethod(int value) @@ -49,4 +49,4 @@ public override int GetHashCode() return i; // we don't report on this } } -} +} \ No newline at end of file diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestFramework/IssueLocationCollector.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestFramework/IssueLocationCollector.cs index 68ebc0b1aa5..fac01e7d24b 100644 --- a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestFramework/IssueLocationCollector.cs +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestFramework/IssueLocationCollector.cs @@ -18,14 +18,14 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -using FluentAssertions.Execution; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.Text; using System; using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Text.RegularExpressions; +using FluentAssertions.Execution; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Text; namespace SonarAnalyzer.UnitTest.TestFramework { @@ -39,7 +39,7 @@ public class IssueLocationCollector private const string NO_PRECISE_LOCATION_PATTERN = @"\s*(?\/\/|\')"; - private const string ISSUE_LOCATION_PATTERN = + internal const string ISSUE_LOCATION_PATTERN = COMMENT_PATTERN + "*" + NO_PRECISE_LOCATION_PATTERN + TYPE_PATTERN + OFFSET_PATTERN + ISSUE_IDS_PATTERN + MESSAGE_PATTERN; private const string PRECISE_ISSUE_LOCATION_PATTERN = @"^" + COMMENT_PATTERN + PRECISE_LOCATION_PATTERN + TYPE_PATTERN + "*" + OFFSET_PATTERN + ISSUE_IDS_PATTERN + MESSAGE_PATTERN + "$"; diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Verifier.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Verifier.cs index 1202499b1bc..9e8dbd0c42b 100644 --- a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Verifier.cs +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Verifier.cs @@ -300,12 +300,9 @@ private static Project AddDocument(this Project project, FileInfo file, if (removeAnalysisComments) { - lines = lines.Where(IssueLocationCollector.IsNotIssueLocationLine).ToArray(); - lines = lines.Select(l => - { - var match = Regex.Match(l, NONCOMPLIANT_PATTERN); - return match.Success ? l.Replace(match.Groups[0].Value, FIXED_MESSAGE) : l; - }).ToArray(); + lines = lines.Where(IssueLocationCollector.IsNotIssueLocationLine) + .Select(ReplaceNonCompliantComment) + .ToArray(); } var text = string.Join(Environment.NewLine, lines); @@ -313,6 +310,23 @@ private static Project AddDocument(this Project project, FileInfo file, return project.AddDocument(file.Name, text).Project; } + private static string ReplaceNonCompliantComment(string line) + { + var match = Regex.Match(line, IssueLocationCollector.ISSUE_LOCATION_PATTERN); + if (!match.Success) + { + return line; + } + + if (match.Groups["issueType"].Value == "Noncompliant") + { + var startIndex = line.IndexOf(match.Groups["issueType"].Value); + return string.Concat(line.Remove(startIndex), FIXED_MESSAGE); + } + + return line.Replace(match.Value, string.Empty).TrimEnd(); + } + #endregion #region Analyzer helpers