Add a new analyzer for checking record equality implementations

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

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,52 @@ public static bool IsOrInheritsFromClass(this ITypeSymbol type, string className
         {
             return type.GetBaseTypesAndThis().OfType<INamedTypeSymbol>().Any(t => t.GetFullName() == className);
         }
+
+        /// <summary>
+        /// Gets all of the public properties of the type.
+        /// </summary>
+        /// <param name="typeSymbol">the symbol to inspect</param>
+        /// <returns>Array of <see cref="IPropertySymbol"/> of the public properties</returns>
+        public static ImmutableArray<IPropertySymbol> GetPublicPropertySymbols(
+            this ITypeSymbol typeSymbol)
+        {
+            return typeSymbol.GetMembers()
+                .Where(m => m.Kind == SymbolKind.Property
+                    && m.DeclaredAccessibility == Accessibility.Public)
+                .Cast<IPropertySymbol>()
+                .ToImmutableArray();
+        }
+
+        /// <summary>
+        /// Gets whether this type is an enumerable.
+        /// </summary>
+        /// <param name="typeSymbol">the symbol to inspect.</param>
+        /// <returns>True if the type implements IEnumerable, false otherwise.</returns>
+        /// <remarks>
+        /// string types will return false, despite implementing IEnumerable{char}, because
+        /// we don't generally consider strings as enumerables.
+        /// </remarks>
+        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<char> but we don't generally
+                // consider strings as an "enumerable".
+                return false;
+            }
+
+            return typeSymbol.AllInterfaces.Any(i => i.Name == nameof(IEnumerable));
+        }
     }
 }

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

src/NationalInstruments.Analyzers/Correctness/RecordWithEnumerablesShouldOverrideDefaultEqualityAnalyzer.cs

@@ -0,0 +1,95 @@
+using System;
+using System.Collections.Immutable;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+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<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
+
+        public override void Initialize(AnalysisContext context)
+        {
+            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)
+                .Select(p => p.Name)
+                .ToImmutableHashSet();
+
+            var enumerableProperties = typeSymbol
+                .GetPublicPropertySymbols()
+                .Where(p => p.Type.IsEnumerable() && !baseTypeProperties.Contains(p.Name))
+                .ToImmutableArray();
+
+            if (enumerableProperties.Length == 0)
+            {
+                return;
+            }
+
+            // check if the record type has implemented its own Equality methods
+            foreach (var location in typeSymbol.Locations)
+            {
+                var rootNode = (CompilationUnitSyntax?)location.SourceTree?.GetRoot()
+                    ?? throw new InvalidOperationException("The SourceTree of the record is null");
+
+                var recordDeclarationNode = rootNode
+                    .DescendantNodes()
+                    .OfType<RecordDeclarationSyntax>()
+                    .First(r => r.Identifier.ValueText == typeSymbol.Name);
+
+                if (recordDeclarationNode.Members
+                    .OfType<MethodDeclarationSyntax>()
+                    .Any(m => m.Identifier.Text == "Equals"))
+                {
+                    // we don't need to check for GetHashCode because the built-in C# analyzer will
+                    // flag code that implements Equals but not GetHashCode.
+                    return;
+                }
+            }
+
+            context.ReportDiagnostic(Diagnostic.Create(Rule, typeSymbol.Locations[0], typeSymbol.Name));
+        }
+
+        private ImmutableArray<IPropertySymbol> 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<IPropertySymbol>.Empty;
+            }
+
+            return baseType.GetPublicPropertySymbols();
+        }
+    }
+}

src/NationalInstruments.Analyzers/Properties/Resources.resx

@@ -414,6 +414,16 @@
   <data name="NI1816X_Title" xml:space="preserve">
     <value>Disposable should not have finalizer</value>
   </data>
+  <data name="NI1019_Description" xml:space="preserve">
+    <value>The default equality method implementation for records is to compare enumerable types by reference, not the element values;  thus it is important for records with enumerable properties to override the default implementation and compare the enumerables using SequenceEqual or UnorderedEquals (depending on whether ordering matters).</value>
+  </data>
+  <data name="NI1019_Message" xml:space="preserve">
+    <value>Record '{0}' has properties that are enumerable collections, but does not override the default Equals and GetHashCode methods</value>
+    <comment>{0} is the name of the record</comment>
+  </data>
+  <data name="NI1019_Title" xml:space="preserve">
+    <value>Records that have enumerable properties should override the default Equals and GetHashCode methods</value>
+  </data>
   <assembly alias="System.Windows.Forms" name="System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" />
   <data name="Dictionary" type="System.Resources.ResXFileRef, System.Windows.Forms">
     <value>Dictionary.dic;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;Windows-1252</value>