From 013bb485050a1245c73d2edd8390e02b39df76bb Mon Sep 17 00:00:00 2001 From: Jonathan Ou-yang Date: Fri, 22 Mar 2024 22:00:02 +0800 Subject: [PATCH] Add a new analyzer for checking record equality implementations (#94) * Add analyzer for checking record enumerable equality * Fix release build * Support finding records inside namespaces and nested classes, as well as multiple records in a single file * Enable CA1852 ignore internals * Enable concurrent execution * fix build * Add failing test * make test pass * add failing test * make test pass * Update resource strings * Fix release build * Update documentation and method name * Update capitalization. * Add new test * Update rule description and message * update designer * update description * Remove space --------- Co-authored-by: Peter Kurlak --- .../NI.CSharp.Analyzers.globalconfig | 11 + .../Extensions/IMethodSymbolExtensions.cs | 13 + .../Extensions/ITypeSymbolExtensions.cs | 63 ++++ .../AnalyzerReleases.Shipped.md | 8 + ...esShouldOverrideDefaultEqualityAnalyzer.cs | 82 +++++ .../Properties/Resources.Designer.cs | 27 ++ .../Properties/Resources.resx | 10 + ...uldOverrideDefaultEqualityAnalyzerTests.cs | 285 ++++++++++++++++++ 8 files changed, 499 insertions(+) create mode 100644 src/NationalInstruments.Analyzers/Correctness/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer.cs create mode 100644 tests/NationalInstruments.Analyzers.UnitTests/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzerTests.cs diff --git a/src/AnalyzerConfiguration/NI.CSharp.Analyzers.globalconfig b/src/AnalyzerConfiguration/NI.CSharp.Analyzers.globalconfig index 13b3edd..7e56548 100644 --- a/src/AnalyzerConfiguration/NI.CSharp.Analyzers.globalconfig +++ b/src/AnalyzerConfiguration/NI.CSharp.Analyzers.globalconfig @@ -231,6 +231,17 @@ dotnet_diagnostic.CA1825.severity = none dotnet_diagnostic.CA1826.severity = none +# CA1852: Seal internal types +# CA 1852 is already enabled, but by default, the analyzer treats internal types as public +# if the assembly has InternalsVisibleTo attribute defined. Most of our assemblies have +# InternalsVisibleTo defined for at least test projects, and thus checking internal classes +# will basically be disabled for our code base. +# +# By setting ignore_internalsvisibleto to true, we ensure internal classes are checked. For +# any unsealed internal class which the test class needs to derive from, we will require developers to +# manually suppress CA 1852 and explain why. +dotnet_code_quality.CA1852.ignore_internalsvisibleto = true + dotnet_diagnostic.CA2000.severity = none dotnet_diagnostic.CA2001.severity = warning diff --git a/src/NationalInstruments.Analyzers.Utilities/Extensions/IMethodSymbolExtensions.cs b/src/NationalInstruments.Analyzers.Utilities/Extensions/IMethodSymbolExtensions.cs index de6e69a..1384ac7 100644 --- a/src/NationalInstruments.Analyzers.Utilities/Extensions/IMethodSymbolExtensions.cs +++ b/src/NationalInstruments.Analyzers.Utilities/Extensions/IMethodSymbolExtensions.cs @@ -82,5 +82,18 @@ public static ImmutableArray GetOriginalDefinitions(this IMethodS return originalDefinitionsBuilder.ToImmutable(); } + + /// + /// Whether the method is IEquatable{T}.Equals method and explicitly declared + /// (i.e. not synthesized by the compiler). + /// + /// The method symbol to check. + /// True if the method is the IEquatable{T}.Equals method and explicitly declared, false otherwise. + public static bool IsExplicitIEquatableEquals(this IMethodSymbol method) + => method.Name == WellKnownMemberNames.ObjectEquals + && method.ReturnType.SpecialType == SpecialType.System_Boolean + && method.Parameters.Length == 1 + && method.Parameters[0].Type.Equals(method.ContainingType, SymbolEqualityComparer.Default) + && !method.IsImplicitlyDeclared; } } diff --git a/src/NationalInstruments.Analyzers.Utilities/Extensions/ITypeSymbolExtensions.cs b/src/NationalInstruments.Analyzers.Utilities/Extensions/ITypeSymbolExtensions.cs index 9d7f485..7801788 100644 --- a/src/NationalInstruments.Analyzers.Utilities/Extensions/ITypeSymbolExtensions.cs +++ b/src/NationalInstruments.Analyzers.Utilities/Extensions/ITypeSymbolExtensions.cs @@ -1,4 +1,7 @@ +using System; +using System.Collections; using System.Collections.Generic; +using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; @@ -104,5 +107,65 @@ public static bool IsOrInheritsFromClass(this ITypeSymbol type, string className { return type.GetBaseTypesAndThis().OfType().Any(t => t.GetFullName() == className); } + + /// + /// Gets all of the public properties of the type. + /// + /// The symbol to inspect + /// Array of of the public properties + public static ImmutableArray GetPublicPropertySymbols( + this ITypeSymbol typeSymbol) + { + return typeSymbol.GetMembers() + .Where(m => m.Kind == SymbolKind.Property + && m.DeclaredAccessibility == Accessibility.Public) + .Cast() + .ToImmutableArray(); + } + + /// + /// Gets whether this type is an enumerable. + /// + /// The symbol to inspect. + /// True if the type implements IEnumerable, false otherwise. + /// + /// String types will return false, despite implementing IEnumerable{char}, because + /// we don't generally consider strings as enumerables. + /// + public static bool IsEnumerable( + this ITypeSymbol typeSymbol) + { + if (typeSymbol.TypeKind == TypeKind.Array) + { + return true; + } + + if (typeSymbol.Name == nameof(IEnumerable)) + { + return true; + } + + if (typeSymbol.Name.Equals("string", StringComparison.OrdinalIgnoreCase)) + { + // strings also implement IEnumerable but we don't generally + // consider strings as an "enumerable". + return false; + } + + return typeSymbol.AllInterfaces.Any(i => i.Name == nameof(IEnumerable)); + } + + /// + /// Whether the type has an explicitly declared IEquatable{T}.Equals method. + /// + /// The type symbol to check. + /// True if the type has explicitly declared an IEquatable{T}.Equals method, false otherwise. + public static bool HasExplicitIEquatableEquals( + this ITypeSymbol typeSymbol) + { + return typeSymbol.GetMembers() + .OfType() + .Any(m => m.IsExplicitIEquatableEquals()); + } } } diff --git a/src/NationalInstruments.Analyzers/AnalyzerReleases.Shipped.md b/src/NationalInstruments.Analyzers/AnalyzerReleases.Shipped.md index 8da84c7..6e802de 100644 --- a/src/NationalInstruments.Analyzers/AnalyzerReleases.Shipped.md +++ b/src/NationalInstruments.Analyzers/AnalyzerReleases.Shipped.md @@ -22,3 +22,11 @@ NI1001 | Style | Warning | FieldsCamelCasedWithUnderscoreAnalyzer NI1017 | Style | Warning | ChainOfMethodsWithLambdasAnalyzer NI1704 | Style | Warning | SpellingAnalyzer NI1728 | Style | Disabled | SpellingAnalyzer + +## Release 2.0.22 + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +NI1019 | Correctness | Warning | RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer \ No newline at end of file diff --git a/src/NationalInstruments.Analyzers/Correctness/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer.cs b/src/NationalInstruments.Analyzers/Correctness/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer.cs new file mode 100644 index 0000000..341964f --- /dev/null +++ b/src/NationalInstruments.Analyzers/Correctness/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer.cs @@ -0,0 +1,82 @@ +using System; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using NationalInstruments.Analyzers.Properties; +using NationalInstruments.Analyzers.Utilities; +using NationalInstruments.Analyzers.Utilities.Extensions; + +namespace NationalInstruments.Analyzers.Correctness +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer + : NIDiagnosticAnalyzer + { + internal const string DiagnosticId = "NI1019"; + + public static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor( + DiagnosticId, + new LocalizableResourceString(nameof(Resources.NI1019_Title), Resources.ResourceManager, typeof(Resources)), + new LocalizableResourceString(nameof(Resources.NI1019_Message), Resources.ResourceManager, typeof(Resources)), + Category.Correctness, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: new LocalizableResourceString(nameof(Resources.NI1019_Description), Resources.ResourceManager, typeof(Resources))); + + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecutionIf(IsRunningInProduction); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSymbolAction(AnalyzeRecord, SymbolKind.NamedType); + } + + private void AnalyzeRecord(SymbolAnalysisContext context) + { + var typeSymbol = (ITypeSymbol)context.Symbol; + if (typeSymbol.IsRecord == false) + { + return; + } + + var baseTypeProperties = GetBaseTypeProperties(typeSymbol) + .ToImmutableHashSet(SymbolEqualityComparer.Default); + + var enumerableProperties = typeSymbol + .GetPublicPropertySymbols() + .Where(p => p.Type.IsEnumerable() && !baseTypeProperties.Contains(p)) + .ToImmutableArray(); + + if (enumerableProperties.Length == 0) + { + // if the record does not have any enumerable properties, + // then the default record equality implementation will work as expected. + return; + } + + if (typeSymbol.HasExplicitIEquatableEquals()) + { + // we don't need to check for GetHashCode because the built-in + // C# warning CS0659 will flag code that implements Equals but not GetHashCode. + return; + } + + context.ReportDiagnostic(Diagnostic.Create(Rule, typeSymbol.Locations[0], typeSymbol.Name)); + } + + private ImmutableArray GetBaseTypeProperties(ITypeSymbol typeSymbol) + { + var baseType = typeSymbol.BaseType; + if (baseType is null || baseType.Name == nameof(Object)) + { + // For classes that don't have a base class declared, + // the base type is not null but System.Object. + return ImmutableArray.Empty; + } + + return baseType.GetPublicPropertySymbols(); + } + } +} diff --git a/src/NationalInstruments.Analyzers/Properties/Resources.Designer.cs b/src/NationalInstruments.Analyzers/Properties/Resources.Designer.cs index ee3362d..c28a726 100644 --- a/src/NationalInstruments.Analyzers/Properties/Resources.Designer.cs +++ b/src/NationalInstruments.Analyzers/Properties/Resources.Designer.cs @@ -571,6 +571,33 @@ internal static string NI1018_Title { } } + /// + /// Looks up a localized string similar to By default, record equality is determined by calling Equals on property values. An enumerable's Equals method compares the enumerable's reference, making it a poor choice for determining whether two enumerables have the same elements . Thus, you must explicitly implement Equals(TRecord other) and compare enumerable elements with SequenceEqual (if order matters) or UnorderedEquals for the comparison to make logical sense.. + /// + internal static string NI1019_Description { + get { + return ResourceManager.GetString("NI1019_Description", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Record '{0}' has properties that are enumerable collections, but it does not explicitly implement the Equals(TRecord other) and GetHashCode methods. + /// + internal static string NI1019_Message { + get { + return ResourceManager.GetString("NI1019_Message", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Records that have enumerable properties should explicitly implement both Equals and GetHashCode methods. + /// + internal static string NI1019_Title { + get { + return ResourceManager.GetString("NI1019_Title", resourceCulture); + } + } + /// /// Looks up a localized string similar to Correct the spelling of '{0}' in assembly name {1}. /// diff --git a/src/NationalInstruments.Analyzers/Properties/Resources.resx b/src/NationalInstruments.Analyzers/Properties/Resources.resx index 572a2ee..c745b0c 100644 --- a/src/NationalInstruments.Analyzers/Properties/Resources.resx +++ b/src/NationalInstruments.Analyzers/Properties/Resources.resx @@ -414,6 +414,16 @@ Disposable should not have finalizer + + By default, record equality is determined by calling Equals on property values. An enumerable's Equals method compares the enumerable's reference, making it a poor choice for determining whether two enumerables have the same elements. Thus, you must explicitly implement Equals(TRecord other) and compare enumerable elements with SequenceEqual (if order matters) or UnorderedEquals for the comparison to make logical sense. + + + Record '{0}' has properties that are enumerable collections, but it does not explicitly implement the Equals(TRecord other) and GetHashCode methods + {0} is the name of the record + + + Records that have enumerable properties should explicitly implement both Equals and GetHashCode methods + Dictionary.dic;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;Windows-1252 diff --git a/tests/NationalInstruments.Analyzers.UnitTests/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzerTests.cs b/tests/NationalInstruments.Analyzers.UnitTests/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzerTests.cs new file mode 100644 index 0000000..1a9344f --- /dev/null +++ b/tests/NationalInstruments.Analyzers.UnitTests/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzerTests.cs @@ -0,0 +1,285 @@ +using NationalInstruments.Analyzers.Correctness; +using NationalInstruments.Analyzers.TestUtilities; +using NationalInstruments.Analyzers.TestUtilities.TestFiles; +using NationalInstruments.Analyzers.TestUtilities.Verifiers; +using Xunit; + +namespace NationalInstruments.Analyzers.UnitTests +{ + public class RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzerTests + : NIDiagnosticAnalyzerTests + { + [Fact] + public void RecordHasNoEnumerableProperties_NoDiagnostics() + { + var test = new AutoTestFile( + @"public record TestRecord + { + public int MyInt {get;} + public string MyString {get;} + }"); + + VerifyDiagnostics(test); + } + + [Fact] + public void ClassHasEnumerableProperty_NoDiagnostics() + { + var test = new AutoTestFile( + @"using System.Collections.Generic; + + public class TestClass + { + public IEnumerable MyInts {get;} + }"); + + VerifyDiagnostics(test); + } + + [Fact] + public void RecordHasEnumerableProperties_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Collections.Generic; + + public record TestRecord + { + public IEnumerable MyInts {get;} + }", + GetNI1019Rule("TestRecord")); + + VerifyDiagnostics(test); + } + + [Fact] + public void RecordHasDictionaryProperty_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Collections.Generic; + + public record TestRecord + { + public IDictionary MyDictionary {get;} + }", + GetNI1019Rule("TestRecord")); + + VerifyDiagnostics(test); + } + + [Fact] + public void RecordHasEnumerablePropertiesAndCustomEqualsImplementation_NoDiagnostics() + { + var test = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + public record TestRecord + { + public IEnumerable MyInts {get;} + + public virtual bool Equals(TestRecord other) + { + return other is not null + && MyInts.SequenceEqual(other.MyInts); + } + }"); + + VerifyDiagnostics(test); + } + + [Fact] + public void RecordHasEnumerablePropertiesAndCustomEqualsImplementationInOtherFile_NoDiagnostics() + { + var recordPropertySource = new AutoTestFile( + @"using System.Collections.Generic; + + public partial record TestRecord + { + public IEnumerable MyInts {get;} + }"); + var recordEqualsSource = new AutoTestFile( + @"using System.Linq; + public partial record TestRecord + { + public virtual bool Equals(TestRecord other) + { + return other is not null + && MyInts.SequenceEqual(other.MyInts); + } + }"); + + VerifyDiagnostics(new[] { recordPropertySource, recordEqualsSource }); + } + + [Fact] + public void RecordHasEnumerableBaseTypePropertiesAndDoesNotImplementEquality_NoDiagnostics() + { + var baseRecord = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + namespace Test; + + public record BaseRecord + { + public IEnumerable MyInts {get;} + + public virtual bool Equals(BaseRecord other) + { + return other is not null + && MyInts.SequenceEqual(other.MyInts); + } + }"); + var derivedRecord = new AutoTestFile( + @"using Test; + public record DerivedRecord : BaseRecord + { + }"); + + VerifyDiagnostics(new[] { baseRecord, derivedRecord }); + } + + [Fact] + public void NestedRecordWithEnumerablePropertyInClass_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + namespace Test; + + public class Test + { + private record TestRecord + { + public IEnumerable MyInts {get;} + } + }", + GetNI1019Rule("TestRecord")); + + VerifyDiagnostics(test); + } + + [Fact] + public void MultipleRecordsWithEnumerablePropertyInSingleFile_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + namespace Test; + + public record TestRecord + { + public IEnumerable MyInts {get;} + } + + public record TestRecord2 + { + public IEnumerable MyInts {get;} + } + + public record TestRecord3 + { + public int MyInt {get;} + } + ", + GetNI1019Rule("TestRecord"), + GetNI1019Rule("TestRecord2")); + + VerifyDiagnostics(test); + } + + [Fact] + public void DerivedRecordHidesBasePropertyWithEnumerableTypeAndDoesNotImplementEquality_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + namespace Test; + + public record BaseRecord + { + public int MyInts { get; } + } + + public record TestRecord : BaseRecord + { + public IEnumerable MyInts { get; } + }", + GetNI1019Rule("TestRecord")); + + VerifyDiagnostics(test); + } + + [Fact] + public void RecordImplementEqualsWithUnexpectedSignature_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + namespace Test; + + public record TestRecord + { + public IEnumerable MyInts { get; } + + public bool Equals(int otherInt) + { + return false; + } + }", + GetNI1019Rule("TestRecord")); + + VerifyDiagnostics(test); + } + + [Fact] + public void DerivedRecordDeclaresDifferentlyNamedPropertyAndDoesNotImplementEquality_Diagnostics() + { + var test = new AutoTestFile( + @"using System.Linq; + using System.Collections.Generic; + + namespace Test; + + public record BaseRecord + { + public IEnumerable MyInts { get; } + + public BaseRecord(IEnumerable myInts) + { + MyInts = myInts; + } + + public virtual bool Equals(BaseRecord? other) + { + return other is not null + && MyInts.SequenceEqual(other.MyInts); + } + } + + public record DerivedRecord : BaseRecord + { + public IEnumerable OtherInts { get; } + + public DerivedRecord(IEnumerable otherInts) + : base(otherInts) + { + } + }", + GetNI1019Rule("DerivedRecord")); + + VerifyDiagnostics(test); + } + + private Rule GetNI1019Rule(string typeName) + { + return new Rule( + RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer.Rule, + typeName); + } + } +}