Obsolete structural comparers and make key comparers do deep snapshot…

… for arrays

See PR discussion: #19644 (review)

src/EFCore.Cosmos/Storage/Internal/CosmosTypeMapping.cs

@@ -26,15 +26,13 @@ public class CosmosTypeMapping : CoreTypeMapping
         public CosmosTypeMapping(
             [NotNull] Type clrType,
             [CanBeNull] ValueComparer comparer = null,
-            [CanBeNull] ValueComparer keyComparer = null,
-            [CanBeNull] ValueComparer structuralComparer = null)
+            [CanBeNull] ValueComparer keyComparer = null)
             : base(
                 new CoreTypeMappingParameters(
                     clrType,
                     converter: null,
                     comparer,
-                    keyComparer,
-                    structuralComparer))
+                    keyComparer))
         {
         }
 

src/EFCore.Cosmos/Storage/Internal/CosmosTypeMappingSource.cs

@@ -32,7 +32,7 @@ public CosmosTypeMappingSource([NotNull] TypeMappingSourceDependencies dependenc
             _clrTypeMappings
                 = new Dictionary<Type, CosmosTypeMapping>
                 {
-                    { typeof(byte[]), new CosmosTypeMapping(typeof(byte[]), structuralComparer: new ArrayStructuralComparer<byte>()) }
+                    { typeof(byte[]), new CosmosTypeMapping(typeof(byte[]), keyComparer: new ArrayStructuralComparer<byte>()) }
                 };
         }
 

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs

@@ -503,7 +503,9 @@ protected virtual void GeneratePropertyAnnotations([NotNull] IProperty property,
                 CoreAnnotationNames.TypeMapping,
                 CoreAnnotationNames.ValueComparer,
                 CoreAnnotationNames.KeyValueComparer,
+#pragma warning disable 618
                 CoreAnnotationNames.StructuralValueComparer,
+#pragma warning restore 618
                 CoreAnnotationNames.ValueConverter,
                 CoreAnnotationNames.ProviderClrType);
 

src/EFCore.Design/Migrations/Design/MigrationsCodeGenerator.cs

