diff --git a/docs/README.md b/docs/README.md index 305d93253..6bd7e998f 100755 --- a/docs/README.md +++ b/docs/README.md @@ -2,7 +2,7 @@ |Id|Category|Description|Severity|Is enabled|Code fix|Configurable| |--|--------|-----------|:------:|:--------:|:------:|:----------:| |[MA0001](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0001.md)|Usage|StringComparison is missing|ℹ️|✔️|✔️|❌| -|[MA0002](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0002.md)|Usage|IEqualityComparer\ or IComparer\ is missing|⚠️|✔️|✔️|✔️| +|[MA0002](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0002.md)|Usage|IEqualityComparer\ or IComparer\ is missing|⚠️|✔️|✔️|✔️| |[MA0003](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0003.md)|Style|Add parameter name to improve readability|ℹ️|✔️|✔️|✔️| |[MA0004](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0004.md)|Usage|Use Task.ConfigureAwait|⚠️|✔️|✔️|✔️| |[MA0005](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0005.md)|Performance|Use Array.Empty\()|⚠️|✔️|✔️|❌| diff --git a/docs/Rules/MA0002.md b/docs/Rules/MA0002.md index 5cd0ebcf1..c5fd3f160 100644 --- a/docs/Rules/MA0002.md +++ b/docs/Rules/MA0002.md @@ -44,8 +44,6 @@ list.Distinct(StringComparer.Ordinal); `IQueryable` query-operator calls are excluded from this recommendation because comparer overloads are often not translatable by query providers. -Calls to `Meziantou.Framework.Assertions.Assert` are excluded because assertion methods intentionally choose the comparison semantics. - # Configuration The rule can be configured using an `.editorconfig` file: @@ -57,6 +55,13 @@ MA0002.exclude_query_operator_syntaxes = false # Report collection expressions ([]) for C#14 and lower # C# preview reports those diagnostics by default MA0002.report_collection_expressions = false + +# Only report APIs whose default string comparison is not known to be ordinal. +# When enabled, equality-based APIs whose default comparer is already ordinal are not reported +# (e.g. HashSet, Dictionary, ConcurrentDictionary, ToDictionary, ToImmutableDictionary, Distinct, +# GroupBy, and Meziantou.Framework.Assertions.Assert), while culture-sensitive ordering APIs are +# still reported (e.g. SortedDictionary, SortedSet, SortedList, OrderBy, ThenBy, Order). +MA0002.report_only_non_ordinal = false ``` ## Additional resources diff --git a/src/Meziantou.Analyzer/Rules/UseStringComparerAnalyzer.cs b/src/Meziantou.Analyzer/Rules/UseStringComparerAnalyzer.cs index 5ccee6b5b..fd05ca947 100644 --- a/src/Meziantou.Analyzer/Rules/UseStringComparerAnalyzer.cs +++ b/src/Meziantou.Analyzer/Rules/UseStringComparerAnalyzer.cs @@ -42,6 +42,48 @@ public sealed class UseStringComparerAnalyzer : DiagnosticAnalyzer { "ToLookup", 1 }, }; + // Methods whose default string comparison is ordinal (equality-based). Ordering methods + // (Order/OrderBy/OrderByDescending/ThenBy/ThenByDescending) are intentionally excluded because + // their default is Comparer.Default (= StringComparer.CurrentCulture, culture-sensitive). + private static readonly HashSet KnownOrdinalMethodNames = new(StringComparer.Ordinal) + { + // System.Linq.Enumerable / System.Linq.Queryable (methods taking IEqualityComparer). + // Ordering methods (Order/OrderBy/OrderByDescending/OrderDescending/ThenBy/ThenByDescending) and + // Min/Max/MinBy/MaxBy take IComparer (culture-sensitive) and are intentionally excluded. + "AggregateBy", + "Contains", + "CountBy", + "Distinct", + "DistinctBy", + "Except", + "ExceptBy", + "GroupBy", + "GroupJoin", + "Intersect", + "IntersectBy", + "Join", + "LeftJoin", + "RightJoin", + "SequenceEqual", + "ToDictionary", + "ToHashSet", + "ToLookup", + "Union", + "UnionBy", + + // System.Collections.Immutable / System.Collections.Frozen factory methods. These names also + // exist on the Sorted variants (IComparer, culture-sensitive), but those are excluded + // by scoping to the containers in BuildKnownOrdinalContainerTypes. + "Create", + "CreateBuilder", + "CreateRange", + "CreateRangeWithOverwrite", + "ToImmutableDictionary", + "ToImmutableHashSet", + "ToFrozenDictionary", + "ToFrozenSet", + }; + private static readonly DiagnosticDescriptor Rule = new( RuleIdentifiers.UseStringComparer, title: "IEqualityComparer or IComparer is missing", @@ -53,6 +95,7 @@ public sealed class UseStringComparerAnalyzer : DiagnosticAnalyzer helpLinkUri: RuleIdentifiers.GetHelpUri(RuleIdentifiers.UseStringComparer)); private static readonly ConfigurationDefinition ExcludeQueryOperatorSyntaxesConfiguration = new(Rule.Id + ".exclude_query_operator_syntaxes", defaultValue: false); + private static readonly ConfigurationDefinition ReportOnlyNonOrdinalConfiguration = new(Rule.Id + ".report_only_non_ordinal", defaultValue: false); #if ROSLYN_4_14_OR_GREATER private static readonly ConfigurationDefinition ReportCollectionExpressionsConfiguration = new(Rule.Id + ".report_collection_expressions", defaultValue: false); #endif @@ -80,6 +123,15 @@ private sealed class AnalyzerContext(Compilation compilation) private readonly OverloadFinder _overloadFinder = new(compilation); private readonly OperationUtilities _operationUtilities = new(compilation); + // Types whose default string comparison is ordinal (equality-based collections). Ordering + // collections (SortedDictionary/SortedList/SortedSet) are intentionally excluded because their + // default is Comparer.Default (= StringComparer.CurrentCulture, culture-sensitive). + private readonly HashSet _knownOrdinalTypes = BuildKnownOrdinalTypes(compilation); + + // Static classes hosting the known-ordinal methods, used to avoid suppressing unrelated + // user-defined methods that happen to share a name with a BCL method. + private readonly HashSet _knownOrdinalContainerTypes = BuildKnownOrdinalContainerTypes(compilation); + public INamedTypeSymbol? EqualityComparerStringType { get; } = GetIEqualityComparerString(compilation); public INamedTypeSymbol? ComparerStringType { get; } = GetIComparerString(compilation); public INamedTypeSymbol? EnumerableType { get; } = compilation.GetBestTypeByMetadataName("System.Linq.Enumerable"); @@ -102,6 +154,9 @@ public void AnalyzeConstructor(OperationAnalysisContext ctx) if ((EqualityComparerStringType is not null && _overloadFinder.HasOverloadWithAdditionalParameterOfType(method, options: default, [EqualityComparerStringType])) || (ComparerStringType is not null && _overloadFinder.HasOverloadWithAdditionalParameterOfType(method, options: default, [ComparerStringType]))) { + if (ctx.Options.GetConfigurationValue(operation, ReportOnlyNonOrdinalConfiguration) && IsKnownOrdinalType(operation.Type)) + return; + ctx.ReportDiagnostic(Rule, operation); } } @@ -116,8 +171,6 @@ public void AnalyzeInvocation(OperationAnalysisContext ctx) return; var method = operation.TargetMethod; - if (method.ContainingType.IsEqualTo(MeziantouFrameworkAssertType)) - return; // Most ISet implementation already configured the IEqualityComparer in this constructor, // so it should be ok to skip method calls on those types. @@ -145,6 +198,9 @@ public void AnalyzeInvocation(OperationAnalysisContext ctx) if ((EqualityComparerStringType is not null && _overloadFinder.HasOverloadWithAdditionalParameterOfType(operation, options: default, [EqualityComparerStringType])) || (ComparerStringType is not null && _overloadFinder.HasOverloadWithAdditionalParameterOfType(operation, options: default, [ComparerStringType]))) { + if (IsInvocationReportSuppressedByOrdinalOption(ctx, operation, method)) + return; + ctx.ReportDiagnostic(Rule, operation, DefaultDiagnosticInvocationReportOptions); return; } @@ -179,6 +235,9 @@ public void AnalyzeInvocation(OperationAnalysisContext ctx) if (!HasEqualityComparerArgument(operation.Arguments)) { + if (IsInvocationReportSuppressedByOrdinalOption(ctx, operation, method)) + return; + ctx.ReportDiagnostic(Rule, operation, DefaultDiagnosticInvocationReportOptions); } } @@ -210,6 +269,9 @@ public void AnalyzeCollectionExpression(OperationAnalysisContext ctx) if ((EqualityComparerStringType is not null && _overloadFinder.HasOverloadWithAdditionalParameterOfType(constructMethod, options: default, [EqualityComparerStringType])) || (ComparerStringType is not null && _overloadFinder.HasOverloadWithAdditionalParameterOfType(constructMethod, options: default, [ComparerStringType]))) { + if (ctx.Options.GetConfigurationValue(operation, ReportOnlyNonOrdinalConfiguration) && IsKnownOrdinalType(operation.Type)) + return; + ctx.ReportDiagnostic(Rule, operation); } @@ -267,6 +329,50 @@ private bool HasEqualityComparerConstructArgument(ImmutableArray con #pragma warning restore RSEXPERIMENTAL006 #endif + private bool IsInvocationReportSuppressedByOrdinalOption(OperationAnalysisContext ctx, IInvocationOperation operation, IMethodSymbol method) + { + if (!ctx.Options.GetConfigurationValue(operation, ReportOnlyNonOrdinalConfiguration)) + return false; + + if (method.ContainingType.IsEqualTo(MeziantouFrameworkAssertType)) + return true; + + return KnownOrdinalMethodNames.Contains(method.Name) + && _knownOrdinalContainerTypes.Contains(method.ContainingType.OriginalDefinition); + } + + private bool IsKnownOrdinalType(ITypeSymbol? type) + { + return type is INamedTypeSymbol namedType + && _knownOrdinalTypes.Contains(namedType.OriginalDefinition); + } + + private static HashSet BuildKnownOrdinalTypes(Compilation compilation) + { + var result = new HashSet(SymbolEqualityComparer.Default); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Generic.HashSet`1")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Generic.Dictionary`2")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Generic.OrderedDictionary`2")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Concurrent.ConcurrentDictionary`2")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Immutable.ImmutableDictionary`2")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Immutable.ImmutableHashSet`1")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Frozen.FrozenDictionary`2")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Frozen.FrozenSet`1")); + return result; + } + + private static HashSet BuildKnownOrdinalContainerTypes(Compilation compilation) + { + var result = new HashSet(SymbolEqualityComparer.Default); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Linq.Enumerable")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Linq.Queryable")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Immutable.ImmutableDictionary")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Immutable.ImmutableHashSet")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Frozen.FrozenDictionary")); + result.AddIfNotNull(compilation.GetBestTypeByMetadataName("System.Collections.Frozen.FrozenSet")); + return result; + } + private static INamedTypeSymbol? GetIEqualityComparerString(Compilation compilation) { var equalityComparerInterfaceType = compilation.GetBestTypeByMetadataName("System.Collections.Generic.IEqualityComparer`1"); diff --git a/tests/Meziantou.Analyzer.Test/Rules/UseStringComparerAnalyzerTests.cs b/tests/Meziantou.Analyzer.Test/Rules/UseStringComparerAnalyzerTests.cs index c13b8f22b..3e1c1a771 100644 --- a/tests/Meziantou.Analyzer.Test/Rules/UseStringComparerAnalyzerTests.cs +++ b/tests/Meziantou.Analyzer.Test/Rules/UseStringComparerAnalyzerTests.cs @@ -7,6 +7,7 @@ namespace Meziantou.Analyzer.Test.Rules; public sealed class UseStringComparerAnalyzerTests { private const string ReportCollectionExpressionsConfigurationName = "MA0002.report_collection_expressions"; + private const string ReportOnlyNonOrdinalConfigurationName = "MA0002.report_only_non_ordinal"; private static ProjectBuilder CreateProjectBuilder() { @@ -332,6 +333,25 @@ public void Test() .ValidateAsync(); } + [Fact] + public async Task HashSet_String_CollectionExpression_ReportOnlyNonOrdinal_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithLanguageVersion(Microsoft.CodeAnalysis.CSharp.LanguageVersion.CSharp12) + .AddAnalyzerConfiguration(ReportCollectionExpressionsConfigurationName, "true") + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + System.Collections.Generic.HashSet a = []; + } + } + """) + .ValidateAsync(); + } + [Fact] public async Task HashSet_String_CollectionExpression_WithElements_CSharp12_OptionEnabled_ShouldReportDiagnostic() { @@ -350,6 +370,25 @@ public void Test() .ValidateAsync(); } + [Fact] + public async Task HashSet_String_CollectionExpression_WithElements_ReportOnlyNonOrdinal_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithLanguageVersion(Microsoft.CodeAnalysis.CSharp.LanguageVersion.CSharp12) + .AddAnalyzerConfiguration(ReportCollectionExpressionsConfigurationName, "true") + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + System.Collections.Generic.HashSet a = ["a", "b"]; + } + } + """) + .ValidateAsync(); + } + [Fact] public async Task FrozenSet_String_CollectionExpression_DefaultOnCSharp12_ShouldNotReportDiagnostic() { @@ -768,7 +807,77 @@ await CreateProjectBuilder() } [Fact] - public async Task MeziantouFrameworkAssertions_Assert_ShouldNotReportDiagnostic() + public async Task ImmutableDictionary_Create_String_ShouldReportDiagnostic() + { + const string SourceCode = """ + class TypeName + { + public void Test() + { + System.Collections.Immutable.ImmutableDictionary.[|Create()|]; + } + } + """; + + await CreateProjectBuilder() + .WithTargetFramework(TargetFramework.Net4_8) + .WithSourceCode(SourceCode) + .ValidateAsync(); + } + + [Fact] + public async Task MeziantouFrameworkAssertions_Assert_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + namespace Meziantou.Framework.Assertions + { + static class Assert + { + public static void AreEqual(string expected, string actual) { } + public static void AreEqual(string expected, string actual, System.Collections.Generic.IEqualityComparer comparer) { } + } + } + + class TypeName + { + public void Test() + { + Meziantou.Framework.Assertions.Assert.[|AreEqual("a", "b")|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task MeziantouFrameworkAssertions_Assert_ReportOnlyNonOrdinal_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + namespace Meziantou.Framework.Assertions + { + static class Assert + { + public static void AreEqual(string expected, string actual) { } + public static void AreEqual(string expected, string actual, System.Collections.Generic.IEqualityComparer comparer) { } + } + } + + class TypeName + { + public void Test() + { + Meziantou.Framework.Assertions.Assert.AreEqual("a", "b"); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task MeziantouFrameworkAssertions_Assert_WithoutComparerOverload_ShouldNotReportDiagnostic() { await CreateProjectBuilder() .WithTargetFramework(TargetFramework.Net10_0) @@ -786,6 +895,205 @@ public void Test() .ValidateAsync(); } + [Fact] + public async Task ReportOnlyNonOrdinal_HashSet_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + new System.Collections.Generic.HashSet(); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_Dictionary_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + new System.Collections.Generic.Dictionary(); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_ConcurrentDictionary_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + new System.Collections.Concurrent.ConcurrentDictionary(); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_EnumerableDistinct_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + using System.Linq; + class TypeName + { + public void Test() + { + System.Collections.Generic.IEnumerable obj = null; + obj.Distinct(); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_EnumerableToDictionary_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + using System.Linq; + class TypeName + { + public void Test() + { + System.Collections.Generic.IEnumerable obj = null; + obj.ToDictionary(p => p); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_ImmutableDictionaryCreateBuilder_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithTargetFramework(TargetFramework.Net4_8) + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + System.Collections.Immutable.ImmutableDictionary.CreateBuilder(); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_ImmutableDictionaryCreate_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithTargetFramework(TargetFramework.Net4_8) + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + System.Collections.Immutable.ImmutableDictionary.Create(); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_ImmutableSortedDictionaryCreate_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithTargetFramework(TargetFramework.Net4_8) + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + System.Collections.Immutable.ImmutableSortedDictionary.[|Create()|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_SortedDictionary_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + class TypeName + { + public void Test() + { + [|new System.Collections.Generic.SortedDictionary()|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_OrderBy_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + using System.Linq; + class TypeName + { + public void Test() + { + System.Collections.Generic.IEnumerable obj = null; + obj.[|OrderBy(p => p)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ReportOnlyNonOrdinal_Order_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithTargetFramework(TargetFramework.Net7_0) + .AddAnalyzerConfiguration(ReportOnlyNonOrdinalConfigurationName, "true") + .WithSourceCode(""" + using System.Linq; + class TypeName + { + public void Test() + { + System.Collections.Generic.IEnumerable obj = null; + obj.[|Order()|]; + } + } + """) + .ValidateAsync(); + } + [Fact] public async Task EnumerableContains_String_ShouldReportDiagnostic() {