Improve comments

rust/src/database/mod.rs

@@ -1,4 +1,4 @@
-//! DBAPI2-shaped Connection / Cursor types implemented in Rust.
+//! Database access implemented in Rust.
 //!
 //! Currently this provides a single Postgres backend ([`tokio_postgres`]); see
 //! the [`postgres`] submodule.
@@ -17,8 +17,8 @@ pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()>
 
     m.add_submodule(&child)?;
 
-    // Mirror the convention used by other rust submodules so
-    // `from synapse.synapse_rust import database` works.
+    // We need to manually add the module to sys.modules to make `from
+    // synapse.synapse_rust import database` work.
     py.import("sys")?
         .getattr("modules")?
         .set_item("synapse.synapse_rust.database", child)?;

rust/src/database/postgres/helpers.rs

@@ -14,7 +14,7 @@
  */
 
 //! Extension traits for driving [`tokio_postgres`] futures to completion from
-//! the synchronous, GIL-holding Python methods.
+//! synchronous, GIL-holding Python code.
 //!
 //! Both [`BlockingPostgres`] and [`BlockingPostgresStream`] release the GIL
 //! (`py.detach`) while blocking on the shared tokio runtime, so other Python
@@ -23,10 +23,9 @@
 //! the `detach` boundary.
 //!
 //! The stream helper is generic over the underlying stream type rather than
-//! hard-wired to [`tokio_postgres::RowStream`]. In production it is always used
-//! with a `RowStream`, but keeping it generic lets the cursor state machine
-//! (which uses these helpers) be unit-tested against an in-memory fake stream
-//! with no live database — see [`BlockingPostgresStream`]'s tests.
+//! hard-wired to [`tokio_postgres::RowStream`], so code built on it can be
+//! unit-tested against an in-memory stream with no live database — see
+//! [`BlockingPostgresStream`]'s tests.
 
 use std::{future::Future, pin::Pin};
 
@@ -52,8 +51,8 @@ where
     /// (see [`crate::tokio_runtime`]) entered first with
     /// [`Handle::enter`](tokio::runtime::Handle::enter); [`Handle::current`]
     /// panics otherwise. The blocking wait runs on the calling (Python) thread,
-    /// never on a runtime worker — see this module's docs for why that can't
-    /// deadlock.
+    /// never on a runtime worker, so it cannot starve the worker threads that
+    /// complete the future.
     fn block_on(self, py: Python<'_>) -> Self::Output {
         py.detach(|| Handle::current().block_on(self))
     }
@@ -91,16 +90,13 @@ where
 /// the shared runtime only when the next item isn't already buffered.
 ///
 /// Implemented for any pinned, fused stream (`Pin<&mut Fuse<S>>`) whose items
-/// can cross the GIL-release boundary. In production `S` is
-/// [`tokio_postgres::RowStream`]; the generic bound is what lets the cursor
-/// logic be tested against an in-memory fake.
+/// can cross the GIL-release boundary, so it can be tested against an
+/// in-memory stream as well as a [`tokio_postgres::RowStream`].
 ///
-/// The [`Fuse`] is *required* by the impl (the trait is implemented only for
-/// `Pin<&mut Fuse<S>>`), not merely assumed. This matters because
-/// [`Self::get_next_if_ready`] may poll the stream again after it has finished:
-/// a bare `Stream` is free to panic if polled past completion, whereas a fused
-/// stream simply keeps yielding `None`. So repeated `get_next_if_ready` /
-/// `block_on_next` calls after exhaustion are safe by construction.
+/// The [`Fuse`] bound matters because [`Self::get_next_if_ready`] may poll the
+/// stream again after it has finished. A bare `Stream` is allowed to panic if
+/// polled past completion; a fused stream keeps yielding `None`, so calls
+/// after exhaustion are safe.
 pub trait BlockingPostgresStream
 where
     Self: futures::Stream + Sized + Send + Ungil + Unpin,
@@ -134,9 +130,8 @@ where
     }
 }
 
-// Blanket impl over any pinned, fused stream. Requiring the helper bounds here
-// (rather than only for `RowStream`) is what makes the cursor logic testable
-// with a fake stream.
+// Blanket impl over any pinned, fused stream, not just `RowStream`, so the
+// tests can use an in-memory stream.
 impl<S> BlockingPostgresStream for Pin<&mut Fuse<S>>
 where
     Self: futures::Stream + Send + Ungil + Unpin,
