fix for incorrect implementation of KeyDeserialize for (T, U)

[0xForerunner]

I think this should be sufficient to fix the problem we were seeing. There is one open question here, and that is wether or not to provide the default implementation for KEY_LEN = 1. I think this is unwise as it will likely result in uncaught bugs for the end user. I recommend leaving it as it is in this PR.

[0xForerunner]

One other option we have is make an entirely separate trait KeyLen which provides access to the const KEY_LEN. We can then require the bounds:

impl<T: KeyDeserialize + KeyLen, U: KeyDeserialize> KeyDeserialize for (T, U)

This has the benefit of limiting the breaking changes only to projects who are very likely to be exposed to this bug.

Our project is a bit held up by this PR so it would be awesome to get this pushed through as soon as we're sure it's sound.

[maurolacy]

Thanks for the PR. I think this is good enough as it is.

Would just like to test it a bit more before approval / merging; which can be as soon as beginning next week.

[maurolacy]

LGTM.

[0xForerunner]

@maurolacy Thanks so much for your help! Really appreciate it. When do you think we can expect a new release on crates.io?

[webmaster128]

This is a breaking change since the trait is public

[0xForerunner]

Yes it is. Unfortunately there is no solution to this bug that does not introduce breaking changes. If you look at one of my earlier suggestions I mentioned that creating a new trait called KeyElems, which contains the const, and then using that as a trait bound for KeyDeserialize for (T, U) would make this change less braking for most users. I still think that's a good solution.

[webmaster128]

Thank you! We'll check next week how we can ensure the 1.x release series remains free of breaking changes. It might take a while until this can be released. So I recommend using the main branch from GitHub as a dependency to unblock you.

[0xForerunner]

Will do. If you go with the new trait solution, then the changes will only be braking specifically to users who are directly exposed to the bug. Isn't it a good thing that they will now receive a compiler error, forcing them to address the issue? That is definitely the route I would take.

[webmaster128]

Yeah it might be the best option. I need to look into it with a fresh head again. For now I just want to make sure this does not break happy users when released.

[maurolacy]

This breaks happy users, but the fix is straightforward. I think if we document this in the MIGRATING file, we should be fine.

[webmaster128]

Thank you for your contribution, @ewoolsey & @maurolacy and sorry for my curt comment before. The change makes sense but given that this is a breaking change, I wonder what the root cause of the issue is and if there alternative ways to fix it.

Do I understand correctly that there are fundamentally two design decitions that made this change necessary?

1. When serializing tuples in keys, they are flattened
2. It is possible to use () as a key which has not even a size when serialized

Both of them are questionmarks to me. Why should (U, V, H) have 6 key elements instead of 3 when each of them is a tuple? Should this be nested the same way the Rust type is? And then large parts of the PR look like special handling for (). I don't understand why () is allowed as a key type in the fist place. I mean I can create a Map<(), String> but it does not make any sense. Did we go too far with the complexity here?

Anyway, given the state of things, the PR looks like great work.

[maurolacy]

Thank you for your contribution, @ewoolsey & @maurolacy and sorry for my curt comment before. The change makes sense but given that this is a breaking change, I wonder what the root cause of the issue is and if there alternative ways to fix it.

Do I understand correctly that there are fundamentally two design decitions that made this change necessary?

1. When serializing tuples in keys, they are flattened
2. It is possible to use () as a key which has not even a size when serialized

2 is not an issue here. The issue is with nested tuples used as keys. I don't understand the exact details, but basically, when nesting tuples, they are being conflated. Probably because of flattening, as you say. And a way around it (given that nested tuples are in fact being used) is this PR.

If you find an alternative (non-breaking) way to address this, we can in fact revert these changes and go for the non-breaking way.

Both of them are questionmarks to me. Why should (U, V, H) have 6 key elements instead of 3 when each of them is a tuple? Should this be nested the same way the Rust type is?

As mentioned, if you find an alternative that is non-breaking, that is very much welcome.

And then large parts of the PR look like special handling for (). I don't understand why () is allowed as a key type in the > fist place. I mean I can create a Map<(), String> but it does not make any sense. Did we go too far with the complexity here?

Amusingly, maps with () are being used; basically, to serve as a proxy or alternative for an item. This is good for generalisation. Take a look at SnapshotItem implementation.

Anyway, given the state of things, the PR looks like great work.

Indeed. The change may be breaking, but it is minimally so IMO. And, the entire thing with keys and keys deserialisation, including nested ones, is working; which is kind of amazing.