diff --git a/Documentation/Diagnostics/PH2164.md b/Documentation/Diagnostics/PH2164.md new file mode 100644 index 000000000..2f7f2cf92 --- /dev/null +++ b/Documentation/Diagnostics/PH2164.md @@ -0,0 +1,73 @@ +# PH2164: String-keyed collections should specify a StringComparer + +| Property | Value | +|--|--| +| Package | [Philips.CodeAnalysis.MaintainabilityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MaintainabilityAnalyzers) | +| Diagnostic ID | PH2164 | +| Category | [Maintainability](../Maintainability.md) | +| Analyzer | [StringDictionaryNeedsComparerAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs) +| CodeFix | No | +| Severity | Error | +| Enabled By Default | Yes | + +## Introduction + +String-keyed collections should explicitly specify a StringComparer (or IComparer for sorted collections) to avoid culture-dependent or case-sensitivity surprises. Without an explicit comparer, collections may use culture-sensitive string comparisons that can lead to unexpected behavior in different locales. + +## How to solve + +Explicitly specify a StringComparer when creating string-keyed collections: + +- For equality-based collections: Use `StringComparer.Ordinal`, `StringComparer.OrdinalIgnoreCase`, or other appropriate StringComparer options +- For sorted collections: Use `StringComparer.Ordinal` (cast to IComparer) or other appropriate comparer + +## Example + +Code that triggers a diagnostic: +``` cs +class BadExample +{ + public void BadMethod() + { + // Missing StringComparer - uses default culture-sensitive comparison + var dict = new Dictionary(); + var set = new HashSet(); + + // Also problematic for sorted collections + var sortedDict = new SortedDictionary(); + var sortedSet = new SortedSet(); + } +} +``` + +And the improved replacement: +``` cs +class GoodExample +{ + public void GoodMethod() + { + // Explicit StringComparer for predictable behavior + var dict = new Dictionary(StringComparer.Ordinal); + var set = new HashSet(StringComparer.OrdinalIgnoreCase); + + // Explicit comparer for sorted collections + var sortedDict = new SortedDictionary(StringComparer.Ordinal); + var sortedSet = new SortedSet(StringComparer.OrdinalIgnoreCase); + } +} +``` + +## Supported Collections + +The analyzer detects the following string-keyed collection types: + +- `Dictionary` +- `HashSet` +- `ConcurrentDictionary` (planned) +- `SortedDictionary` (planned) +- `SortedSet` (planned) +- `ImmutableDictionary` (planned) + +## Configuration + +This analyzer is enabled by default. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply. \ No newline at end of file diff --git a/Philips.CodeAnalysis.Common/DiagnosticId.cs b/Philips.CodeAnalysis.Common/DiagnosticId.cs index 60aa34ec8..2106e4ad5 100644 --- a/Philips.CodeAnalysis.Common/DiagnosticId.cs +++ b/Philips.CodeAnalysis.Common/DiagnosticId.cs @@ -145,5 +145,6 @@ public enum DiagnosticId AvoidUnlicensedPackages = 2155, AvoidPkcsPaddingWithRsaEncryption = 2158, AvoidUnnecessaryAttributeParentheses = 2159, + StringDictionaryNeedsComparer = 2164, } } diff --git a/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs b/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs new file mode 100644 index 000000000..2de48d90b --- /dev/null +++ b/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs @@ -0,0 +1,236 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using Philips.CodeAnalysis.Common; + +namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class StringDictionaryNeedsComparerAnalyzer : DiagnosticAnalyzerBase + { + private const string Title = @"String-keyed collections should specify a StringComparer"; + public const string MessageFormat = @"{0} with a string key should explicitly specify a {1}"; + private const string Description = @"String-keyed collections should explicitly specify a StringComparer (or IComparer for sorted collections) to avoid culture-dependent or case-sensitivity surprises."; + private const string Category = Categories.Maintainability; + + private static readonly DiagnosticDescriptor Rule = new(DiagnosticId.StringDictionaryNeedsComparer.ToId(), Title, MessageFormat, Category, DiagnosticSeverity.Error, isEnabledByDefault: true, description: Description); + + public override ImmutableArray SupportedDiagnostics { get { return ImmutableArray.Create(Rule); } } + + protected override void InitializeCompilation(CompilationStartAnalysisContext context) + { + var wellKnownTypes = new WellKnownTypes(context.Compilation); + + // Debug: Only proceed if we have the basic types + if (wellKnownTypes.String == null || wellKnownTypes.Dictionary_T2 == null) + { + return; + } + + context.RegisterOperationAction(ctx => AnalyzeObjectCreation(ctx, wellKnownTypes), OperationKind.ObjectCreation); + context.RegisterOperationAction(ctx => AnalyzeInvocation(ctx, wellKnownTypes), OperationKind.Invocation); + } + + private static void AnalyzeObjectCreation(OperationAnalysisContext context, WellKnownTypes types) + { + var creation = (IObjectCreationOperation)context.Operation; + if (creation.Type is not INamedTypeSymbol constructed) + { + return; + } + + // Determine if this is one of the supported collection types with string key + if (!TryGetCollectionInfo(constructed, types, out _, out RequiredComparer requiresComparerType)) + { + return; + } + + // If a comparer argument is already provided, nothing to do + IMethodSymbol ctor = creation.Constructor; + if (ctor == null) + { + return; + } + + if (HasRequiredComparerParameter(ctor, requiresComparerType, types)) + { + return; // explicit comparer provided + } + + // No comparer in the selected overload → report + var collectionDisplay = constructed.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat); + var comparerDisplay = requiresComparerType == RequiredComparer.IEqualityComparer + ? "StringComparer/IEqualityComparer" + : "StringComparer/IComparer"; + + context.ReportDiagnostic(Diagnostic.Create( + Rule, + creation.Syntax.GetLocation(), + collectionDisplay, + comparerDisplay)); + } + + private static void AnalyzeInvocation(OperationAnalysisContext context, WellKnownTypes types) + { + var invocation = (IInvocationOperation)context.Operation; + IMethodSymbol targetMethod = invocation.TargetMethod; + + // ImmutableDictionary.Create(...) without comparer + if (types.ImmutableDictionary != null && targetMethod.ContainingType.Equals(types.ImmutableDictionary, SymbolEqualityComparer.Default) && + targetMethod.Name is "Create" or "CreateRange") + { + ImmutableArray typeArgs = targetMethod.TypeArguments; + if (typeArgs.Length == 2 && SymbolEqualityComparer.Default.Equals(typeArgs[0], types.String) && + !HasRequiredComparerParameter(targetMethod, RequiredComparer.IEqualityComparer, types)) + { + var collectionDisplay = $"ImmutableDictionary<{typeArgs[0].ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat)}, {typeArgs[1].ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat)}>"; + context.ReportDiagnostic(Diagnostic.Create( + Rule, + invocation.Syntax.GetLocation(), + collectionDisplay, + "StringComparer/IEqualityComparer")); + } + } + } + + + private enum RequiredComparer { IEqualityComparer, IComparer } + + private static bool TryGetCollectionInfo(INamedTypeSymbol constructed, WellKnownTypes types, out string kind, out RequiredComparer requires) + { + kind = default; + requires = default; + + // HashSet + if (types.HashSet_T != null && constructed.ConstructedFrom.Equals(types.HashSet_T, SymbolEqualityComparer.Default)) + { + if (constructed.TypeArguments.Length == 1 && SymbolEqualityComparer.Default.Equals(constructed.TypeArguments[0], types.String)) + { + kind = "HashSet"; + requires = RequiredComparer.IEqualityComparer; + return true; + } + return false; + } + + // Dictionary + if (types.Dictionary_T2 != null && constructed.ConstructedFrom.Equals(types.Dictionary_T2, SymbolEqualityComparer.Default)) + { + if (constructed.TypeArguments.Length == 2 && SymbolEqualityComparer.Default.Equals(constructed.TypeArguments[0], types.String)) + { + kind = "Dictionary"; + requires = RequiredComparer.IEqualityComparer; + return true; + } + return false; + } + + // ConcurrentDictionary + if (types.ConcurrentDictionary_T2 != null && constructed.ConstructedFrom.Equals(types.ConcurrentDictionary_T2, SymbolEqualityComparer.Default)) + { + if (constructed.TypeArguments.Length == 2 && SymbolEqualityComparer.Default.Equals(constructed.TypeArguments[0], types.String)) + { + kind = "ConcurrentDictionary"; + requires = RequiredComparer.IEqualityComparer; + return true; + } + return false; + } + + // SortedDictionary + if (types.SortedDictionary_T2 != null && constructed.ConstructedFrom.Equals(types.SortedDictionary_T2, SymbolEqualityComparer.Default)) + { + if (constructed.TypeArguments.Length == 2 && SymbolEqualityComparer.Default.Equals(constructed.TypeArguments[0], types.String)) + { + kind = "SortedDictionary"; + requires = RequiredComparer.IComparer; + return true; + } + return false; + } + + // SortedSet + if (types.SortedSet_T != null && constructed.ConstructedFrom.Equals(types.SortedSet_T, SymbolEqualityComparer.Default)) + { + if (constructed.TypeArguments.Length == 1 && SymbolEqualityComparer.Default.Equals(constructed.TypeArguments[0], types.String)) + { + kind = "SortedSet"; + requires = RequiredComparer.IComparer; + return true; + } + return false; + } + + // ImmutableDictionary + if (types.ImmutableDictionary_T2 != null && constructed.ConstructedFrom.Equals(types.ImmutableDictionary_T2, SymbolEqualityComparer.Default)) + { + if (constructed.TypeArguments.Length == 2 && SymbolEqualityComparer.Default.Equals(constructed.TypeArguments[0], types.String)) + { + kind = "ImmutableDictionary"; + requires = RequiredComparer.IEqualityComparer; + return true; + } + return false; + } + + return false; + } + + private static bool HasRequiredComparerParameter(IMethodSymbol method, RequiredComparer requires, WellKnownTypes types) + { + foreach (IParameterSymbol param in method.Parameters) + { + if (requires == RequiredComparer.IEqualityComparer && + types.IEqualityComparer_T != null && + param.Type is INamedTypeSymbol p && + p.OriginalDefinition.Equals(types.IEqualityComparer_T, SymbolEqualityComparer.Default) && + p.TypeArguments.Length == 1 && SymbolEqualityComparer.Default.Equals(p.TypeArguments[0], types.String)) + { + return true; + } + + if (requires == RequiredComparer.IComparer && + types.IComparer_T != null && + param.Type is INamedTypeSymbol p2 && + p2.OriginalDefinition.Equals(types.IComparer_T, SymbolEqualityComparer.Default) && + p2.TypeArguments.Length == 1 && SymbolEqualityComparer.Default.Equals(p2.TypeArguments[0], types.String)) + { + return true; + } + } + + return false; + } + + private sealed class WellKnownTypes + { + public INamedTypeSymbol String { get; } + public INamedTypeSymbol IEqualityComparer_T { get; } + public INamedTypeSymbol IComparer_T { get; } + public INamedTypeSymbol Dictionary_T2 { get; } + public INamedTypeSymbol HashSet_T { get; } + public INamedTypeSymbol ConcurrentDictionary_T2 { get; } + public INamedTypeSymbol SortedDictionary_T2 { get; } + public INamedTypeSymbol SortedSet_T { get; } + public INamedTypeSymbol ImmutableDictionary_T2 { get; } + public INamedTypeSymbol ImmutableDictionary { get; } + + public WellKnownTypes(Compilation compilation) + { + String = compilation.GetSpecialType(SpecialType.System_String); + IEqualityComparer_T = compilation.GetTypeByMetadataName("System.Collections.Generic.IEqualityComparer`1"); + IComparer_T = compilation.GetTypeByMetadataName("System.Collections.Generic.IComparer`1"); + Dictionary_T2 = compilation.GetTypeByMetadataName("System.Collections.Generic.Dictionary`2"); + HashSet_T = compilation.GetTypeByMetadataName("System.Collections.Generic.HashSet`1"); + ConcurrentDictionary_T2 = compilation.GetTypeByMetadataName("System.Collections.Concurrent.ConcurrentDictionary`2"); + SortedDictionary_T2 = compilation.GetTypeByMetadataName("System.Collections.Generic.SortedDictionary`2"); + SortedSet_T = compilation.GetTypeByMetadataName("System.Collections.Generic.SortedSet`1"); + ImmutableDictionary_T2 = compilation.GetTypeByMetadataName("System.Collections.Immutable.ImmutableDictionary`2"); + ImmutableDictionary = compilation.GetTypeByMetadataName("System.Collections.Immutable.ImmutableDictionary"); + } + } + } +} \ No newline at end of file diff --git a/Philips.CodeAnalysis.Test/Maintainability/Maintainability/StringDictionaryNeedsComparerAnalyzerTest.cs b/Philips.CodeAnalysis.Test/Maintainability/Maintainability/StringDictionaryNeedsComparerAnalyzerTest.cs new file mode 100644 index 000000000..7f6c56f90 --- /dev/null +++ b/Philips.CodeAnalysis.Test/Maintainability/Maintainability/StringDictionaryNeedsComparerAnalyzerTest.cs @@ -0,0 +1,302 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Philips.CodeAnalysis.Common; +using Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability; +using Philips.CodeAnalysis.Test.Helpers; +using Philips.CodeAnalysis.Test.Verifiers; + +namespace Philips.CodeAnalysis.Test.Maintainability.Maintainability +{ + [TestClass] + public class StringDictionaryNeedsComparerAnalyzerTest : DiagnosticVerifier + { + protected override DiagnosticAnalyzer GetDiagnosticAnalyzer() + { + return new StringDictionaryNeedsComparerAnalyzer(); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task DictionaryWithStringKeyWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new Dictionary(); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task DictionaryWithStringKeyWithComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new Dictionary(StringComparer.Ordinal); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task HashSetWithStringKeyWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var set = new HashSet(); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task HashSetWithStringKeyWithComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var set = new HashSet(StringComparer.OrdinalIgnoreCase); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task ConcurrentDictionaryWithStringKeyWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Concurrent; +class Foo +{ + public void Test() + { + var dict = new ConcurrentDictionary(); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task ConcurrentDictionaryWithStringKeyWithComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Concurrent; +class Foo +{ + public void Test() + { + var dict = new ConcurrentDictionary(StringComparer.Ordinal); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task SortedDictionaryWithStringKeyWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new SortedDictionary(); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task SortedDictionaryWithStringKeyWithComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new SortedDictionary(StringComparer.Ordinal); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task SortedSetWithStringKeyWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var set = new SortedSet(); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task SortedSetWithStringKeyWithComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var set = new SortedSet(StringComparer.OrdinalIgnoreCase); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task ImmutableDictionaryCreateWithStringKeyWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Immutable; +class Foo +{ + public void Test() + { + var dict = ImmutableDictionary.Create(); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task ImmutableDictionaryCreateWithStringKeyWithComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Immutable; +class Foo +{ + public void Test() + { + var dict = ImmutableDictionary.Create(StringComparer.Ordinal); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task DictionaryWithNonStringKeyDoesNotTriggerAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new Dictionary(); + var dict2 = new Dictionary(); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task HashSetWithNonStringKeyDoesNotTriggerAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var set = new HashSet(); + var set2 = new HashSet(); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task DictionaryWithCapacityParameterWithoutComparerTriggersAsync() + { + const string template = @" +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new Dictionary(10); + } +} +"; + await VerifyDiagnostic(template, DiagnosticId.StringDictionaryNeedsComparer).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task DictionaryWithCapacityAndComparerDoesNotTriggerAsync() + { + const string template = @" +using System; +using System.Collections.Generic; +class Foo +{ + public void Test() + { + var dict = new Dictionary(10, StringComparer.Ordinal); + } +} +"; + await VerifySuccessfulCompilation(template).ConfigureAwait(false); + } + } +} \ No newline at end of file