Fix build warnings and a few assorted tweaks #13
Conversation
src/tests/net.rs
Outdated
| }; | ||
|
|
||
| #[alkahest(Formula)] | ||
| pub enum GameMessageFormula { | ||
| Client(Ref<ClientMessageFormula>), |
There was a problem hiding this comment.
The Ref looks unnecessary here
There was a problem hiding this comment.
Ah yeah, I was just experimenting with it -- this is actually the reason I initially wanted to separate the formula from the serialize and deserialize types, to insert a wrapping handler in the mix. Ultimately I'm trying to play with making a Ref-like wrapper that would help to do schema evolution (forward/backward compatibility). But it is of course unnecessary in this case.
There was a problem hiding this comment.
What exactly are properties of Ref-like wrapper you have in mind?
Current Ref IMHO is enough to allow separate evolution of field's Formula.
Currently user may evolve schema and manually ensure desired compatibility.
I have the following plan to better support them:
- Add validation to check compatibility between two
Formulas. - Add attributes to mark new fields when deriving
Formula. Then compatibility will be checked between versions. All in proc-macro execution time.
There was a problem hiding this comment.
Hm, not sure if I quite follow. If you serialize some data with Ref<SomeTV1>, add a field, then try to deserialize the old data with Ref<SomeTV2>, that will not work, right?
It would be nice to have the ability to provide default values for fields added and then based on size of serialized data detect which fields are missing and fill them in.
You could also put another wrapper that is like Lazy<Ref<T>> where instead of filling in with default values, you could "attempt to get" the type at different versions, i.e. with different numbers of fields. Similar to "probe" types I made in protoss experiment for rkyv: https://github.com/fu5ha/protoss/blob/master/protoss_test/src/derive.rs#L169-L173
There was a problem hiding this comment.
Yes, you would need to have defaults for new fields for backward compatibility.
For forward compatibility it would be enough to mark formula as non-exhaustive, so it would just skip fields added at the end.
How about Lazy<AnyRef> that can be viewed as Lazy<AnyRef<T>>.
Where the only difference between Ref<T> and AnyRef<T> is that binary format of AnyRef doesn't depend on T
There was a problem hiding this comment.
Maybe Lazy layer is not needed.
struct AnyRef {...}
impl AnyRef {
fn decode<F: Formula, T: Deserialize<F>>() -> Option<T> { ... }
}This requires more work 🤣
There was a problem hiding this comment.
Yeah exactly, I was thinking of making a single thing similar to what you propose that's like a hybrid.
Ideally it would still be typed based on the main type it represents "some version" of. Then decode would only let you ask for one of the known versions of that type
There was a problem hiding this comment.
struct AnyVersionRef<T: Versioned> {...}
impl<T: Versioned> AnyVersionRef<T> {
fn decode_version<V: VersionOf<T>>(&self) -> Option<V> { ... }
fn latest_with_defaults(self) -> T { ... }
}Needs more thinking about how exactly to integrate the deserialize and formula types/bounds but thats the basic vision I guess.
|
|
||
| for message in packet.game_messages.iter::<GameMessageRead>() { | ||
| match message.unwrap() { | ||
| GameMessageRead::Client(ClientMessageRead::ClientData { nickname, clan }) => { | ||
| drop(nickname); | ||
| drop(clan); | ||
| let _ = nickname; |
There was a problem hiding this comment.
Any reason for this besides style?
There was a problem hiding this comment.
Clippy doesn't like dropping references and says to ignore it via _ instead. But yeah it's ultimately just style.
There was a problem hiding this comment.
If default-configured clippy says so I have no objections.
| pub enum GameMessageRead<'de> { | ||
| Client(ClientMessageRead<'de>), | ||
| Server(ServerMessageRead<'de>), | ||
| } | ||
|
|
||
| #[alkahest(Formula)] | ||
| pub enum ClientMessageFormula { | ||
| ClientData { nickname: Ref<str>, clan: Ref<str> }, |
There was a problem hiding this comment.
Ref<str> and String are distinct but actually equivalent formulas.
There was a problem hiding this comment.
Right :D Which is cool
There was a problem hiding this comment.
So why making separate type? ClientMessage as Formula would be the same.
There was a problem hiding this comment.
It was not obvious to me from documentation what a formula actually is -- that it is a rust type that mirrors the type you want that contains types which have a direct exact representation. Making this split it becomes more obvious what's going on -- in my eyes at least. And it shows how you can create multiple formulas which the same type can serialize as / deserialize from, or customize the exact representation with wrappers like Ref. The point of all this change was to try what it would look like to 1. separate all the formulas 2. make tweaks like adding Ref to build my intuition on the versioned wrapper we talked about in the other thread.
Whether the proper place for that is in a test like here: perhaps not x) but I was just playing around with it to get a better idea of how things work.
Probably it should go in a separate test/example to show it better. I would be fine with reverting the formula changes in this file for sure.
|
I will merge this and add my own changes on top. |
This makes the repo workspace build under stable by putting the criterion
real_blackboxfeature undernightlyfeature in the benchmark crate. More convenient to develop with stable rust-analyzer that way. Also, adds a separate "deserialize" category to the benchmark which deserializes to owned types rather than partially zero-copy reader structs.It also fixes some various warnings that existed on the code and reworks a bit of the
netexample to show off creating separate formula types from the actual serialize/deserialize structs.