-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Add PH2164 analyzer to enforce StringComparer for string-keyed collections #1029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
8863cc6
f906595
30290a3
1cb8646
f72b6d6
afbf16f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | Warning | | ||
|
||
| | Enabled By Default | No | | ||
|
||
|
|
||
| ## Introduction | ||
|
|
||
| String-keyed collections should explicitly specify a StringComparer (or IComparer<string> 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<string>) 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<string, int>(); | ||
| var set = new HashSet<string>(); | ||
|
|
||
| // Also problematic for sorted collections | ||
| var sortedDict = new SortedDictionary<string, int>(); | ||
| var sortedSet = new SortedSet<string>(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| And the improved replacement: | ||
| ``` cs | ||
| class GoodExample | ||
| { | ||
| public void GoodMethod() | ||
| { | ||
| // Explicit StringComparer for predictable behavior | ||
| var dict = new Dictionary<string, int>(StringComparer.Ordinal); | ||
| var set = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| // Explicit comparer for sorted collections | ||
| var sortedDict = new SortedDictionary<string, int>(StringComparer.Ordinal); | ||
| var sortedSet = new SortedSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Supported Collections | ||
|
|
||
| The analyzer detects the following string-keyed collection types: | ||
|
|
||
| - `Dictionary<string, TValue>` | ||
| - `HashSet<string>` | ||
| - `ConcurrentDictionary<string, TValue>` (planned) | ||
| - `SortedDictionary<string, TValue>` (planned) | ||
| - `SortedSet<string>` (planned) | ||
| - `ImmutableDictionary<string, TValue>` (planned) | ||
|
|
||
| ## Configuration | ||
|
|
||
| This analyzer is disabled by default. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| // © 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<string> 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.Warning, isEnabledByDefault: false, description: Description); | ||
|
||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> 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<string>" | ||
| : "StringComparer/IComparer<string>"; | ||
|
|
||
| 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<TKey, TValue>(...) without comparer | ||
| if (types.ImmutableDictionary != null && targetMethod.ContainingType.Equals(types.ImmutableDictionary, SymbolEqualityComparer.Default)) | ||
| { | ||
| if (targetMethod.Name is "Create" or "CreateRange") | ||
|
Check warning on line 84 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs
|
||
|
||
| { | ||
| ImmutableArray<ITypeSymbol> typeArgs = targetMethod.TypeArguments; | ||
| if (typeArgs.Length == 2 && SymbolEqualityComparer.Default.Equals(typeArgs[0], types.String)) | ||
| { | ||
| if (!HasRequiredComparerParameter(targetMethod, RequiredComparer.IEqualityComparer, types)) | ||
|
Check warning on line 89 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs
|
||
|
||
| { | ||
| var collectionDisplay = $"ImmutableDictionary<{typeArgs[0].ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat)}, {typeArgs[1].ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat)}>"; | ||
| context.ReportDiagnostic(Diagnostic.Create( | ||
| Rule, | ||
| invocation.Syntax.GetLocation(), | ||
| collectionDisplay, | ||
| "StringComparer/IEqualityComparer<string>")); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private enum RequiredComparer { IEqualityComparer, IComparer } | ||
|
|
||
| private static bool TryGetCollectionInfo(INamedTypeSymbol constructed, WellKnownTypes types, out string kind, out RequiredComparer requires) | ||
|
Check warning on line 105 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs
|
||
| { | ||
| kind = default; | ||
| requires = default; | ||
|
|
||
| // HashSet<string> | ||
| 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<string, TValue> | ||
| 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<string, TValue> | ||
| 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<string, TValue> | ||
| 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<string> | ||
| 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<string, TValue> | ||
| 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) | ||
|
Check warning on line 187 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/StringDictionaryNeedsComparerAnalyzer.cs
|
||
| { | ||
| 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"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Severity should be error