Object reference equality should be used when checking for circular references

[D3-LucaPiombino]

In Newtonsoft.Json\Src\Newtonsoft.Json\Serialization\JsonSerializerInternalWriter.cs(252):

private bool CheckForCircularReference(JsonWriter writer, object value, JsonProperty property, JsonContract contract, JsonContainerContract containerContract, JsonProperty containerProperty)

(Last commit: 70d3e93) the code:

if (_serializeStack.IndexOf(value) != -1)

should be replaced with:

if (_serializeStack.IndexOf(itemOnStack => object.ReferenceEquals(itemOnStack,value)) != -1)

(or a custom ReferenceEqualityComparer can be created and passed to the other overload)
because if the instance contained in value or any entry in the stack overrides object.Equals it can cause the code to report a false positive.

Test case:

        public class ValueContainer
        {
            public object Value;

            public override bool Equals(object obj)
            {
                if (obj == null)
                    return false;

                var otherContainer = obj as ValueContainer;

                if (otherContainer != null)
                    return EqualityComparer<object>.Default.Equals(Value, otherContainer.Value);

                return EqualityComparer<object>.Default.Equals(Value, obj);
            }

            public override int GetHashCode()
            {
                return EqualityComparer<object>.Default.GetHashCode(Value);
            }
        }

        [Test]
        public void ShouldCheckForCircularReferencesUsingReferenceEquality()
        {
            var source = new ValueContainer
            {
                Value = new object()
            };

            string json = JsonConvert.SerializeObject(source, Formatting.Indented);
        }

I think this behavior was not intended by the author, since checking for circular references in an object graph should only be about referential identity.

[ivanjh]

Duplicate of #214

[D3-LucaPiombino]

I have checked in an alternative implementation that allow to maintain the legacy behavior.
Can you take a look?

Thanks.

[JamesNK]

I prefer the current behavior which lets devs customize logic by overriding Equals.

Besides, this is a big breaking change.

[lookbusy1344]

This causes a lot of unnecessary calls to object.Equals when your classes have value semantics (which after all, Json.net doesn't care about, it's looking for referential loops). Even when ReferenceLoopHandling.Serialize is set the call to object.Equals still fires.

I have made myself a custom build using object.ReferenceEquals(), but can I suggest you either don't call _serializeStack.IndexOf(value) when the mode is Serialize, or have a flag to do reference equality, or allow us to decorate classes with a 'JsonEqualityComparer'?

The equality we want in our code is not necessarily the same we want for CheckForCircularReference.

[D3-LucaPiombino]

@johnsparrowuk
My point exactly. Thanks for explaining it so well.

@JamesNK
If you look at the pull request there were no breaking changes.
The behavior implemented was configurable (using the JsonSerializerSettings and a new ReferenceComparisonHandling property) and the default was to fallback on the current behavior.

[lookbusy1344]

@D3-LucaPiombino your changes look great, much better than my rush job. If it doesn't get pulled into the official repo I will switch to yours.

[JamesNK]

Added EqualityComparer to JsonSerializer 3cc797c