feat: Introduce PreferReadOnlyParameters analyzer

Documentation/Diagnostics/PH2140.md

@@ -0,0 +1,47 @@
+# PH2140: Prefer readonly parameters
+
+| Property | Value  |
+|--|--|
+| Package | [Philips.CodeAnalysis.MaintainabilityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MaintainabilityAnalyzers) |
+| Diagnostic ID | PH2140 |
+| Category  | [Maintainability](../Maintainability.md) |
+| Analyzer | [PreferReadOnlyParametersAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/PreferReadOnlyParametersAnalyzer.cs)
+| CodeFix  | No |
+| Severity | Error |
+| Enabled by default | Yes |
+
+## Introduction
+
+Methods that do not modify a parameter should declare that parameter as read-only. This enabled calling code to use read-only types, which increase readability of the code. Please note this is similar to the `const` keyword in C++.
+
+In C# read-only types are available for Collections (`Array`, `List<>`, `Dictionary` and `HashSet`) and the `Span` class. 
+
+## How to solve
+
+When not modifying the collection, declare the parameter as read-only, using for example `IReadOnlyList<>` iso `List<>`. Alternatively consider the Immutable collections, like [ImmutableArray](https://learn.microsoft.com/en-us/dotnet/api/system.collections.immutable.immutablearray?view=net-7.0).
+
+## Example
+
+Code that triggers a diagnostic:
+``` cs
+using System.Collection.Generic;
+public class BadExample {
+  public static int BadCode(List<string> list) {
+    return list.IndexOf("42");
+  }
+}
+```
+
+And the replacement code:
+``` cs
+using System.Collection.Generic;
+public class GoodExample {
+  public static int GoodCode(IReadOnlyList<string> list) {
+    return list.IndexOf("42");
+  }
+}
+```
+
+## Configuration
+
+This analyzer does not offer any special configuration. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply.

Philips.CodeAnalysis.Common/DataFlowHelper.cs

@@ -0,0 +1,38 @@
+// © 2023 Koninklijke Philips N.V. See License.md in the project root for license information.
+
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace Philips.CodeAnalysis.Common
+{
+	public class DataFlowHelper
+	{
+		public DataFlowAnalysis GetDataFlowAnalysis(SemanticModel semanticModel, BaseMethodDeclarationSyntax methodDeclaration)
+		{
+			DataFlowAnalysis flow;
+			BlockSyntax body = methodDeclaration.Body;
+
+			if (body != null)
+			{
+				if (!body.Statements.Any())
+				{
+					return null;
+				}
+
+				StatementSyntax firstStatement = body.Statements.First();
+				StatementSyntax lastStatement = body.Statements.Last();
+				flow = semanticModel.AnalyzeDataFlow(firstStatement, lastStatement);
+			}
+			else if (methodDeclaration.ExpressionBody != null)
+			{
+				flow = semanticModel.AnalyzeDataFlow(methodDeclaration.ExpressionBody.Expression);
+			}
+			else
+			{
+				return null;
+			}
+
+			return flow;
+		}
+	}
+}

Philips.CodeAnalysis.Common/DiagnosticId.cs

@@ -133,5 +133,6 @@ public enum DiagnosticId
 		RegexNeedsTimeout = 2137,
 		AvoidVoidReturn = 2138,
 		EnableDocumentationCreation = 2139,
+		PreferReadOnlyParameters = 2140,
 	}
 }

Philips.CodeAnalysis.MaintainabilityAnalyzers/Documentation/ExceptionWalker.cs

@@ -143,7 +143,7 @@ private void HandleCallInstruction(Instruction instruction, CallTreeNode node, S
 			}
 		}
 
