Object graph traversal fails when encountering objects of type System.Reflection.Emit.SignatureHelper

[abeham]

Calling GetHashCode() to see if it's contained in the HashSet of previously seen objects results in a NullReferenceException.

This type was introduced into the object graph, because I tried to add protocol buffer's ExtensionRegistry as a field to the ExternalEvaluator. I can find another, slightly slower solution for my problem, but the problem remains that object graph traversal is not completely safe.

Line 50 in ObjectExtensions.cs fails:

if (o != null && !objects.Contains(o) && !ExcludeType(o.GetType()))

Excluding this type would require swapping the second and the third check. I don't know if this has performance implications, but I suspect that o.GetType().GetHashCode() is slower than o.GetHashCode().

[mkommend]

r8309: Minor modifications in CollectGASample unit test.

[mkommend]

r8310:

• Corrected GetAllFields extension methods to return fields across the hierarchy only once.
• Changed filtering of object and types to first check for excluded types.
• Removed unnecassary comparer for excluded members HashSet.
• Corrected outdated comments
• Added SignatureHelper to the excluded types.

Overall the performance increased due to the changes above and the signature helper should not cause problems anymore. Btw, thanks for spotting this defect.

[abeham]

Thx for fixing this!

I think it's not correct to remove the ReferenceEqualityComparer. We always want to check objects by reference and we need to ignore any object's overriden Equals and GetHashCode methods. MSDN confirms that the default EqualityComparer<T> (if that's not somehow different from the default equality comparer for sets) makes use of these overrides: "The Default property checks whether type T implements the System.IEquatable<T> interface and, if so, returns an EqualityComparer<T> that uses that implementation. Otherwise, it returns an EqualityComparer<T> that uses the overrides of Object.Equals and Object.GetHashCode provided by T."

Also I don't think DeclaredOnly is correct, according to MSDN: "Specifies that only members declared at the level of the supplied type's hierarchy should be considered. Inherited members are not considered.". Why do we want to ignore inherited members?

[mkommend]

Replying to abeham:

Thx for fixing this!

I think it's not correct to remove the ReferenceEqualityComparer. We always want to check objects by reference and we need to ignore any object's overriden Equals and GetHashCode methods. MSDN confirms that the default EqualityComparer<T> (if that's not somehow different from the default equality comparer for sets) makes use of these overrides: "The Default property checks whether type T implements the System.IEquatable<T> interface and, if so, returns an EqualityComparer<T> that uses that implementation. Otherwise, it returns an EqualityComparer<T> that uses the overrides of Object.Equals and Object.GetHashCode provided by T."

The comparer was only used to determine the if members should be ignored in the object graph traversal. As the HashSet containing the objects to ignore is provided as parameter, the user can control how the objects should be compared by providing another comparer.

Also I don't think DeclaredOnly is correct, according to MSDN: "Specifies that only members declared at the level of the supplied type's hierarchy should be considered. Inherited members are not considered.". Why do we want to ignore inherited members?

The MSDN also states only public & protected fields are returned and to get all private fields we have to follow the inheritance chain upwards. As we do so, we do not need to consider fields declared in base classes, as these are discovered by the recursive application of the method on its base type.