Prevent bug when reusing type ids in hashing

[tadeohepperle]

fixes #1066.

I first thought a simple restructuring approach like @jsdw mentioned in #1066 would work, but correct me if I'm wrong, I think it cannot solve the recursive type problem, because creating a new HashSet in get_type_id() will eventually run into an infinite loop.

Instead of using a HashSet for keeping track of the visited ids, we can use a HashMap that saves some info about if the type has been visited yet and also if a complete hash has been calculated yet.

I added a test to make sure it works (Without my code changes, it fails).

[jsdw]

because creating a new HashSet in get_type_id() will eventually run into an infinite loop.

What I meant was that get_type_id could be exposed to the rest of the module to get type IDs, and this would internally create a new HashSet or whatever.

And then "internally" (and not exposed to the rest of the crate) one could use a get_type_id_with_cache or whatever that looks like the current one does.

Maybe some stuff would end up in a different module or something to support that.

And the point of doing this would just be to guarantee that get_type_id always starts from a fresh state and doesn't re-use anything from other invocations which could change the result or make it worse :)

But anyways, maybe that's irrelevant now and I'll have a gander at the approach you took!

[jsdw]

Ooh, I like the idea of caching the hashes like this in general; good thinking!

There's maybe a small chance that the first 4 bytes of type_hash are actually [0,0,0,0], which would make that type look like the special recursive type that leads to an early return.

I also wonder; is it better to store the full [u8; HASH_LEN] type hash in the cache? We use more memory with this, but then don't need to hash it, and it means that the cache is truly caching the computed hashes for various types rather than caching something that's not actually the original hash we computed for a type. It feels "safer" to reuse the cache in other places if we know that it is storing the actual computed hashes over something a bit different.

Combining the two thoughts above, I wonder whether the cache should look like:

type Cache = HashMap<u32, CachedValue>;

enum CachedValue {
    Recursive,
    Hash([u8; HASH_LEN])
}

Where we insert CachedValue::Recursive initially and update that to a final hash when done. (if get_type_hash looks in the cache and sees a CachedValue::Recursive it can return with eg MAGIC_RECURSIVE_HASH_VALUE, which is functionally the same as right now where it would return with hash(0)).

I'm suggesting a new enum above over using Option just because it would be a little more self-documenting, but either works really!

[tadeohepperle]

We could definitely do that, thought about something like that as well, it is more secure at the cost of a little bit more memory. I will change it! :)

[jsdw]

That'd be great, and then perhaps also we could add a test which uses the cache across a couple of invocations of hashing the same type to comfirm that indeed you get the same hash back both times (first time caches the hash, second time just returns it straight off). This should even work if the type has a recursive definition in which is cool :)

Ultimately whether we pass a new cache or the same cache in to multiple calls, the results should all be the same.

[jsdw]

Nit: it's more like "in this case, the CachedHash::Recursive returns a fixed "magic" hash rather than recursing into the same type again" :)

[jsdw]

This looks great! Just one tiny suggestion to add a test showing that the cache is cahcing actual hashes and thus can safely be reused across multiple calls to hash the same types or whatever, but otherwise looks good to me!

[jsdw]

Can we also check that a_hash == b_hash; imo that's more iomportant than looking into the cached stuff :)

[tadeohepperle]

I don't think we want the hashes to be the same, because A and B are different structs, with different fields:

struct A { pub b: Box<B> }
struct B { pub a: Box<A> }

Maybe I understood you wrong, what did you mean?

[jsdw]

Ah, sorry yeah, I mean, it'd be nice to have a test like:

let a_type_id = registry.register_type(&meta_type::<A>()).id;

let a_hash1 = get_type_hash(&registry, a_type_id, &mut cache);
let a_hash2 = get_type_hash(&registry, a_type_id, &mut cache);

assert_eq!(a_hash1, a_hash2);

ie, we want to check that sharing the cache across different uses doesn't change the hashes that are produced for some type.