What changed in .NET 6 that would affect JsonElement Equals behavior?

[jeffijoe]

I wasn't really sure what category to use for the issue type or whether this is even the right place to bring it up, but I checked the breaking changes list for .NET 6 and I don't see any mention of changing equality checking behavior for JsonElement.

I'm not certain whether this is specifically an issue for comparing System.Text.Json.JsonElement objects, or whether it's something deeper, but essentially the following test passes on .NET 5 and fails for me on .NET 6:

var jsonElement = JsonSerializer.Deserialize<JsonElement>("true");
var cloned = jsonElement.Clone();

jsonElement.Equals(cloned).Should().BeTrue();

When debugging this, I found that in ValueType.cs, the following check evaluates to true in .NET 5 and false in .NET 6. thisResult and thatResult are JsonDocument instances.

if (!thisResult.Equals(thatResult))
{
    // This path is entered on .NET 6
    return false;
}

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I wasn't really sure what category to use for the issue type or whether this is even the right place to bring it up, but I checked the breaking changes list for .NET 6 and I don't see any mention of changing equality checking behavior for JsonElement.

I'm not certain whether this is specifically an issue for comparing System.Text.Json.JsonElement objects, or whether it's something deeper, but essentially the following test passes on .NET 5 and fails for me on .NET 6:

var jsonElement = JsonSerializer.Deserialize<JsonElement>("true");
var cloned = jsonElement.Clone();

jsonElement.Equals(cloned).Should().BeTrue();

When debugging this, I found that in ValueType.cs, the following check evaluates to true in .NET 5 and false in .NET 6. thisResult and thatResult are JsonDocument instances.

if (!thisResult.Equals(thatResult))
{
    // This path is entered on .NET 6
    return false;
}

Author:	jeffijoe
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

JsonElement is a struct that does not implement equality, as such the Equals operation performs a point-by-point comparison of the struct's internal fields. The reason that in .NET 6 the result has changed to false is because of the different value of the internal IsDisposable property, which was changed by #42538. I don't believe this is a breaking change, since JsonElement and JsonDocument are not meant to support equality. You could try using one of the ValueEquals methods instead, although they can only be used to compare against raw JSON strings.

[jeffijoe]

That makes sense, I was trying to find out what was causing it to change all of a sudden.

Thanks for the response!

[svick]

JsonElement is a struct that does not implement equality

How is the user supposed to figure that out? Just from Equals(object) not being listed in API reference? Maybe there should be an analyzer that warns about this?

[stephentoub]

I don't believe this is a breaking change, since JsonElement and JsonDocument are not meant to support equality.

I think it is; the methods are there and documented and callable and return a value. Just because the struct doesn't override the method doesn't mean the method isn't still part of the contract.

Why haven't we overridden it to return something meaningful?

[jeffijoe]

@svick @stephentoub yes my initial assumption was that it's a struct, so if I clone it, it'll have the same values and thus be equal to its' source.

[eiriktsarpalis]

Why haven't we overridden it to return something meaningful?

I'm not sure if this was considered back when the types were being designed, @bartonjs might be able to comment on that.

Just because the struct doesn't override the method doesn't mean the method isn't still part of the contract.

True, but my impression is that all structs that define a meaningful concept of equality must also implement IEquatable<Self>. I don't think this meets the bar for servicing, and changing it in .NET 7 could also be considered a further breaking change. If we do change it I think we should have the types implement IEquatable as well.

[eiriktsarpalis]

To demonstrate why this is not a regression, consider the following example:

using System;
using System.Text.Json;

var jsonElement = JsonDocument.Parse("true").RootElement;
var cloned = jsonElement.Clone();

Console.WriteLine(jsonElement.Equals(cloned));

The above prints False in both net5.0 and net6.0. The reason why we're seeing divergence in the behavior of the original example is a performance optimization in the parser implementation that caches JsonDocument instances with IsDisposable set to true:

https://github.com/dotnet/runtime/pull/42538/files#diff-10d329fa2ffa433633001d2b0875be4fcfba133fb63ba6df3222235e23bfa3e1R588-R609

[stephentoub]

To demonstrate why this is not a regression

Are there no scenarios where it produced the "right" answer in .NET 5? Just because it doesn't always produce the right answer doesn't mean there aren't scenarios where it did and where devs could have taken a dependency on it.

Regardless, it's very strange that a struct's Equal would be invalid. Are there other cases of that in our core libraries, where a public struct's equality doesn't provide some definition of equality?

[eiriktsarpalis]

Are there no scenarios where it produced the "right" answer in .NET 5? Just because it doesn't always produce the right answer doesn't mean there aren't scenarios where it did and where devs could have taken a dependency on it.

I guess it depends on what the perceived definition of equality is. There more I look into this the more it seems that the answer is "wrong" for most useful applications:

var jsonElement = JsonDocument.Parse("[true,true]").RootElement;
Console.WriteLine(jsonElement[0].Equals(jsonElement[1])); // False

var jsonElement1 = JsonDocument.Parse("{}").RootElement;
var jsonElement2 = JsonDocument.Parse("{}").RootElement;
Console.WriteLine(jsonElement1.Equals(jsonElement2)); // False

when run in both .NET 5 and .NET 6.

Regardless, it's very strange that a struct's Equal would be invalid. Are there other cases of that in our core libraries, where a public struct's equality doesn't provide some definition of equality?

A quick reflection-based query seems to indicate we have ~200 public structs like that only in System.Private.CoreLib:

typeof(int).Assembly.GetTypes()
     .Where(t => t.IsPublic && t.IsValueType)
     .Where(t => t.GetMethod("Equals", new Type[] { typeof(object) }).DeclaringType != t)
     .Count()

[stephentoub]