@@ -156,11 +151,9 @@ mod tests {
 
     use super::*;
 
-    /// A throwaway runtime standing in for the shared one. Production code
-    /// enters `crate::tokio_runtime`'s runtime on each thread; the helpers only
-    /// need *some* runtime entered on the current thread (so [`Handle::current`]
-    /// resolves), and (as in production) the blocking wait runs on this test
-    /// thread rather than on a worker. Each test calls `rt.enter()` and holds
+    /// A throwaway runtime standing in for the shared one. The helpers only
+    /// need *some* runtime entered on the current thread, so that
+    /// [`Handle::current`] resolves. Each test calls `rt.enter()` and holds
     /// the guard for the duration.
     fn test_runtime() -> Runtime {
         tokio::runtime::Builder::new_multi_thread()
@@ -189,8 +182,8 @@ mod tests {
             let ok = async { Ok::<i32, tokio_postgres::Error>(5) };
             assert_eq!(ok.block_on_result(py).unwrap(), 5);
             // The error path (mapping a `tokio_postgres::Error` to a `PyErr`)
-            // can't be unit-tested here, as that error type can't be
-            // constructed by hand; it's exercised by the integration tests.
+            // isn't covered here, because that error type can't be constructed
+            // by hand. Exercising it needs a live server.
         });
     }
 