@@ -230,7 +230,9 @@ private IEnumerable<string> GetAnnotationNamespaces(IEnumerable<IAnnotatable> it
                 CoreAnnotationNames.TypeMapping,
                 CoreAnnotationNames.ValueComparer,
                 CoreAnnotationNames.KeyValueComparer,
+#pragma warning disable 618
                 CoreAnnotationNames.StructuralValueComparer,
+#pragma warning restore 618
                 CoreAnnotationNames.ConstructorBinding,
                 CoreAnnotationNames.NavigationAccessMode,
                 CoreAnnotationNames.PropertyAccessMode,

src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs

@@ -65,7 +65,7 @@ public InMemoryTable([NotNull] IEntityType entityType, bool sensitiveLoggingEnab
                     _valueConverters.Add((property.GetIndex(), converter));
                 }
 
-                var comparer = property.GetStructuralValueComparer();
+                var comparer = property.GetKeyValueComparer();
                 if (!comparer.HasDefaultBehavior)
                 {
                     if (_valueComparers == null)
@@ -146,8 +146,8 @@ public virtual IReadOnlyList<object[]> SnapshotRows()
             return rows;
         }
 
-        private static List<ValueComparer> GetStructuralComparers(IEnumerable<IProperty> properties)
-            => properties.Select(p => p.GetStructuralValueComparer()).ToList();
+        private static List<ValueComparer> GetKeyComparers(IEnumerable<IProperty> properties)
+            => properties.Select(p => p.GetKeyValueComparer()).ToList();
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -158,7 +158,7 @@ private static List<ValueComparer> GetStructuralComparers(IEnumerable<IProperty>
         public virtual void Create(IUpdateEntry entry)
         {
             var row = entry.EntityType.GetProperties()
-                .Select(p => SnapshotValue(p, p.GetStructuralValueComparer(), entry))
+                .Select(p => SnapshotValue(p, p.GetKeyValueComparer(), entry))
                 .ToArray();
 
             _rows.Add(CreateKey(entry), row);
@@ -207,7 +207,7 @@ private static bool IsConcurrencyConflict(
         {
             if (property.IsConcurrencyToken)
             {
-                var comparer = property.GetStructuralValueComparer();
+                var comparer = property.GetKeyValueComparer();
                 var originalValue = entry.GetOriginalValue(property);
 
                 if ((comparer != null && !comparer.Equals(rowValue, originalValue))
@@ -235,7 +235,7 @@ public virtual void Update(IUpdateEntry entry)
             if (_rows.ContainsKey(key))
             {
                 var properties = entry.EntityType.GetProperties().ToList();
-                var comparers = GetStructuralComparers(properties);
+                var comparers = GetKeyComparers(properties);
                 var valueBuffer = new object[properties.Count];
                 var concurrencyConflicts = new Dictionary<IProperty, object>();
 

src/EFCore.InMemory/Storage/Internal/InMemoryTypeMapping.cs

@@ -26,15 +26,13 @@ public class InMemoryTypeMapping : CoreTypeMapping
         public InMemoryTypeMapping(
             [NotNull] Type clrType,
             [CanBeNull] ValueComparer comparer = null,
-            [CanBeNull] ValueComparer keyComparer = null,
-            [CanBeNull] ValueComparer structuralComparer = null)
+            [CanBeNull] ValueComparer keyComparer = null)
             : base(
                 new CoreTypeMappingParameters(
                     clrType,
                     converter: null,
                     comparer,
-                    keyComparer,
-                    structuralComparer))
+                    keyComparer))
         {
         }
 

src/EFCore.InMemory/Storage/Internal/InMemoryTypeMappingSource.cs

@@ -49,16 +49,12 @@ protected override CoreTypeMapping FindMapping(in TypeMappingInfo mappingInfo)
             Check.DebugAssert(clrType != null, "ClrType is null");
 
             if (clrType.IsValueType
-                || clrType == typeof(string))
+                || clrType == typeof(string)
+                || clrType == typeof(byte[]))
             {
                 return new InMemoryTypeMapping(clrType);
             }
 
-            if (clrType == typeof(byte[]))
-            {
-                return new InMemoryTypeMapping(clrType, structuralComparer: new ArrayStructuralComparer<byte>());
-            }
-
             if (clrType.FullName == "NetTopologySuite.Geometries.Geometry"
                 || clrType.GetBaseTypes().Any(t => t.FullName == "NetTopologySuite.Geometries.Geometry"))
             {
@@ -67,7 +63,6 @@ protected override CoreTypeMapping FindMapping(in TypeMappingInfo mappingInfo)
                 return new InMemoryTypeMapping(
                     clrType,
                     comparer,
-                    comparer,
                     comparer);
             }
 

src/EFCore/ChangeTracking/ValueComparer.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
 using JetBrains.Annotations;
@@ -28,6 +29,14 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking
     /// </summary>
     public abstract class ValueComparer : IEqualityComparer
     {
+        internal static readonly MethodInfo ArrayCopyMethod
+            = typeof(Array).GetMethods()
+                .Single(t => t.Name == nameof(Array.Copy)
+                    && t.GetParameters().Length == 3
+                    && t.GetParameters()[0].ParameterType == typeof(Array)
+                    && t.GetParameters()[1].ParameterType == typeof(Array)
+                    && t.GetParameters()[2].ParameterType == typeof(int));
+
         internal static readonly MethodInfo EqualityComparerHashCodeMethod
             = typeof(IEqualityComparer).GetRuntimeMethod(nameof(IEqualityComparer.GetHashCode), new[] { typeof(object) });
 

src/EFCore/ChangeTracking/ValueComparer`.cs

@@ -44,7 +44,8 @@ public class ValueComparer<T> : ValueComparer, IEqualityComparer<T>
         public ValueComparer(bool favorStructuralComparisons)
             : this(
                 CreateDefaultEqualsExpression(),
-                CreateDefaultHashCodeExpression(favorStructuralComparisons))
+                CreateDefaultHashCodeExpression(favorStructuralComparisons),
+                CreateDefaultSnapshotExpression(favorStructuralComparisons))
         {
         }
 
@@ -57,7 +58,7 @@ public ValueComparer(bool favorStructuralComparisons)
         public ValueComparer(
             [NotNull] Expression<Func<T, T, bool>> equalsExpression,
             [NotNull] Expression<Func<T, int>> hashCodeExpression)
-            : this(equalsExpression, hashCodeExpression, v => v)
+            : this(equalsExpression, hashCodeExpression, CreateDefaultSnapshotExpression(false))
         {
         }
 
@@ -157,7 +158,46 @@ protected static Expression<Func<T, T, bool>> CreateDefaultEqualsExpression()
         }
 
         /// <summary>
-        ///     Creates an expression for generated a hash code.
+        ///     Creates an expression for creating a snapshot of a value.
+        /// </summary>
+        /// <returns> The snapshot expression. </returns>
+        protected static Expression<Func<T, T>> CreateDefaultSnapshotExpression(bool favorStructuralComparisons)
+        {
+            if (!favorStructuralComparisons
+                || !typeof(T).IsArray)
+            {
+                return v => v;
+            }
+
+            var sourceParameter = Expression.Parameter(typeof(T), "source");
+            var lengthVariable = Expression.Variable(typeof(int), "length");
+            var destinationVariable = Expression.Variable(typeof(T), "destination");
+
+            // Code looks like:
+            // var length = source.Length;
+            // var destination = new T[length];
+            // Array.Copy(source, destination, length);
+            // return destination;
+            return Expression.Lambda<Func<T, T>>(
+                Expression.Block(
+                    new[] { lengthVariable, destinationVariable },
+                    Expression.Assign(
+                        lengthVariable,
+                        Expression.Property(sourceParameter, typeof(T).GetProperty(nameof(Array.Length)))),
+                    Expression.Assign(
+                        destinationVariable,
+                        Expression.NewArrayBounds(typeof(T).TryGetSequenceType(), lengthVariable)),
+                    Expression.Call(
+                        ArrayCopyMethod,
+                        sourceParameter,
+                        destinationVariable,
+                        lengthVariable),
+                    destinationVariable),
+                sourceParameter);
+        }
+
+        /// <summary>
+        ///     Creates an expression for generating a hash code.
         /// </summary>
         /// <param name="favorStructuralComparisons">
         ///     If <c>true</c>, then <see cref="IStructuralEquatable" /> is used if the type implements it.

src/EFCore/Extensions/ConventionPropertyExtensions.cs

@@ -306,19 +306,20 @@ public static void SetKeyValueComparer(
         /// <param name="property"> The property. </param>
         /// <param name="comparer"> The comparer, or <c>null</c> to remove any previously set comparer. </param>
         /// <param name="fromDataAnnotation"> Indicates whether the configuration was specified using a data annotation. </param>
+        [Obsolete("Use SetKeyValueComparer. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         public static void SetStructuralValueComparer(
             [NotNull] this IConventionProperty property,
             [CanBeNull] ValueComparer comparer,
             bool fromDataAnnotation = false)
-            => property.AsProperty().SetStructuralValueComparer(
-                comparer, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention);
+            => property.SetKeyValueComparer(comparer, fromDataAnnotation);
 
         /// <summary>
         ///     Returns the configuration source for <see cref="PropertyExtensions.GetStructuralValueComparer" />.
         /// </summary>
         /// <param name="property"> The property to find configuration source for. </param>
         /// <returns> The configuration source for <see cref="PropertyExtensions.GetStructuralValueComparer" />. </returns>
+        [Obsolete("Use GetKeyValueComparerConfigurationSource. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         public static ConfigurationSource? GetStructuralValueComparerConfigurationSource([NotNull] this IConventionProperty property)
-            => property.FindAnnotation(CoreAnnotationNames.StructuralValueComparer)?.GetConfigurationSource();
+            => property.GetKeyValueComparerConfigurationSource();
     }
 }

src/EFCore/Extensions/MutablePropertyExtensions.cs

@@ -189,7 +189,8 @@ public static void SetKeyValueComparer([NotNull] this IMutableProperty property,
         /// </summary>
         /// <param name="property"> The property. </param>
         /// <param name="comparer"> The comparer, or <c>null</c> to remove any previously set comparer. </param>
+        [Obsolete("Use SetKeyValueComparer. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         public static void SetStructuralValueComparer([NotNull] this IMutableProperty property, [CanBeNull] ValueComparer comparer)
-            => property.AsProperty().SetStructuralValueComparer(comparer, ConfigurationSource.Explicit);
+            => property.SetKeyValueComparer(comparer);
     }
 }

src/EFCore/Extensions/PropertyExtensions.cs

@@ -310,19 +310,9 @@ public static ValueComparer GetKeyValueComparer([NotNull] this IProperty propert
         /// <param name="property"> The property. </param>
         /// <param name="fallback"> If true, then the key comparer is returned when the structural comparer is not set. </param>
         /// <returns> The comparer, or <c>null</c> if none has been set. </returns>
+        [Obsolete("Use GetKeyValueComparer. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         public static ValueComparer GetStructuralValueComparer([NotNull] this IProperty property, bool fallback = true)
-        {
-            Check.NotNull(property, nameof(property));
-
-            var comparer = (ValueComparer)property[CoreAnnotationNames.StructuralValueComparer];
-
-            return fallback
-                ? comparer
-                ?? (ValueComparer)property[CoreAnnotationNames.KeyValueComparer]
-                ?? (ValueComparer)property[CoreAnnotationNames.ValueComparer]
-                ?? property.FindTypeMapping()?.StructuralComparer
-                : comparer;
-        }
+            => property.GetKeyValueComparer(fallback);
 
         /// <summary>
         ///     Creates a formatted string representation of the given properties such as is useful

src/EFCore/Metadata/Builders/IConventionPropertyBuilder.cs

@@ -403,6 +403,7 @@ bool CanSetValueGenerator(
         ///     The same builder instance if the configuration was applied,
         ///     <c>null</c> otherwise.
         /// </returns>
+        [Obsolete("Use HasKeyValueComparer. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         IConventionPropertyBuilder HasStructuralValueComparer([CanBeNull] ValueComparer comparer, bool fromDataAnnotation = false);
 
         /// <summary>
@@ -414,6 +415,7 @@ bool CanSetValueGenerator(
         /// <returns>
         ///     <c>true</c> if the given <see cref="ValueComparer" /> can be configured for this property.
         /// </returns>
+        [Obsolete("Use CanSetStructuralValueComparer. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         bool CanSetStructuralValueComparer([CanBeNull] ValueComparer comparer, bool fromDataAnnotation = false);
     }
 }

src/EFCore/Metadata/Internal/CoreAnnotationNames.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Generic;
 
 namespace Microsoft.EntityFrameworkCore.Metadata.Internal
@@ -139,6 +140,7 @@ public static class CoreAnnotationNames
         ///     any release. You should only use it directly in your code with extreme caution and knowing that
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
+        [Obsolete("Use KeyValueComparer. Starting with EF Core 5.0, key comparers must implement structural comparisons and deep copies.")]
         public const string StructuralValueComparer = "StructuralValueComparer";
 
         /// <summary>
@@ -252,7 +254,9 @@ public static class CoreAnnotationNames
             ValueConverter,
             ValueComparer,
             KeyValueComparer,
+#pragma warning disable 618
             StructuralValueComparer,
+#pragma warning restore 618
             AfterSaveBehavior,
             BeforeSaveBehavior,
             QueryFilter,

src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs

@@ -553,37 +553,6 @@ public virtual bool CanSetKeyValueComparer([CanBeNull] ValueComparer comparer, C
                     && Metadata.CheckValueComparer(comparer) == null)
                 || Metadata.GetKeyValueComparer(fallback: false) == comparer;
 
-        /// <summary>
-        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
-        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
-        ///     any release. You should only use it directly in your code with extreme caution and knowing that
-        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-        /// </summary>
-        public virtual InternalPropertyBuilder HasStructuralValueComparer(
-            [CanBeNull] ValueComparer comparer, ConfigurationSource configurationSource)
-        {
-            if (configurationSource.Overrides(Metadata.GetStructuralValueComparerConfigurationSource())
-                || Metadata.GetStructuralValueComparer(fallback: false) == comparer)
-            {
-                Metadata.SetStructuralValueComparer(comparer, configurationSource);
-
-                return this;
-            }
-
-            return null;
-        }
-
-        /// <summary>
-        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
-        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
-        ///     any release. You should only use it directly in your code with extreme caution and knowing that
-        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-        /// </summary>
-        public virtual bool CanSetStructuralValueComparer([CanBeNull] ValueComparer comparer, ConfigurationSource? configurationSource)
-            => (configurationSource.Overrides(Metadata.GetStructuralValueComparerConfigurationSource())
-                    && Metadata.CheckValueComparer(comparer) == null)
-                || Metadata.GetStructuralValueComparer(fallback: false) == comparer;
-
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -975,7 +944,7 @@ bool IConventionPropertyBuilder.CanSetKeyValueComparer(ValueComparer comparer, b
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
         IConventionPropertyBuilder IConventionPropertyBuilder.HasStructuralValueComparer(ValueComparer comparer, bool fromDataAnnotation)
-            => HasStructuralValueComparer(
+            => HasKeyValueComparer(
                 comparer, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention);
 
         /// <summary>
@@ -985,7 +954,7 @@ IConventionPropertyBuilder IConventionPropertyBuilder.HasStructuralValueComparer
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
         bool IConventionPropertyBuilder.CanSetStructuralValueComparer(ValueComparer comparer, bool fromDataAnnotation)
-            => CanSetStructuralValueComparer(
+            => CanSetKeyValueComparer(
                 comparer, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention);
     }
 }