Tidy up Rust RoomVersion structs#19766
Conversation
48b0729 to
f1705f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors Synapse’s Rust-side room version definitions (RoomVersion) to make them easier to maintain and more convenient to consume from Rust/PyO3, ahead of increased Rust usage of room versions.
Changes:
- Reworks room version definitions so each version is expressed as a delta from the previous version via associated
RoomVersion::{V1..}constants. - Promotes various version constants to
puband updates mapping accessors to useCloneinstead ofCopy. - Adds
DisplayforRoomVersionand aFromStrparser returning references to known room versions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/src/room_versions.rs | Refactors RoomVersion constants into associated consts, adds parsing/formatting traits, and updates mapping access patterns. |
| changelog.d/19766.misc | Adds a changelog entry for the Rust room version refactor. |
Comments suppressed due to low confidence (2)
rust/src/room_versions.rs:448
items()usesself.versions.read().unwrap(), which can panic on a poisoned lock and bring down the Python interpreter. Please handle the poisoning case and return aPyRuntimeError, consistent withadd_room_version/__len__.
let versions = self.versions.read().unwrap();
Ok(versions.iter().map(|v| (v.identifier, v.clone())).collect())
}
rust/src/room_versions.rs:466
get()usesself.versions.read().unwrap(), which can panic if the lock is poisoned (crashing the process). Consider usingread().map_err(|_| PyRuntimeError::new_err(...))?here as well for consistency and safety.
let versions = self.versions.read().unwrap();
if let Some(version) = versions.iter().find(|v| v.identifier == key) {
return Ok(Some(version.clone().into_bound_py_any(py)?));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RoomVersion::MSC3757V10, | ||
| RoomVersion::MSC3757V11, | ||
| RoomVersion::HYDRA_V11, | ||
| ]; |
There was a problem hiding this comment.
MSC4242V12 supposed to be in this list? 🤷 I guess it wasn't there before
Overall, I haven't gone through everything with a fine tooth comb to spot this kind of thing if it happened elsewhere but this stood out to me.
There was a problem hiding this comment.
Yeah, we only add it in to that mapping if the experimental config is set:
synapse/synapse/config/experimental.py
Lines 479 to 483 in ff0420a
This is in prep for using the room versions more from Rust.
Main changes:
RoomVersionconstants, likeRoomVersion::V1, for convenience.