Open dtolnay opened 1 month ago
@dtolnay Thanks for the PR! I'll try to have comments this week
Thank you @dtolnay for coming up with a fix so quickly!
This PR is very interesting to me, I think that it may also move us closer to advanced functionality like being able to exhaustively trace enums which are nested as discussed in #21? I'd be interested in any thoughts on that angle. After a cursory read through it seems like this approach could maybe be extended to enable a branch to be repeatedly explored until its children are completed instead of the current behavior of panicking with incomplete enums:
pub(crate) enum EnumProgress {
/// There are children which are incomplete
ChildTypesRemaining(VariantId),
/// There are variant names that have not yet been traced.
NamedVariantsRemaining,
/// There are variant numbers that have not yet been traced.
IndexedVariantsRemaining,
}
@jerel, yes your intuition is correct. As I was working on this PR I was surprised to find out the underpowered approach to trace_type that this crate was using (although I did not come across #21). I had heard about the existence of this crate a while ago, and what you're saying matches how I had always assumed this crate already works.
@jerel @dtolnay I don't quite see how this PR changes anything regarding #21. The most desirable approach (processing all enum variants right away the first time we visit an enum type) is still prevented by the definition of the Deserializer
trait, and notably the fact that the given visitor
s are not clonable (for good reasons).
This is what I responded on #21:
Consider nested enums: enum A { ... SomethingUsingB(.. B ..) } and enum B { .. many variants .. }. To efficiently trace B when A is the top user type, one would have to repeatedly force the same path from A to B until all the variants of B are covered. This path from A to B could be quite complex and would need to be recorded in a Rust datatype so that it can be replayed. (Also, the first path found from A to B might not be the shortest.)
Feel free to continue the discussion on #21 if you disagree.
@dtolnay Not sure if you remember but indeed we discussed the design of this crate at its very beginning where we were both Facebook employees. You suggested the idea of trace_type
, so thank you for that! This crate has been used by
at least three new blockchain projects since then.
Summary
Closes https://github.com/zefchain/serde-reflection/issues/52. Previously, in the case of an enum with aliases:
serde-reflection would look at the list of expected variant names ("A", "B", "B1") and decide that it should expect to deserialize them using the same number of variant indices 0, 1, 2. This isn't correct because "B" and "B1" actually share the same index 1, and there does not exist any variant with index 2. Thus
tracer.trace_type::<Enum>
would fail with an error like Failed to deserialize value: "invalid value: integer '2', expected variant index 0 ≤ i < 2".This PR improves
trace_type
's deserializer to first try deserializing a variant using each of the supplied names ("A", "B", "B1"), record the enum discriminant of each resulting enum value, then deserializing again using sequential indices (0, 1, …) until it has found a result with the same discriminant as each of the named variants. From this it can see that both "A" and 0 deserialize to the same variant Enum::A, while all of "B" and "B1" and 1 deserialize to Enum::B.Test Plan
cd serde-reflection; cargo test