Report a struct once when a collection repeats it

CHANGELOG.md

@@ -36,7 +36,8 @@ Breaking changes are marked **BREAKING**.
   - A collection that a property holds is enumerated later than before: after the objects at its own level and above are validated, not when the property is read. A lazy sequence runs later. A change that a `Validate` method makes to the collection is now seen. An item whose `Validate` adds to its own list no longer throws `Collection was modified`. When a collection throws, the exception is the same, but the result list holds different partial results.
 - The NuGet package title now reads "Recursive DataAnnotations Validation". It was misspelled "Recurisive".
 - The NuGet package now has a description. It showed "Package Description" before.
-- Known limit: an invalid struct that an item collection returns by enumeration and also through one of its properties is reported twice, with two member names, for example `Value[0][0].Text` and `Value[0].Array[0].Text` for an `ArraySegment<T>`. The validator cannot match two copies of a struct by reference. A struct that two properties hold was already reported twice. Two properties that return the same struct, such as a property and a computed copy of it, now report it twice too. Objects of a class are reported once.
+- **BREAKING** A struct that the validator reaches by two routes is reported once where the validator can tell. For a collection that is an item or the root object, the properties that framework types declare, such as `ArraySegment<T>.Array`, `LinkedList<T>.First` and `Dictionary<TKey, TValue>.Values`, are no longer walked, as for a collection that a property holds. A collection that a second route reaches is enumerated again, so an item added in the meantime is found, but its struct items are not reported again. Change any code that counts the results: `Value[0].Array[0].Text`, `Value[0].First.Value.Text` and `Value[0].Values[0].Text` are gone, and so is `Second[0].Text` for an array that `First` also holds. An object that only such a property reached is no longer validated, such as an item of the array outside an `ArraySegment<T>`, or the comparer of a dictionary. A jagged array of simple values, such as `int[][]`, is no longer enumerated, because it cannot hold an invalid object.
+- Known limit: two properties that each return the same struct, such as a property and a computed copy of it, report it twice, with two member names. The validator cannot match two copies of a struct by reference. Objects of a class are reported once.
 - README: say what `Validator.TryValidateObject` leaves out, show the usage as C#, and tell the `Tuple` classes, which are walked, from value tuples, which are not.
 
 ### Fixed

README.md