@@ -246,9 +239,8 @@ mod tests {
             assert_eq!(stream.as_mut().block_on_next(py), Some(Ok(7)));
             assert_eq!(stream.as_mut().block_on_next(py), None);
 
-            // And, on a fresh stream, `block_on_next` handles the pending first
-            // poll entirely on its own (no preceding `get_next_if_ready`),
-            // proving it doesn't rely on being "primed" by an earlier call.
+            // And, on a fresh stream, `block_on_next` handles a pending first
+            // poll on its own, with no preceding `get_next_if_ready`.
             let stream = stream::once(async {
                 tokio::task::yield_now().await;
                 Ok::<i32, ()>(8)

rust/src/database/postgres/mod.rs

@@ -1,29 +1,21 @@
 //! [`tokio_postgres`]-backed Postgres backend for the Rust `database` module.
 //!
-//! This module will grow the Python-facing `Connection` / `Cursor` classes and
-//! the `connect` factory; for now it hosts the value-mapping layer ([`value`])
-//! that converts between Python objects and Postgres' binary wire format.
-//!
-//! The driver itself is async; the eventual `Connection` / `Cursor` types will
-//! drive it from sync Python methods via a shared multi-thread tokio runtime.
+//! The driver is async. [`helpers`] drives its futures to completion from
+//! synchronous Python code on the shared tokio runtime, and [`value`] converts
+//! between Python objects and Postgres' binary wire format.
 
 use pyo3::exceptions::PyRuntimeError;
 use pyo3::prelude::*;
 use pyo3::types::PyModule;
 
-// `pub` (rather than private) so the not-yet-consumed public items in these
-// submodules are reachable from the crate root as public API. This is what
-// stops clippy's `dead_code` lint from firing on them before the
-// cursor/connection code (added in later changes) wires them up; the visibility
-// is tightened back to private once that happens.
+// `pub` so the items in these submodules count as public API even though
+// nothing consumes them yet, which keeps clippy's `dead_code` lint quiet.
+// Tighten to private once the connection/cursor code uses them.
 pub mod helpers;
 pub mod value;
 
-/// Register the `postgres` submodule under the parent `database` module.
-///
-/// The `Connection` / `Cursor` classes and the `connect` factory are added in
-/// later changes; for now this just creates the (otherwise empty) submodule so
-/// the module tree — and the `value` mapping layer hanging off it — exists.
+/// Register the (currently empty) `postgres` submodule under the parent
+/// `database` module.
 pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
     let child = PyModule::new(py, "postgres")?;
 

rust/src/database/postgres/value.rs

@@ -1,12 +1,9 @@
 //! Conversions between Python values and the Postgres SQL value
 //! representations.
 //!
-//! Kept in its own module so the cursor code stays focused on the DBAPI shape
-//! rather than the type-mapping table.
-//!
-//! First cut: int / float / bool / str / bytes / None. Lists (for
-//! `ANY($1)`-style queries) and richer types — json, decimal, timestamps —
-//! are deferred to a follow-up.
+//! Supports int / float / bool / str / bytes / None. Lists (for
+//! `ANY($1)`-style queries) and richer types such as json, decimal and
+//! timestamps are not yet supported.
 //!
 //! The mapping is column-type-driven on the way *out* (a single Python `int`
 //! becomes `INT2`/`INT4`/`INT8` depending on the column it is bound to) and
@@ -52,14 +49,12 @@ impl PgValue {
     /// Classify a Python object into a [`PgValue`], or error if its type isn't
     /// one we know how to send to Postgres.
     ///
-    /// Two subtleties worth calling out:
     ///   * `bool` is classified as [`PgValue::Bool`], never [`PgValue::Int`],
     ///     even though Python's `bool` is a subclass of `int`.
     ///   * `int` must fit in an `i64`; a larger Python integer raises an
     ///     `OverflowError` here, since Postgres has no wider integer type in
     ///     this mapping.
-    ///
-    /// A type we don't recognise raises `TypeError`.
+    ///   * A type we don't recognise raises `TypeError`.
     pub fn from_py(obj: &Bound<PyAny>) -> PyResult<Self> {
         if obj.is_none() {
             return Ok(PgValue::Null);
@@ -88,8 +83,8 @@ impl PgValue {
     }
 }
 
-// Lets PyO3 extract a `PgValue` directly from a Python argument, e.g. when a
-// cursor method takes `Option<Vec<PgValue>>` for its parameters.
+// Lets PyO3 extract a `PgValue` directly from a Python argument, e.g. from a
+// method that takes `Option<Vec<PgValue>>` as its parameter list.
 impl<'a, 'py> FromPyObject<'a, 'py> for PgValue {
     type Error = PyErr;
 
@@ -135,13 +130,10 @@ impl ToSql for PgValue {
                 Ok(IsNull::No)
             }
             (&PgValue::Float(v), &Type::FLOAT4) => {
-                // The `as` cast here generates the closest f32 to the f64,
-                // with loss of precision. Since Python floats are variable
-                // precision anyway, this is the best we can do.
-                //
-                // (Crucially, there is no way of doing a "fallible" cast
-                // here, since unlike integers there is no notion of "out of
-                // range" for floats, just varying precision.)
+                // The `as` cast narrows to the nearest f32, losing precision.
+                // Unlike the integer arms there is no fallible conversion to
+                // use, since floats have no notion of "out of range", just
+                // varying precision.
                 float4_to_sql(v as f32, buf);
                 Ok(IsNull::No)
             }
@@ -359,8 +351,8 @@ mod tests {
 
     #[test]
     fn from_py_extracts_via_frompyobject() {
-        // The cursor binds parameters by extracting `PgValue` straight off the
-        // Python argument; check that `FromPyObject` path forwards to `from_py`.
+        // Check that extracting a `PgValue` from a Python object (the
+        // `FromPyObject` path) forwards to `from_py`.
         Python::initialize();
         Python::attach(|py| {
             let obj = 7i64.into_pyobject(py).unwrap().into_any();
@@ -376,7 +368,7 @@ mod tests {
             let list = pyo3::types::PyList::new(py, [1, 2, 3]).unwrap();
             let err = PgValue::from_py(&list.into_any()).unwrap_err();
             assert!(err.is_instance_of::<PyTypeError>(py));
-            // The message names the offending type, which is the useful part.
+            // The message should name the offending type.
             assert!(err.to_string().contains("list"), "got: {err}");
         });
     }
@@ -420,7 +412,7 @@ mod tests {
             encode(&PgValue::Float(value), &Type::FLOAT4).0,
             (value as f32).to_be_bytes()
         );
-        // And the result is provably narrower than the FLOAT8 encoding.
+        // And the bytes are not just a truncation of the f64 encoding.
         assert_ne!(
             encode(&PgValue::Float(value), &Type::FLOAT4).0,
             value.to_be_bytes()[..4].to_vec()

rust/src/tokio_runtime.rs

@@ -51,10 +51,9 @@ impl PyTokioRuntime {
 impl PyTokioRuntime {
     /// Build the runtime if it hasn't been built yet.
     ///
-    /// Idempotent, so it is safe to call both from the reactor's
-    /// `callWhenRunning(start)` hook and from a caller that needs the runtime
-    /// before the reactor has run that hook (see [`runtime_handle`]): whichever
-    /// runs first builds it, the other is a no-op.
+    /// Both the reactor's `callWhenRunning(start)` hook and [`runtime_handle`]
+    /// call this, in either order. Whichever runs first builds the runtime;
+    /// for the other it is a no-op.
     fn ensure_started(&mut self) -> PyResult<()> {
         if self.runtime.is_some() {
             return Ok(());
@@ -92,11 +91,10 @@ pub fn runtime<'a>(reactor: &Bound<'a, PyAny>) -> PyResult<PyRef<'a, PyTokioRunt
 /// Get a clonable handle to the shared runtime, starting it on demand.
 ///
 /// Unlike [`runtime`], this does not require the reactor to have already run
-/// its `callWhenRunning(start)` hook: it starts the runtime if necessary. That
-/// lets callers that need the runtime before the reactor is up — the database
-/// backend's schema setup, `synapse_port_db`, and trial tests — still get a
-/// working handle. Once the reactor does run, its `start` hook finds the
-/// runtime already built and is a no-op, so there is still only one runtime.
+/// its `callWhenRunning(start)` hook; the runtime is started here if needed,
+/// so a caller that needs it before the reactor is up still gets a working
+/// handle. Once the reactor does run, its `start` hook finds the runtime
+/// already built and is a no-op, so there is still only one runtime.
 pub fn runtime_handle(reactor: &Bound<'_, PyAny>) -> PyResult<Handle> {
     let runtime = get_or_install(reactor)?;
     let mut runtime = runtime.borrow_mut();