A quick reflection-based query seems to indicate we have ~200 public structs like that only in System.Private.CoreLib

First, 142 of those 202 results are enums.

Second, most of the remaining ones are in InteropServices and CompilerServices. And of the ones that aren't, like struct enumerator types, there's little-to-no expectation that comparison of those would be meaningful, there's little-to-no reason to try to store them in a dictionary, etc.

Third, just because a struct doesn't override Equals doesn't mean its Equals is wrong; the base implementation can be doing the 100% correct thing, albeit more slowly in some cases than would be ideal. System.Reflection.InterfaceMapping's Equals will do the "right thing", just comparing each of the fields. KeyValuePair's Equals will do the "right thing", just comparing each of the fields. Etc.

JsonDocument, JsonElement, as a user, I fully expect to be able to check those for equality, they provide Equals, there's no documentation about Equals not doing the "right thing", there's no attribution on them that will produce a compilation warning suggesting I shouldn't be using them, etc.

Regardless of whether this is classified as a regression or a breaking change, there's an issue here, IMHO.

[eiriktsarpalis]

We could consider adding equality (and implement IEquatable<T> while we're at it) for these types, but it is not clear to me what the definition of equality should be for JSON documents: should it check for byte-for-byte UTF8 equality? should indented documents be equal to their minified equivalents? should objects containing permutations of keys be considered equal?

There are types for which equality simply does not make sense; the fact that every type inherits an Equals method is a historical artifact that we need to live with when designing such types. In the case of classes we just default to reference equality. It's not clear to me what the approach should be in the case of structs. I took a look at Framework Design Guidelines but couldn't find any guidance on the matter. Perhaps always returning false is one possible approach?

[jeffijoe]

should indented documents be equal to their minified equivalents?

I think semantic equality is more involved and I wouldn't expect that from a .Equals, maybe from a EqualityComparer implementation. But it did come as a surprise that a method named .Clone() returns a struct that wouldn't be considered equal to its' source. 😅

Does a .Clone() result in a JsonDocument that is byte-for-byte UTF8 equivalent to its' source? If so, then I think that would make sense?

[svick]

@eiriktsarpalis

Perhaps always returning false is one possible approach?

That would violate the requirement that x.Equals(x) always returns true (which is mentioned in Notes for Inheritors in the docs for object.Equals()).

[eiriktsarpalis]

Technically though the argument passed in the Equals method is a boxed copy of the struct and thus one could argue not "the same as" x.

[svick]

@eiriktsarpalis Even if you interpret it that way, it would still require the following code to print True and your suggested implementation wouldn't:

JsonElement element = ...;
object box = element;
Console.WriteLine(box.Equals(box));

[eiriktsarpalis]

Fair enough. We could consider implementing byte-for-byte equality equality comparison in the future although that is bound to disappoint users expecting a different brand of document equality. I feel that changing nothing at all is a valid approach too.

[KubaSzostak]

I would love to have meaningful JsonElement.Equals() results. Consider the following example:

using System.Text.Json;

var json = "{\"value\": 123}";
var dict1 = JsonSerializer.Deserialize<Dictionary<string, object>>(json);
var dict2 = JsonSerializer.Deserialize<Dictionary<string, object>>(json);
var value1 = dict1["value"];
var value2 = dict2["value"];
Console.WriteLine(value1.Equals(value2)); // False

This is really strange that the same JSON deserialized by the same deserializer gives values that are not equal. I also heard that you should always override Equals() for custom structs for performance reasons.

[scharnyw]

How about providing a JsonElement.Equals(JsonElement, JsonComparison) overload that gives users some flexibility in choosing the desired equality behavior, then the JsonElement.Equals(object) override can default to default(JsonComparison). Basically something like StringComparison. Then for consumers it will be very clear from the signatures what the actual behaviors are.

As it currently stands JsonElement.Equals(object) is not only useless but gives totally unexpected and inconsistent results, which leads to subtle bugs in consumer code. You may as well override it and throw NotSupportedException to fail fast.

[krwq]

@scharnyw it's a valid approach but needs design/experimentation and implementation. would you be willing to contribute? I'm not quite sure about the original design if this was intentionally left out or missed out by accident. I don't want to give anyone false hopes we will pick this up anytime soon given we have plenty of issues in this area and relatively little people working on it...

[scharnyw]

@krwq I may do some experimentation when I have time available and if this issue remains unresolved. But since I have next-to-no knowledge of the internal workings of STJ I don't think I'll be a good candidate for contribution.

[krwq]

As with any feature even if you know some internals like us we'll still need to learn more of them if we want to provide some good design/implementation since most of the people don't know every bit of code - our code base is rather large overall and usually we know only code we touched or reviewed. It's usually good to start with public APIs and review what will likely need changes and then that should immediately give you idea on what are the corner cases. Then you do some prototyping - as much testing as it makes sense at this stage, then API review/design review if needed and after that final implementation and possibly reiteration.

Here I'd personally review current behavior and most likely make breaking change if current implementation is mostly useless and document it. It will likely need API review for recommendation if they prefer breaking change or new APIs - some possible design could be provided here but also add recommendation that breaking change might be better option.

As you see that requires a bit of time and given all issues we can't fix them all so we will need to pick based on cost/value of given API or change. It's not that we don't want to fix this issue - we do - we just have other issues which are likely higher priority than this. Usually we go through list of issues in the beginning of the release (or very end of the previous one) which would be soon from now and decide based on that. My initial thought is that this one likely won't meet the bar yet (unless there is a series of similar issues and we would tackle all of them) and therefore we ask for help here.

Hope that helps.

[eiriktsarpalis]

Closing as duplicate of #33388. We can debate there whether we want JsonElement to implement IEquatable or have a dedicated DeepEquals method as is the case with JsonNode.