@@ -35,7 +35,7 @@ The [`[SkipRecursiveValidation]`](https://github.com/tgharold/RecursiveDataAnnot
 ### Shared objects, cycles and computed properties
 
 - Each object is validated once, even when several properties point to it. This also stops cycles, such as a child that points back to its parent. The error names the shortest path to the object (see "Order of results" below).
-- A struct is the exception to the first rule. The validator cannot tell that two copies of a struct are the same value, so it reports an invalid struct once for each route that reaches it. For example, two properties that hold the same array of structs report each struct twice, so do two properties that return the same struct, such as a property and a computed copy of it, and so does a `List<object>` that holds an `ArraySegment<T>` of structs, because the segment returns its items by enumeration and through its `Array` property. The error is not lost, but the result list has a duplicate with a different member name. For a collection type of your own, mark the property that repeats the items with `[SkipRecursiveValidation]`.
+- A struct is the exception to the first rule. The validator cannot tell that two copies of a struct are the same value. It does remember each collection, so a struct in an array that two properties hold is reported once, at the shortest path. Two properties that each return the struct, such as a property and a computed copy of it, give two separate values, and the validator reports each one. The error is not lost, but the result list has a duplicate with a different member name. Mark the copy with `[SkipRecursiveValidation]`.
 - Objects are compared by reference. Two separate objects that are `Equals` to each other, such as records with the same values or entities with the same `Id`, are each validated.
 - Only public instance properties are walked. A static property holds data of the type, not of your object, so it is not walked, as `Validator` ignores it too.
 - A property that builds a new object on each read, such as `public Money Zero => new Money(0)`, could make the walk go on forever. So when an object's type overrides `Equals`, and the object equals an object of the same type, a base type or a derived type on its own path from the root, the validator checks that object's own attributes but does not walk into its properties from there. The check uses the shortest path to the object. A longer path that does not pass an equal object walks into it. An invalid object that every path reaches through an equal ancestor is not reached. This can happen when a shared object, such as a customer, is reached first through an equal ancestor.
@@ -60,7 +60,7 @@ An object that several paths reach is validated once, and its errors name the sh
 
 - The items of a collection that a property holds are validated. The error names the property and the index of the item: `Items[1].Name`.
 - An item that is itself a collection is validated too, at each level: `Matrix[0][2].Name`. This holds for lists, arrays, sets, dictionaries and your own collection types. A dictionary is enumerated as `KeyValuePair` items, so a value that is an object is reported as `Map[0].Value.Name`. A value that is a struct is walked when it has something to validate, as it is when a property holds it (see "Structs" below).
-- An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items.
+- An item that is a collection is validated as an object first, so its own attributes and `IValidatableObject.Validate` run, then its items. The properties that a framework type declares on it, such as `ArraySegment<T>.Array`, `LinkedList<T>.First` or `Dictionary<TKey, TValue>.Values`, are not walked, because they repeat the items. A collection that a property holds is treated the same way. The properties that your own collection type adds are walked.
 - A collection of simple values, such as `List<int>` or `string[]`, is not enumerated, because it cannot hold an invalid object.
 - A collection that you pass to the validator as the root object is validated as an object, then its items are validated. The error starts with the index of the item: `[1].Name`.
 - A collection that is a struct, such as `ImmutableArray<T>`, is validated when a property holds it and when it is an item of another collection: `Lines[0].Sku`.

src/RecursiveDataAnnotationsValidation/Extensions/TypeExtensions.cs

@@ -152,10 +152,21 @@ public static bool IsWalked(this PropertyInfo property)
         /// <see cref="IsWalked"/>). A static property is left out: it holds data of the type, not
         /// of the object, and Validator ignores it too. Type.GetProperties() with no arguments
         /// would return public static properties as well.
+        /// For a collection, the properties that a framework type declares are left out too, such as
+        /// Array.SyncRoot, LinkedList.First or Dictionary.Values. The validator enumerates the
+        /// collection, and those properties repeat its items, so a struct item would be reported once
+        /// for each route. A collection that a property holds is only enumerated, so an item or root
+        /// collection is now walked the same way. The properties that a collection type of your own
+        /// adds are still walked.
         /// </summary>
         public static List<PropertyInfo> GetWalkedProperties(this Type type)
         {
-            return type.GetProperties(BindingFlags.Public | BindingFlags.Instance).Where(IsWalked).ToList();
+            var isCollection = typeof(IEnumerable).IsAssignableFrom(type);
+
+            return type.GetProperties(BindingFlags.Public | BindingFlags.Instance)
+                .Where(property => property.IsWalked()
+                    && !(isCollection && IsInSystemNamespace(property.DeclaringType)))
+                .ToList();
         }
 
         /// <summary>

src/RecursiveDataAnnotationsValidation/RecursiveDataAnnotationValidator.cs

@@ -147,6 +147,13 @@ private sealed class GraphWalk
             //every object that was validated but not walked, because it Equals an ancestor on its path
             private readonly HashSet<object> stoppedObjects = new HashSet<object>(ObjectReferenceComparer.Instance);
 
+            //every collection that was enumerated, compared by reference. A collection that a second
+            //route reaches is enumerated again, so an item that was added in the meantime, such as by a
+            //Validate method, is found. Its struct items are skipped the second time: a struct has no
+            //identity of its own, so queuedObjects cannot tell that the copy is the same struct, and it
+            //would be reported once for each route. Its class items are dropped by queuedObjects.
+            private readonly HashSet<object> enumeratedCollections = new HashSet<object>(ObjectReferenceComparer.Instance);
+
             private readonly ICollection<ValidationResult> validationResults;
             private readonly IServiceProvider serviceProvider;
             private readonly IDictionary<object, object> validationContextItems;
@@ -229,14 +236,17 @@ EqualsAncestor ancestors
             }
 
             //Queues each item of a collection. The path of an item is the path of the collection
-            //plus its index, and its depth is itemDepth.
+            //plus its index, and its depth is itemDepth. A collection that was enumerated before skips
+            //its struct items (see enumeratedCollections).
             private void EnqueueItems(
                 IEnumerable items,
                 PathStep collectionPath,
                 int itemDepth,
                 EqualsAncestor ancestors
                 )
             {
+                var enumeratedBefore = !enumeratedCollections.Add(items);
+
                 var arrayIndex = -1;
                 foreach (var item in items)
                 {
@@ -245,6 +255,7 @@ EqualsAncestor ancestors
                     //NOTE: Possibly should have a separate case for Dictionary which reports on the key
 
                     if (item == null) continue;
+                    if (enumeratedBefore && item.GetType().IsValueType) continue;
                     Enqueue(item, collectionPath, null, arrayIndex, itemDepth, ancestors);
                 }
             }
@@ -291,24 +302,22 @@ private bool Walk(WorkItem item)
 
                 //A collection that gets here is the root object or an item of another collection. It is
                 //validated as an object above, so its own attributes run. Then its items are queued,
-                //before its properties. An object that the collection also returns from a property,
-                //such as Array.SyncRoot, is then reported at its index (Value[0][0], or [0] for the root)
-                //and not through the property (Value[0].SyncRoot[0]). A collection that a property
-                //holds never gets here: it waits in the queue as a step of its own (see below).
+                //before its properties. A collection that a property holds never gets here: it waits in
+                //the queue as a step of its own (see below).
                 //A collection of leaf types is skipped, like a collection that a property holds. A default
                 //struct, such as an ImmutableArray nobody set, is skipped: it holds nothing and enumerating it throws.
-                var enumeratedItems = false;
                 if (obj is IEnumerable items
                     && !type.IsCollectionOfLeafType()
                     && !obj.IsDefaultStruct())
                 {
-                    enumeratedItems = true;
                     EnqueueItems(items, item.Path, item.Depth + 1, ancestors);
                 }
 