-		private static void AdjustExceptionFilter(List<ExceptionHandler> catchHandlers, Instruction instruction, Stack<string> filteredExceptions)
+		private static void AdjustExceptionFilter(IReadOnlyList<ExceptionHandler> catchHandlers, Instruction instruction, Stack<string> filteredExceptions)
 		{
 			foreach (ExceptionHandler filter in catchHandlers)
 			{

Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/PreferReadOnlyParametersAnalyzer.cs

@@ -0,0 +1,107 @@
+// © 2023 Koninklijke Philips N.V. See License.md in the project root for license information.
+
+using System.Collections.Generic;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Philips.CodeAnalysis.Common;
+
+namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability
+{
+	[DiagnosticAnalyzer(LanguageNames.CSharp)]
+	public class PreferReadOnlyParametersAnalyzer : SingleDiagnosticAnalyzer<MethodDeclarationSyntax, PreferReadOnlyParametersNodeAction>
+	{
+		private const string Title = @"Prefer readonly parameters";
+		public const string MessageFormat = @"Make parameter '{0}' a readonly type";
+		private const string Description = @"Method parameters that are not written to, can be made readonly";
+		public PreferReadOnlyParametersAnalyzer()
+			: base(DiagnosticId.PreferReadOnlyParameters, Title, MessageFormat, Description, Categories.Maintainability)
+		{ }
+	}
+
+	public class PreferReadOnlyParametersNodeAction : SyntaxNodeAction<MethodDeclarationSyntax>
+	{
+		private static readonly List<string> ReadWriteCollections = new() { "System.Array", "System.Collections.Generic.List", "System.Collections.Generic.IList", "System.Collections.IList", "System.Collections.Generic.Dictionary", "System.Collections.Generic.HashSet", "System.Span" };
+		private static readonly List<string> WriteMethods = new() { "Add", "AddRange", "Clear", "Insert", "InsertRange", "Remove", "RemoveAt", "RemoveRange", "RemoveWhere", "Reverse", "Sort" };
+		private readonly NamespaceIgnoringComparer _comparer = new();
+
+		public override void Analyze()
+		{
+			SeparatedSyntaxList<ParameterSyntax> parameters = Node.ParameterList.Parameters;
+			if (!parameters.Any())
+			{
+				return;
+			}
+
+			IReadOnlyDictionary<string, string> aliases = Helper.GetUsingAliases(Node);
+			IEnumerable<ParameterSyntax> collections = parameters.Where(p => IsReadWriteCollection(p.Type, aliases));
+			if (!collections.Any())
+			{
+				return;
+			}
+			IEnumerable<MemberAccessExpressionSyntax> accesses = Node.DescendantNodes().OfType<MemberAccessExpressionSyntax>();
+			IEnumerable<ElementAccessExpressionSyntax> setters = Node.DescendantNodes()
+				.OfType<AssignmentExpressionSyntax>()
+				.Select(ass => ass.Left as ElementAccessExpressionSyntax);
+			IEnumerable<InvocationExpressionSyntax> invocations = Node.DescendantNodes().OfType<InvocationExpressionSyntax>();
+			foreach (ParameterSyntax collectionParameter in collections)
+			{
+				var parameterName = collectionParameter.Identifier.Text;
+				// Calling member that will change the type...
+				// Or using the setter of the indexer (example: list[8] = 42) ...
+				// Or invocations that require the read-write type
+				if (
+					!accesses.Any(acc => IsCallingParameter(acc.Expression, parameterName) && IsModifyingMember(acc.Name)) &&
+					!setters.Any(element => element != null && IsCallingParameter(element.Expression, parameterName)) &&
+					!invocations.Any(voc => RequiresReadWrite(voc, parameterName)))
+				{
+					Location location = collectionParameter.Type.GetLocation();
+					ReportDiagnostic(location, collectionParameter.Identifier.Text);
+				}
+			}
+		}
+
+		private bool IsReadWriteCollection(TypeSyntax type, IReadOnlyDictionary<string, string> aliases)
+		{
+			var fullName = type?.GetFullName(aliases);
+			return ReadWriteCollections.Exists(col => _comparer.Compare(fullName, col) == 0);
+		}
+
+		private bool IsCallingParameter(ExpressionSyntax expression, string name)
+		{
+			return expression is IdentifierNameSyntax identifierName && _comparer.Compare(identifierName.Identifier.Text, name) == 0;
+		}
+
+		private static bool IsModifyingMember(SimpleNameSyntax name)
+		{
+			var nameString = name.Identifier.Text;
+			return WriteMethods.Exists(method => string.CompareOrdinal(nameString, method) == 0);
+		}
+
+		private bool RequiresReadWrite(InvocationExpressionSyntax invocation, string parameterName)
+		{
+			IEnumerable<ArgumentSyntax> arguments = invocation.ArgumentList.Arguments.Where(arg => IsCallingParameter(arg.Expression, parameterName));
+			if (!arguments.Any())
+			{
+				return false;
+			}
+			var needsReadWrite = false;
+			ISymbol symbol = Context.SemanticModel.GetSymbolInfo(invocation).Symbol;
+			foreach (ArgumentSyntax argument in arguments)
+			{
+				if (symbol != null)
+				{
+					needsReadWrite |= !IsReadOnly(symbol);
+				}
+			}
+			return needsReadWrite;
+		}
+
+		private bool IsReadOnly(ISymbol symbol)
+		{
+			var symbolName = symbol.Name.ToLowerInvariant();
+			return symbolName.Contains("readonly") || _comparer.Compare(symbolName, "System.Collections.Generic.IEnumerable") == 0;
+		}
+	}
+}

Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/PassByRefAnalyzer.cs

@@ -106,23 +106,9 @@ private bool IsWrittenTo(ParameterSyntax parameterSyntax)
 				return true;
 			}
 
-			DataFlowAnalysis flow;
-			if (Node.Body != null)
-			{
-				if (!Node.Body.Statements.Any())
-				{
-					return false;
-				}
-
-				StatementSyntax firstStatement = Node.Body.Statements.First();
-				StatementSyntax lastStatement = Node.Body.Statements.Last();
-				flow = semanticModel.AnalyzeDataFlow(firstStatement, lastStatement);
-			}
-			else if (Node.ExpressionBody != null)
-			{
-				flow = semanticModel.AnalyzeDataFlow(Node.ExpressionBody.Expression);
-			}
-			else
+			DataFlowHelper dfHelper = new();
+			DataFlowAnalysis flow = dfHelper.GetDataFlowAnalysis(Context.SemanticModel, Node);
+			if (flow is null)
 			{
 				// No usage found.
 				return false;

Philips.CodeAnalysis.MaintainabilityAnalyzers/Philips.CodeAnalysis.MaintainabilityAnalyzers.csproj

@@ -121,6 +121,7 @@
       * Introduce PH2135: Match namespace and Assembly Name. This extends the functionality that was previously claimed by PH2006 but never enforced.
 	    * Introduce PH2136: Avoid duplicate strings.
       * Introduce PH2139: Enable documentation creation.
+	  * Introduce PH2140: Prefer readonly parameters.
     </PackageReleaseNotes>
     <Copyright>© 2019-2023 Koninklijke Philips N.V.</Copyright>
     <PackageTags>CSharp Maintainability Roslyn CodeAnalysis analyzers Philips</PackageTags>

Philips.CodeAnalysis.MaintainabilityAnalyzers/Philips.CodeAnalysis.MaintainabilityAnalyzers.md

@@ -89,3 +89,4 @@
 | [PH2136](../Documentation/Diagnostics/PH2136.md)  | Avoid duplicate strings                      | Duplicate strings are less maintainable. |
 | [PH2138](../Documentation/Diagnostics/PH2138.md)  | Avoid void returns                           | Methods that return void are mysterious. |
 | [PH2139](../Documentation/Diagnostics/PH2139.md)  | Enable documentation creation                | Add XML documentation generation to the project file, to be able to see Diagnostics for XML documentation. |
+| [PH2140](../Documentation/Diagnostics/PH2140.md)  | Prefer readonly parameters                   | Method parameters that are not written to, can be made readonly. |

Philips.CodeAnalysis.MsTestAnalyzers/NoEmptyTestMethodsDiagnosticAnalyzer.cs

@@ -28,7 +28,7 @@ protected override Implementation OnInitializeAnalyzer(AnalyzerOptions options,
 
 		public class NoEmptyTestMethodsImplementation : Implementation
 		{
-			public override void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, HashSet<INamedTypeSymbol> presentAttributes)
+			public override void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, IReadOnlyCollection<INamedTypeSymbol> presentAttributes)
 			{
 				if (methodDeclaration.Body == null)
 				{

Philips.CodeAnalysis.MsTestAnalyzers/TestAttributeDiagnosticAnalyzer.cs

@@ -14,7 +14,7 @@ public abstract class TestAttributeDiagnosticAnalyzer : DiagnosticAnalyzer
 	{
 		public abstract class Implementation
 		{
-			public virtual void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, HashSet<INamedTypeSymbol> presentAttributes) { }
+			public virtual void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, IReadOnlyCollection<INamedTypeSymbol> presentAttributes) { }
 		}
 
 		protected TestAttributeDiagnosticAnalyzer()

Philips.CodeAnalysis.MsTestAnalyzers/TestMethodDiagnosticAnalyzer.cs

@@ -46,7 +46,7 @@ protected TestMethodImplementation(MsTestAttributeDefinitions definitions, Attri
 
 			protected abstract void OnTestMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, bool isDataTestMethod);
 
-			public sealed override void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, HashSet<INamedTypeSymbol> presentAttributes)
+			public sealed override void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, IReadOnlyCollection<INamedTypeSymbol> presentAttributes)
 			{
 				var isTestMethod = false;
 				var isDataTestMethod = false;

Philips.CodeAnalysis.MsTestAnalyzers/TestMethodsMustBeInTestClassAnalyzer.cs

@@ -33,7 +33,7 @@ protected override Implementation OnInitializeAnalyzer(AnalyzerOptions options,
 
 		private sealed class TestMethodsMustBeInTestClass : Implementation
 		{
-			public override void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, HashSet<INamedTypeSymbol> presentAttributes)
+			public override void OnTestAttributeMethod(SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDeclaration, IMethodSymbol methodSymbol, IReadOnlyCollection<INamedTypeSymbol> presentAttributes)
 			{
 				TestHelper testHelper = new();
 				if (testHelper.IsInTestClass(context))

Philips.CodeAnalysis.Test/Common/AdditionalFilesHelperTest.cs

@@ -100,12 +100,12 @@ public void InitializeExceptionsShouldReturnSame()
 			CollectionAssert.AreEqual(content, actual.ToArray());
 		}
 
-		private static AnalyzerOptions CreateOptions(Dictionary<string, string> settings)
+		private static AnalyzerOptions CreateOptions(IReadOnlyDictionary<string, string> settings)
 		{
 			return new AnalyzerOptions(ImmutableArray<AdditionalText>.Empty, new TestAnalyzerConfigOptionsProvider(settings));
 		}
 
-		private static AnalyzerOptions CreateOptions(Dictionary<string, string> settings,
+		private static AnalyzerOptions CreateOptions(IReadOnlyDictionary<string, string> settings,
 			ImmutableArray<AdditionalText> additionalTexts)
 		{
 			return new AnalyzerOptions(additionalTexts, new TestAnalyzerConfigOptionsProvider(settings));

Philips.CodeAnalysis.Test/Helpers/TestAnalyzerConfigOptionsProvider.cs

@@ -9,9 +9,9 @@ namespace Philips.CodeAnalysis.Test.Helpers
 {
 	internal sealed class TestAnalyzerConfigOptions : AnalyzerConfigOptions
 	{
-		private readonly Dictionary<string, string> _settings;
+		private readonly IReadOnlyDictionary<string, string> _settings;
 
-		public TestAnalyzerConfigOptions(Dictionary<string, string> settings)
+		public TestAnalyzerConfigOptions(IReadOnlyDictionary<string, string> settings)
 		{
 			_settings = settings ?? new Dictionary<string, string>();
 		}
@@ -26,7 +26,7 @@ internal sealed class TestAnalyzerConfigOptionsProvider : AnalyzerConfigOptionsP
 	{
 		private readonly TestAnalyzerConfigOptions _options;
 
-		public TestAnalyzerConfigOptionsProvider(Dictionary<string, string> settings)
+		public TestAnalyzerConfigOptionsProvider(IReadOnlyDictionary<string, string> settings)
 		{
 			_options = new TestAnalyzerConfigOptions(settings);
 		}