SimpleJSON.JSONXXX breaks Dictionary key semantics (Equals not reflexive) + mutable hash codes in value nodes (JSONBool/JSONString/JSONNumber)

[udlose]

Describe the bug
I noticed that using SimpleJSON.JSONBool as a dictionary key: after adding an instance as a key, ContainsKey(sameInstance) returns false even before any mutation. This appears to be because JSONBool.Equals(object) returns true only when compared to a boxed bool, and returns false when compared to a JSONBool (including itself).

Several Issues:

1. JSONBool.Equals is not reflexive (x.Equals(x) can be false), which breaks core .NET equality and collection semantics immediately.
2. JSONBool/JSONString/JSONNumber have hash codes derived from mutable state, so they’re unsafe in hash-based scenarios after mutation (including implicit hashing in LINQ
3. maybe-by-design???: cross-type equality (JSONNumber vs numeric primitives, JSONString vs string) is inherently non-symmetric, so it should either be avoided, carefully documented, or implemented via explicit comparisons/conversions rather than Equals

Mutable hash (JSONBool/JSONString/JSONNumber)
This can break any hash-based collection or algorithm if the node is mutated after being used as a key/element, for example:

• HashSet<JSONBool> membership can "lose" an item after mutation for the same reason a dictionary key can
  ​
• LINQ methods that build internal hash sets/lookups (e.g., Distinct, Except, Intersect, GroupBy, ToLookup) can behave inconsistently if elements/keys mutate while participating

To Reproduce
small repro:

using System;
using System.Collections.Generic;
using SimpleJSON;

public static class Program
{
    public static void Main()
    {
        const Int32 val = 1;
        SimpleJSON.JSONBool jsonBool = new SimpleJSON.JSONBool(true);
        Dictionary<SimpleJSON.JSONBool, Int32> dict = new Dictionary<SimpleJSON.JSONBool, Int32>();
        dict.Add(jsonBool, val);

        Int32 hash1 = jsonBool.GetHashCode();
        Console.WriteLine("hashcode: " + hash1);
        Console.WriteLine("containsKey: " + dict.ContainsKey(jsonBool));
        Console.WriteLine("containsValue: " + dict.ContainsValue(val));

        jsonBool.AsBool = false; // mutate the object in the dict to lose it
        Int32 hash2 = jsonBool.GetHashCode();
        Console.WriteLine("hashcode: " + hash2);
        Console.WriteLine("containsKey: " + dict.ContainsKey(jsonBool));
        Console.WriteLine("containsValue: " + dict.ContainsValue(val));
    }
}

actual output:

hashcode: 1
containsKey: False
containsValue: True
hashcode: 0
containsKey: False
containsValue: True

Expected behavior

• dict.ContainsKey(jsonBool) should return true immediately after dict.Add(jsonBool, val) when using the same instance as the lookup key.
• x.Equals(x) equals true

[danipen]

Is there any known bug caused by this issue?

[udlose]

Is there any known bug caused by this issue?

No, I was just experimenting :)

[danipen]

Hey @udlose regarding this issue, just for reference: simpleJSON upstream is here: https://github.com/willem88836/SimpleJson

I believe it's okay to keep our own version in this repo. However, you should be aware that the upstream exists just in case.

[udlose]

@danipen would you like me to look into fixing it in the upstream?

[danipen]

No problem. As you prefer. This is a simple class, so I think it's okay to use our version here. But if you prefer to fix the upstream changes too, that's fine as well. The choice is yours.

[udlose]

I looked up the repo owner and he hasn't been active on GH for several years. I'll just fix it here for now and maybe open an issue for him and fix it upstream in the near future.

[udlose]

@danipen did you want me to keep the SimpleJSON API identical to upstream? or are you ok correcting all of the issues with it?

[udlose]

@danipen bump ^^^

what I need to do to properly "correct the issues" is to make the public mutable properties readonly. The other thing that would help is to make the entire SimpleJSON implementation internal so none of the types are publicly exposed. To do that correctly, the best option IMO is to move SimpleJSON to a new shared project that is accessed by TextMateSharp and TextMateSharp.Grammars. You could add the InternalsVisibleTo attribute so that the SimpleJSON class is accessible to TextMateSharp.Grammars, BUT that would make all TextMateSharp internals visible to TextMateSharp.Grammars which isn't ideal. I would also create a new project for the new SimpleJSON unit tests I'm adding.

let me know your thoughts about all of this.

[danipen]

I think we can afford fixing it properly. The library is small, so feel free to correct the issues as you see fit, even if that means diverging from upstream. No need to strictly preserve the current API if it helps make the behavior correct and safer.