-                //IsWalked leaves out properties declared by framework types that throw or never end when
-                //read, such as Uri.Segments on a relative Uri, DirectoryInfo.Root, or the properties of
-                //a Thread or Process read from the wrong thread or process (see IsUnsafeToWalk)
+                //GetWalkedProperties leaves out properties declared by framework types that throw or never
+                //end when read, such as Uri.Segments on a relative Uri, DirectoryInfo.Root, or the properties
+                //of a Thread or Process read from the wrong thread or process (see IsUnsafeToWalk). For a
+                //collection, it also leaves out the properties that framework types declare, such as
+                //Array.SyncRoot or LinkedList.First, because they repeat the items that were just queued.
                 var properties = type.GetWalkedProperties();
 
                 foreach (var property in properties)
@@ -332,10 +341,6 @@ private bool Walk(WorkItem item)
                             continue;
 
                         case IEnumerable asEnumerable:
-                            //an item that was enumerated above can return itself from a property, such as
-                            //Array.SyncRoot, and enumerating it a second time would find nothing new
-                            if (enumeratedItems && ReferenceEquals(value, obj)) continue;
-
                             //the property is one level and the index of each item is another
                             queue.Enqueue(new WorkItem(
                                 asEnumerable,

test/RecursiveDataAnnotationsValidation.Tests/Extensions/TypeExtensionsTests.cs

@@ -123,6 +123,10 @@ public class MixedSequence : IEnumerable<int>, IEnumerable<Child>
         [Theory]
         [InlineData(typeof(byte[]))]
         [InlineData(typeof(int[,]))]
+        // An int[] is a sealed collection of ints. Its only property of a reference type,
+        // Array.SyncRoot, is declared by a framework type, so it is not walked, and int[] is a
+        // leaf type. A jagged array of ints holds nothing to validate.
+        [InlineData(typeof(int[][]))]
         [InlineData(typeof(int?[]))]
         [InlineData(typeof(List<int>))]
         [InlineData(typeof(HashSet<Guid>))]
@@ -156,7 +160,6 @@ public void Collection_of_leaf_type_is_detected(Type type)
         [Theory]
         [InlineData(typeof(Child[]))]
         [InlineData(typeof(object[]))]
-        [InlineData(typeof(int[][]))]
         [InlineData(typeof(List<Child>))]
         [InlineData(typeof(Dictionary<string, Child>))]
         [InlineData(typeof(Dictionary<Child, int>))]
@@ -274,6 +277,45 @@ public sealed class SealedStructBagHolder
             public ChildBagStruct Bag { get; set; }
         }
 
+        public class OwnedLinkedList : LinkedList<Child>
+        {
+            public Child Owner { get; set; }
+        }
+
+        // A collection is enumerated, so the properties that a framework type declares on it are
+        // not walked: they repeat the items, such as LinkedList.First and Last. The properties that
+        // your own collection type adds are walked.
+        [Fact]
+        public void Framework_properties_of_a_collection_are_not_walked()
+        {
+            Assert.Empty(typeof(LinkedList<Child>).GetWalkedProperties());
+            Assert.Equal(
+                new[] { nameof(OwnedLinkedList.Owner) },
+                typeof(OwnedLinkedList).GetWalkedProperties().ConvertAll(p => p.Name));
+        }
+
+        // A framework type that is not a collection keeps its properties walked, so the objects
+        // in a Tuple or a KeyValuePair are reached.
+        [Fact]
+        public void Framework_properties_of_a_type_that_is_not_a_collection_are_walked()
+        {
+            Assert.Equal(
+                new[] { nameof(Tuple<Child, int>.Item1) },
+                typeof(Tuple<Child, int>).GetWalkedProperties().ConvertAll(p => p.Name));
+            Assert.Equal(
+                new[] { nameof(KeyValuePair<string, Child>.Value) },
+                typeof(KeyValuePair<string, Child>).GetWalkedProperties().ConvertAll(p => p.Name));
+        }
+
+        // Check 4 of IsLeafType uses the walked properties. ArraySegment<int> has an Array property
+        // of type int[], which used to count as walked. It holds only ints, so it is a leaf type.
+        [Fact]
+        public void Array_segment_of_values_is_a_leaf_type()
+        {
+            Assert.True(typeof(ArraySegment<int>).IsLeafType());
+            Assert.False(typeof(ArraySegment<Child>).IsLeafType());
+        }
+
 #if NET8_0_OR_GREATER
         public class ImmutableArrayHolder
         {

test/RecursiveDataAnnotationsValidation.Tests/StructCollectionPropertyTests.cs

@@ -219,8 +219,7 @@ public class SkippedBag
 
         // The items of a struct collection are objects of a class, so the reference check applies to
         // them as to any other object: one that two properties share is validated, and reported, once.
-        // Only an item that is itself a struct is reported once for each route (see
-        // StructItemsReportedTwiceTests).
+        // A struct item has no identity of its own (see StructsReachedByTwoRoutesTests).
         [Fact]
         public void Object_shared_by_two_struct_collection_properties_is_reported_once()
         {