From 2bb64c534f3016c9f516f779843a461e71529de6 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:30:59 +0000 Subject: [PATCH 01/11] docs(lint): ENG-4684 library lint posture + harden lint_state Replace src/lib.rs's lone `#![deny(unsafe_code)]` with the ticket's full posture: a six-lint deny group (unsafe_code, missing_docs, missing_debug_implementations, rust_2018_idioms, rust_2024_compatibility, rustdoc::broken_intra_doc_links), `#![warn(clippy::pedantic, clippy::nursery)]`, and a three-entry allow list where every entry carries its justification on the lines directly above it. The ticket's suggested `missing_panics_doc` comment ("no public panics") is factually wrong -- `Graph::archived` (src/loader.rs:290) and `Graph::route` (:495) both `.expect(...)`. Replaced with an accurate justification; both sites get a real `# Panics` section in a later commit. Also add a package-wide `[lints.clippy] pedantic = { level = "warn", priority = -1 }` table to Cargo.toml. This is a deliberate addition beyond the ticket: `#![...]` in src/lib.rs reaches only the library crate, so without it the pedantic findings in build.rs, tests/*, benches/ and src/bin/ are gated solely by one line of CI YAML and stay invisible to .pre-commit-config.yaml's `cargo clippy --no-deps -- -D warnings` hook. `priority = -1` keeps per-file allows winning over the group. tests/lint_state.rs grows from one assertion to five, all additive: the existing deny/forbid check gains whitespace normalisation so the multi-line attribute layout still matches, and new tests lock the full deny group, both clippy groups, the absence of any blanket allow, and the Cargo.toml table. The `forbid(unsafe_code)` rejection is unchanged. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- Cargo.toml | 12 +++ src/lib.rs | 28 ++++++- tests/lint_state.rs | 177 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 211 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bfd779e..07e138c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,3 +80,15 @@ codspeed-criterion-compat = { version = "2", features = ["html_reports"] } [[bench]] name = "route" harness = false + +# ENG-4684: package-wide pedantic baseline. `#![warn(...)]` in src/lib.rs +# reaches ONLY the library crate, so without this table the ~120 pedantic +# findings in build.rs, tests/*, benches/ and src/bin/ are gated solely by +# the `-W clippy::pedantic` flag on `.github/workflows/ci.yaml:47` — invisible +# to `.pre-commit-config.yaml`'s `cargo clippy --no-deps -- -D warnings` hook +# and to every contributor's editor. `priority = -1` makes the group rank +# below individual lint settings, so the per-file `#![allow(...)]` blocks and +# site allows still win. Nursery deliberately stays library-scoped (src/lib.rs) +# — the CI flag is pedantic-only and this table mirrors it. +[lints.clippy] +pedantic = { level = "warn", priority = -1 } diff --git a/src/lib.rs b/src/lib.rs index e92e355..9ecd633 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,33 @@ //! and further algorithms follow in later tickets. See `README.md` and //! `NOTICE` for project status and upstream attribution. -#![deny(unsafe_code)] +#![deny( + unsafe_code, + missing_docs, + missing_debug_implementations, + rust_2018_idioms, + rust_2024_compatibility, + rustdoc::broken_intra_doc_links +)] +#![warn(clippy::pedantic, clippy::nursery)] +#![allow( + // Every module here is named for the concept it owns (`data`, + // `graph`, `loader`), so `graph::GraphData` and `data::DATA_LEN_*KM` + // repeat the module name by design — renaming them to satisfy the + // lint would make the public API read worse, not better. + clippy::module_name_repetitions, + // `RouteError` / `LoadError` are `thiserror` enums whose `#[error]` + // messages document each failure mode at the variant, and the + // fallible public methods (`from_bytes`, `load`, `route`, + // `edges_for_groups`) each carry a hand-written `# Errors` section. + clippy::missing_errors_doc, + // NOT "no public panics" — `Graph::archived` and `Graph::route` both + // contain a deliberate `.expect(...)` on an invariant established at + // construction. Both document it in a `# Panics` section; this allow + // exists so the lint does not also demand one on the infallible + // getters that merely call `archived()` internally. + clippy::missing_panics_doc +)] pub mod data; pub mod graph; diff --git a/tests/lint_state.rs b/tests/lint_state.rs index f15a4b2..934ca5f 100644 --- a/tests/lint_state.rs +++ b/tests/lint_state.rs @@ -2,18 +2,71 @@ //! `forbid`. The mmap-based `Graph::load` needs a single targeted //! `#[allow(unsafe_code)]` on the `memmap2::Mmap::map(&file)` call, //! which `forbid` would reject. +//! +//! ENG-4684 extends this file to lock the *whole* lint posture, not just +//! the unsafe-code half: +//! +//! - the `#![deny(...)]` group in `src/lib.rs` names `missing_docs`, +//! `missing_debug_implementations` and +//! `rustdoc::broken_intra_doc_links` alongside `unsafe_code`; +//! - `src/lib.rs` warns both `clippy::pedantic` and `clippy::nursery`; +//! - `Cargo.toml` carries a `[lints.clippy]` table warning `pedantic` +//! package-wide, because `#![...]` in `src/lib.rs` reaches only the +//! library crate and would leave `build.rs`, `tests/*`, `benches/` +//! and `src/bin/` gated by a single line of CI YAML. +//! +//! Everything here is a source-text assertion, deliberately: the lints +//! are *configuration*, so a compile-time check cannot tell "the gate is +//! on" from "the gate is off and nothing happens to violate it". If one +//! of these declarations is deleted, CI stays green and the regression is +//! silent — which is exactly the drift this file exists to catch. This +//! file may only ever be changed in the direction of asserting MORE. + +/// Collapse every run of ASCII whitespace to a single space so the +/// assertions below are insensitive to how `rustfmt` (or a human) chooses +/// to break a multi-line inner attribute. `#![deny(\n unsafe_code,` +/// and `#![deny(unsafe_code,` must satisfy the same check — the intent is +/// "the deny group names this lint", not "the file contains this exact +/// byte sequence". +fn squeeze_whitespace(src: &str) -> String { + let mut out = String::with_capacity(src.len()); + let mut in_ws = false; + for c in src.chars() { + if c.is_whitespace() { + if !in_ws { + out.push(' '); + } + in_ws = true; + } else { + out.push(c); + in_ws = false; + } + } + out +} + +fn lib_rs() -> String { + let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/lib.rs"); + std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("read {}: {e}", path.display())) +} + +fn cargo_toml() -> String { + let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml"); + std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("read {}: {e}", path.display())) +} #[test] fn lib_rs_uses_deny_not_forbid_unsafe_code() { - let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/lib.rs"); - let src = - std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("read {}: {e}", path.display())); + let src = lib_rs(); // Match the inner-attribute prefix and leave the closing paren // open so additional lints in the same attribute group don't // break the check — e.g. `#![deny(unsafe_code, warnings)]` or - // reformatted variants still satisfy the intent. + // reformatted variants still satisfy the intent. Whitespace is + // squeezed first so the multi-line ENG-4684 layout + // (`#![deny(\n unsafe_code,`) also matches. + let squeezed = squeeze_whitespace(&src); assert!( - src.contains("#![deny(unsafe_code"), + squeezed.contains("#![deny( unsafe_code") || squeezed.contains("#![deny(unsafe_code"), "src/lib.rs must declare a deny(unsafe_code) inner attribute (got: {})", src.lines().take(15).collect::>().join("\\n") ); @@ -24,3 +77,117 @@ fn lib_rs_uses_deny_not_forbid_unsafe_code() { "src/lib.rs must NOT declare forbid(unsafe_code) (would block targeted allow on mmap)" ); } + +/// ENG-4684 AC6: the deny group must name every lint the ticket +/// specifies, not just `unsafe_code`. Asserted on the whitespace-squeezed +/// text of the single `#![deny(...)]` attribute so a reflow cannot break +/// it, and so a lint moved out of `deny` into `warn` (or dropped +/// entirely) fails loudly. +#[test] +fn lib_rs_deny_group_names_the_full_lint_set() { + let src = lib_rs(); + let squeezed = squeeze_whitespace(&src); + let start = squeezed + .find("#![deny(") + .expect("src/lib.rs must declare a #![deny(...)] inner attribute"); + let end = squeezed[start..] + .find(')') + .map(|i| start + i) + .expect("the #![deny(...)] attribute must be closed"); + let deny_group = &squeezed[start..end]; + for lint in [ + "unsafe_code", + "missing_docs", + "missing_debug_implementations", + "rust_2018_idioms", + "rust_2024_compatibility", + "rustdoc::broken_intra_doc_links", + ] { + assert!( + deny_group.contains(lint), + "src/lib.rs's #![deny(...)] group must name `{lint}` — without it the \ + corresponding gate is off and nothing goes red when it is violated. \ + Group as read: {deny_group:?}" + ); + } +} + +/// ENG-4684 AC5: the library opts into both clippy groups. `pedantic` +/// is *also* set package-wide in `Cargo.toml` (see +/// `cargo_toml_warns_pedantic_package_wide`), but `nursery` is +/// deliberately library-only — the AC's clippy command carries +/// `-W clippy::pedantic` and no nursery flag, so `src/lib.rs` is the only +/// thing that makes `cargo clippy --lib -- -D warnings` a nursery gate. +#[test] +fn lib_rs_warns_pedantic_and_nursery() { + let squeezed = squeeze_whitespace(&lib_rs()); + assert!( + squeezed.contains("#![warn(clippy::pedantic"), + "src/lib.rs must declare `#![warn(clippy::pedantic, ...)]` — it is what makes \ + `cargo clippy --lib --all-features -- -D warnings` a pedantic gate (AC5)." + ); + assert!( + squeezed.contains("clippy::nursery"), + "src/lib.rs must warn `clippy::nursery` — nursery is deliberately NOT extended \ + to the other targets, so this attribute is the only place it is enabled." + ); +} + +/// ENG-4684: no blanket escape hatch. A `#![allow(clippy::pedantic)]`, +/// `#![allow(clippy::nursery)]` or `#![allow(warnings)]` anywhere in +/// `src/lib.rs` would silently undo the two attributes above while +/// leaving them visible in the file. +#[test] +fn lib_rs_has_no_blanket_allow() { + let squeezed = squeeze_whitespace(&lib_rs()); + for banned in [ + "allow(clippy::pedantic", + "allow(clippy::nursery", + "allow(warnings", + ] { + assert!( + !squeezed.contains(banned), + "src/lib.rs must not contain `{banned})` — a blanket allow re-disables the \ + whole group the ticket just enabled. Allows must name individual lints." + ); + } +} + +/// ENG-4684: `#![...]` attributes in `src/lib.rs` apply to the library +/// crate ONLY. `build.rs`, every `tests/*.rs`, `benches/route.rs` and +/// `src/bin/rustyroute.rs` are separate crates, and the ~120 pedantic +/// findings they carry were fixed or justified-allowed under this ticket. +/// Without the `Cargo.toml [lints.clippy]` table the only thing keeping +/// them clean is the `-W clippy::pedantic` flag on `ci.yaml:47`, which +/// `.pre-commit-config.yaml`'s clippy hook does not pass — so a +/// contributor's local run would not see a regression. +#[test] +fn cargo_toml_warns_pedantic_package_wide() { + let toml = cargo_toml(); + let squeezed = squeeze_whitespace(&toml); + assert!( + squeezed.contains("[lints.clippy]"), + "Cargo.toml must declare a `[lints.clippy]` table so the pedantic baseline \ + applies to every target in the package, not just the library." + ); + let start = squeezed + .find("[lints.clippy]") + .expect("checked immediately above"); + let table = &squeezed[start..]; + assert!( + table.contains("pedantic"), + "Cargo.toml's `[lints.clippy]` table must set `pedantic` — that is the whole \ + reason the table exists (ENG-4684). Table as read: {table:?}" + ); + // `= "allow"` in either the table or the lib attributes would be a + // silent un-gating; only `warn` or `deny` are acceptable levels here. + let pedantic_line = toml + .lines() + .find(|l| l.trim_start().starts_with("pedantic")) + .expect("Cargo.toml must carry a `pedantic = ...` entry under [lints.clippy]"); + assert!( + pedantic_line.contains("\"warn\"") || pedantic_line.contains("\"deny\""), + "Cargo.toml's `pedantic` lint level must be `warn` or `deny`, not allow/forbid — \ + CI escalates warnings with `-D warnings`. Offending line: {pedantic_line:?}" + ); +} From e82a8c97fa24ba51cf0e63a28af3fc31f71bc3d1 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:33:22 +0000 Subject: [PATCH 02/11] docs(api): ENG-4684 document all public items `#![deny(missing_docs)]` (previous commit) surfaced nine undocumented public items. All are now documented to the standard the ticket asks for -- units, ranges and index semantics, never a restatement of the identifier: * `NodeCoord::lng` / `::lat` -- decimal degrees, WGS84 (EPSG:4326), sign convention and valid range. The struct doc now also warns that its field order is `(lng, lat)` while the public routing API uses `(lat, lng)`. * `GroupEntry::name` -- the lowerCamelCase identifier `Graph::edges_for_groups` matches on, and therefore part of the on-disk contract. * `GroupEntry::edge_ids` -- indices into `edge_endpoints` / `undirected_weights`, explicitly *not* into `edges`; sorted and duplicate-free. * `data::BYTES_{5,10,20,50,100}KM` x5 -- documented once, on the `define_bytes!` generator via `#[doc = concat!(...)]`, so all five rendered consts carry the resolution, the required feature, the archive layout, and the alignment caveat. Same tactic as `build/registry.rs`'s doc on the generated `EDGE_GROUPS`. Also splits the over-long first doc paragraphs that `clippy::too_long_first_doc_paragraph` flags on `src/data.rs`, `NodeCoord`, `DirectedEdge`, `GroupEntry` and `GraphData`, and backticks `edge_id` in `GraphData::undirected_weights`. `src/graph.rs` exposes no public *functions* (only `pub const MAGIC` and `pub const SCHEMA_VERSION`, both already documented), so the ticket's "# Examples per public function" clause has no targets in this file -- the nine public methods all live in `src/loader.rs` and are handled in the next commit. Verified: `cargo check --all-features` clean; `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --all-features` clean (AC1); `RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc --no-deps --all-features` clean (AC2); `cargo test --test lint_state` 5 passed (AC6); `cargo fmt --all --check` clean. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- src/data.rs | 40 ++++++++++++++++++++++++++++++++---- src/graph.rs | 57 ++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 78 insertions(+), 19 deletions(-) diff --git a/src/data.rs b/src/data.rs index 40a780e..b954d3c 100644 --- a/src/data.rs +++ b/src/data.rs @@ -1,7 +1,8 @@ //! Feature-gated static byte slices for each pre-baked graph -//! resolution. Each `BYTES_{N}KM` const is `include_bytes!`-baked at -//! rustyroute's compile time from `$OUT_DIR/data/{N}km.rkyv` produced -//! by `build.rs`. +//! resolution. +//! +//! Each `BYTES_{N}KM` const is `include_bytes!`-baked at rustyroute's +//! compile time from `$OUT_DIR/data/{N}km.rkyv` produced by `build.rs`. //! //! These slices are the primary distribution mechanism for downstream //! consumers: with the default `data-50km` feature, downstream code @@ -79,14 +80,40 @@ include!(concat!(env!("OUT_DIR"), "/data_lens.rs")); // `&'static [u8]` slice borrowed from its `data` field. Keeping both // behind one macro avoids five copies of the same `include_bytes!` // boilerplate drifting out of sync. +// The `#[doc = concat!(...)]` on the public slice is what satisfies the +// crate's `#![deny(missing_docs)]` for all five resolutions in one edit +// (ENG-4684). Documenting the generator rather than five generated sites +// follows the same reasoning as `build/registry.rs`'s doc comment on the +// generated `EDGE_GROUPS` const. `$res` exists purely so the rendered +// prose can name the resolution in human form. macro_rules! define_bytes { - ($feature:literal, $raw:ident, $public:ident, $len:ident, $path:literal) => { + ($feature:literal, $raw:ident, $public:ident, $len:ident, $res:literal, $path:literal) => { #[cfg(feature = $feature)] const $raw: Aligned4<{ $len }> = Aligned4 { _align: [], data: *include_bytes!(concat!(env!("OUT_DIR"), $path)), }; #[cfg(feature = $feature)] + #[doc = concat!( + "The complete ", $res, " km graph archive, as a 4-byte-aligned ", + "`&'static [u8]`.\n\n", + "`include_bytes!`-baked at rustyroute's own compile time from ", + "`$OUT_DIR", $path, "`, and therefore present in the library binary ", + "whether or not the consumer has a data directory. Requires the ", + "`", $feature, "` Cargo feature (`data-50km` is the only one enabled ", + "by `default`).\n\n", + "The bytes are the on-disk archive format described in ", + "[`crate::graph`]: a 4-byte `b\"RRG1\"` magic, a little-endian `u32` ", + "[`SCHEMA_VERSION`](crate::graph::SCHEMA_VERSION), then the rkyv ", + "payload. Pass the whole slice to ", + "[`Graph::from_bytes`](crate::Graph::from_bytes) — which validates the ", + "prefix and the payload — rather than slicing it by hand.\n\n", + "Alignment matters: rkyv's relative pointers need a 4-byte-aligned ", + "payload, so the underlying static is wrapped in `Aligned4` (see the ", + "module docs). Copying these bytes into a `Vec` may lose that ", + "alignment and make `from_bytes` fail with ", + "[`LoadError::InvalidArchive`](crate::LoadError::InvalidArchive).", + )] pub const $public: &[u8] = &$raw.data; }; } @@ -96,6 +123,7 @@ define_bytes!( RAW_5KM, BYTES_5KM, DATA_LEN_5KM, + "5", "/data/5km.rkyv" ); define_bytes!( @@ -103,6 +131,7 @@ define_bytes!( RAW_10KM, BYTES_10KM, DATA_LEN_10KM, + "10", "/data/10km.rkyv" ); define_bytes!( @@ -110,6 +139,7 @@ define_bytes!( RAW_20KM, BYTES_20KM, DATA_LEN_20KM, + "20", "/data/20km.rkyv" ); define_bytes!( @@ -117,6 +147,7 @@ define_bytes!( RAW_50KM, BYTES_50KM, DATA_LEN_50KM, + "50", "/data/50km.rkyv" ); define_bytes!( @@ -124,6 +155,7 @@ define_bytes!( RAW_100KM, BYTES_100KM, DATA_LEN_100KM, + "100", "/data/100km.rkyv" ); diff --git a/src/graph.rs b/src/graph.rs index 8b1a8f6..c049bbd 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -58,22 +58,34 @@ pub const MAGIC: &[u8; 4] = b"RRG1"; /// On-disk schema version. Bump on incompatible layout changes. pub const SCHEMA_VERSION: u32 = 1; -/// Lon/lat coordinates of one graph node. f32 precision is ~3 m at 60°N -/// — well below the 5 km grid spacing. +/// Lon/lat coordinates of one graph node. +/// +/// `f32` precision is ~3 m at 60°N — well below the 5 km grid spacing, +/// so widening the fields would cost archive size for no positional +/// gain. Note the **field order is `lng` then `lat`**, matching the +/// source GeoPackage geometry; the public routing API +/// ([`crate::Graph::route`], [`crate::Route::coordinates`]) uses the +/// opposite `(lat, lng)` order. #[derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Clone, Copy, Debug)] #[rkyv(derive(Debug))] pub struct NodeCoord { + /// Longitude in decimal degrees, WGS84 (EPSG:4326), east-positive, + /// in `[-180, 180]`. pub lng: f32, + /// Latitude in decimal degrees, WGS84 (EPSG:4326), north-positive, + /// in `[-90, 90]`. pub lat: f32, } -/// One directed half-edge in the CSR adjacency. For an undirected edge -/// between distinct nodes A and B, two `DirectedEdge`s are emitted -/// (A→B, B→A) sharing the same `edge_id`. Self-loops (`A == B`) are the -/// one exception: they produce a single `DirectedEdge` with -/// `target == source`, because the "reverse" half would just duplicate -/// the forward one. Either way, the undirected edge has exactly one -/// entry in `Graph::edge_endpoints` and `Graph::undirected_weights`. +/// One directed half-edge in the CSR adjacency. +/// +/// For an undirected edge between distinct nodes A and B, two +/// `DirectedEdge`s are emitted (A→B, B→A) sharing the same `edge_id`. +/// Self-loops (`A == B`) are the one exception: they produce a single +/// `DirectedEdge` with `target == source`, because the "reverse" half +/// would just duplicate the forward one. Either way, the undirected edge +/// has exactly one entry in `Graph::edge_endpoints` and +/// `Graph::undirected_weights`. #[derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Clone, Copy, Debug)] #[rkyv(derive(Debug))] pub struct DirectedEdge { @@ -86,18 +98,33 @@ pub struct DirectedEdge { pub edge_id: u32, } -/// One named edge group (chokepoint / passage). `edge_ids` is sorted. +/// One named edge group (chokepoint / passage). +/// +/// The 13 groups are baked in a fixed order by `build/groups.rs`; +/// [`crate::Graph::edges_for_groups`] resolves names to the union of +/// their `edge_ids`. #[derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Clone, Debug)] #[rkyv(derive(Debug))] pub struct GroupEntry { + /// Stable lowerCamelCase group identifier as written by + /// `build/groups.rs` (e.g. `"suezCanal"`, `"panamaCanal"`). This is + /// the string [`crate::Graph::edges_for_groups`] matches on, so it is + /// part of the on-disk contract — changing one requires a + /// [`SCHEMA_VERSION`] bump. pub name: String, + /// Undirected edge ids belonging to this group — indices into + /// [`GraphData::edge_endpoints`] and + /// [`GraphData::undirected_weights`], **not** into + /// [`GraphData::edges`]. Sorted ascending and free of duplicates. pub edge_ids: Vec, } -/// The complete CSR graph for one resolution — the rkyv-serialised -/// schema struct written to and read from `.rkyv` archives. The -/// runtime API (`Graph`, `Graph::load`, `Graph::from_bytes`) lives in -/// `src/loader.rs`; `GraphData` is the underlying schema. +/// The complete CSR graph for one resolution. +/// +/// This is the rkyv-serialised schema struct written to and read from +/// `.rkyv` archives. The runtime API (`Graph`, `Graph::load`, +/// `Graph::from_bytes`) lives in `src/loader.rs`; `GraphData` is the +/// underlying schema. /// /// rkyv auto-derives `ArchivedGraphData` for this struct. See the /// `Graph::archived` accessor which returns `&ArchivedGraphData`. @@ -114,7 +141,7 @@ pub struct GraphData { /// Endpoints of each undirected edge: `(src_node_id, dst_node_id)`. /// Indexed by `DirectedEdge::edge_id`. pub edge_endpoints: Vec<(u32, u32)>, - /// Weight of each undirected edge (km). Indexed by edge_id. + /// Weight of each undirected edge (km). Indexed by `edge_id`. pub undirected_weights: Vec, /// 13 named groups in the fixed `EDGE_GROUPS` order. pub groups: Vec, From bb42003054766749e1d08bc8d25d2dd454b2a0e1 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:37:46 +0000 Subject: [PATCH 03/11] docs(api): ENG-4684 runnable doctests for the public API The ticket's headline deliverable. `cargo test --doc --all-features` goes from `1 ignored / 0 passed` to **15 passed / 0 ignored** in 0.25 s, with no fixture, no network and no `ignore` / `no_run` / `compile_fail` escape hatch anywhere. Every one of the nine public methods now carries an `# Examples` section that asserts real behaviour, plus the crate root and the `Graph` type: * `from_bytes` x2 -- loads the bundled 50 km archive; rejects `b"nope"` with `BadMagic`. * `load` x2 -- resolution is populated (50, vs 0 for `from_bytes`); `load(42)` is `UnknownResolution` before any I/O. * `resolution_km`, `node_count`, `edge_count`, `archived` -- pinned counts (7,390 nodes / 15,498 undirected edges / 13 groups). * `directed_edge_count` -- pins 30,976 and demonstrates the documented `edge_count..=2*edge_count` range: the 20-half-edge shortfall is 20 self-loops whose reverse half is suppressed. * `route` x2 -- Marseille to Shanghai is 16,354 km via Suez and >8,000 km longer with `suezCanal` blocked; a self-route is a single-coordinate zero-distance path; `(91.0, 0.0)` is `BadFromCoord`. * `edges_for_groups` -- two-group union is a superset of one, and `"Suez Canal"` is `UnknownGroup` (matching is byte-exact). * The `Graph` type's long-lived-handle example, previously fenced ```ignore, now runs -- it compiles and passes as written. The doc now states that it deliberately leaks the archive handle for the doctest process lifetime, so a reviewer does not have to discover that. Conventions: setup `use` lines and the trailing `Ok::<(), Box>(())` are hidden behind `#`; `?` throughout, never `.unwrap()`; intra-doc links rather than plain backticks. Examples prefer `Graph::from_bytes(data::BYTES_50KM)` over `Graph::load(50)` because `load`'s step 1 makes it sensitive to `$RUSTYROUTE_DATA_DIR` -- `load` is used only where the example is specifically about `load`, and both such examples say the variable must be unset and why. Also adds the `# Errors` sections the ticket asks for on `from_bytes` and `load` (enumerating every `LoadError` variant each can produce; `route` and `edges_for_groups` already had theirs), a real `# Panics` section on `archived` explaining that its `.expect` is unreachable via the public API, splits the over-long first doc paragraphs on `Graph`, `load`, `resolution_km`, `archived` and `directed_edge_count`, and backticks `OUT_DIR` in `LoadError::DataNotAvailable`. Verified: `cargo test --doc --all-features` 15 passed / 0 ignored (AC4); `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items --all-features` clean (AC1); `RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc --no-deps --all-features` clean (AC2); `cargo fmt --all --check` clean (AC10). Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- src/lib.rs | 34 ++++++ src/loader.rs | 284 ++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 295 insertions(+), 23 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9ecd633..7ea2bc9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,6 +18,40 @@ //! chokepoint/passage groups into an [`EdgeId`] set). Distance matrices //! and further algorithms follow in later tickets. See `README.md` and //! `NOTICE` for project status and upstream attribution. +//! +//! # Examples +//! +//! Route Marseille to Shanghai on the default 50 km graph, first +//! unrestricted and then with the Suez Canal closed: +//! +//! ``` +//! use std::collections::HashSet; +//! use rustyroute::{Graph, data}; +//! +//! let graph = Graph::from_bytes(data::BYTES_50KM)?; +//! let marseille = (43.30, 5.37); +//! let shanghai = (31.23, 121.47); +//! +//! let via_suez = graph.route(marseille, shanghai, &HashSet::new())?; +//! assert!((via_suez.distance_km - 16_354.0).abs() < 1.0); +//! +//! let suez = graph.edges_for_groups(["suezCanal"])?; +//! let round_the_cape = graph.route(marseille, shanghai, &suez)?; +//! assert!(round_the_cape.distance_km > 25_000.0); +//! # Ok::<(), Box>(()) +//! ``` +//! +//! On native targets [`Graph::load`] is the production entry point — it +//! mmaps the archive instead of baking it into the binary, and it is the +//! only constructor that knows the graph's resolution: +//! +//! ``` +//! # use rustyroute::Graph; +//! let graph = Graph::load(50)?; +//! assert_eq!(graph.resolution_km(), 50); +//! assert_eq!(graph.node_count(), 7_390); +//! # Ok::<(), Box>(()) +//! ``` #![deny( unsafe_code, diff --git a/src/loader.rs b/src/loader.rs index 492d963..f29e248 100644 --- a/src/loader.rs +++ b/src/loader.rs @@ -78,7 +78,7 @@ pub enum LoadError { UnknownResolution(u32), /// The requested resolution is allowed, but no source (env var, - /// in-tree OUT_DIR, or static feature) was available. Enable the + /// in-tree `OUT_DIR`, or static feature) was available. Enable the /// matching `data-{N}km` feature or set `$RUSTYROUTE_DATA_DIR`. #[error( "data not available for {0}km — enable the `data-{0}km` feature or \ @@ -111,10 +111,12 @@ pub enum LoadError { InvalidArchive(rkyv::rancor::Error), } -/// Owned handle to a loaded graph archive. Owns its backing buffer -/// (an mmap on native, a `&'static [u8]` for `from_bytes` and wasm -/// targets) and exposes `archived(&self) -> &ArchivedGraphData` tied -/// to the handle's lifetime. +/// Owned handle to a loaded graph archive. +/// +/// Owns its backing buffer (an mmap on native, a `&'static [u8]` for +/// [`Graph::from_bytes`] and wasm targets) and exposes +/// [`archived`](Graph::archived) — returning an [`ArchivedGraphData`] +/// reference tied to the handle's lifetime. /// /// `Graph` is `Send + Sync` (both backings are). It is NOT `Clone`: /// consumers who need multiple handles should wrap in @@ -126,7 +128,7 @@ pub enum LoadError { /// process lifetime (routefinder), stash the `Graph` in a /// `OnceLock` and `Box::leak` it to obtain `&'static Graph`: /// -/// ```ignore +/// ``` /// use std::sync::OnceLock; /// use rustyroute::Graph; /// @@ -137,11 +139,18 @@ pub enum LoadError { /// Box::leak(Box::new(g)) /// }) /// } +/// +/// // Every call returns the same `&'static` handle. +/// assert_eq!(graph().resolution_km(), 50); +/// assert!(std::ptr::eq(graph(), graph())); /// ``` /// /// This deliberately leaks the graph for the process lifetime — that /// is the trade-off for avoiding per-call lifetime annotations on -/// downstream routing APIs. +/// downstream routing APIs. Running the example above as a doctest +/// therefore leaks the ~3 MB 50 km archive handle until the doctest +/// process exits, which is exactly the intended behaviour and not a +/// bug the example is hiding. pub struct Graph { backing: GraphBacking, resolution_km: u32, @@ -173,9 +182,47 @@ impl Graph { /// /// Validates the 4-byte magic, the 4-byte little-endian schema /// version, and then runs rkyv's checked `access` on the - /// remainder. The returned handle's `resolution_km()` is `0` + /// remainder. The returned handle's [`resolution_km`] is `0` /// because the archive bytes do not carry the resolution; use /// [`Graph::load`] when you need that field populated. + /// + /// The intended argument is one of the feature-gated + /// [`crate::data`] slices, which are 4-byte aligned for rkyv's + /// relative pointers. A slice re-borrowed from a `Vec` may not + /// be, and then fails with [`LoadError::InvalidArchive`]. + /// + /// # Errors + /// + /// - [`LoadError::BadMagic`] if the first four bytes are not + /// `b"RRG1"` — including when `bytes` is shorter than the 8-byte + /// header, which reports a zero array. + /// - [`LoadError::UnsupportedSchema`] if bytes `4..8` do not decode + /// to this build's [`SCHEMA_VERSION`]. + /// - [`LoadError::InvalidArchive`] if rkyv's checked access rejects + /// the payload: truncation, byte tampering, or misalignment. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, data}; + /// let graph = Graph::from_bytes(data::BYTES_50KM)?; + /// assert_eq!(graph.node_count(), 7_390); + /// // The archive carries no resolution — see `resolution_km`. + /// assert_eq!(graph.resolution_km(), 0); + /// # Ok::<(), Box>(()) + /// ``` + /// + /// A truncated or tampered slice is rejected rather than trusted: + /// + /// ``` + /// # use rustyroute::{Graph, LoadError}; + /// assert!(matches!( + /// Graph::from_bytes(b"nope"), + /// Err(LoadError::BadMagic(_)) + /// )); + /// ``` + /// + /// [`resolution_km`]: Graph::resolution_km pub fn from_bytes(bytes: &'static [u8]) -> Result { validate_header(bytes)?; // Checked access — surfaces InvalidArchive on tampering. @@ -187,13 +234,61 @@ impl Graph { }) } - /// Load a graph by resolution_km. Tries, in order: + /// Load a graph by resolution in kilometres. + /// + /// Tries, in order: /// 1. `$RUSTYROUTE_DATA_DIR/{N}km.rkyv` (if env var is set; /// missing file → [`LoadError::DataFileMissing`]) /// 2. `$OUT_DIR/data/{N}km.rkyv` baked at rustyroute compile time /// 3. `data::BYTES_{N}KM` static fallback (if the matching /// `data-{N}km` feature is enabled) /// 4. [`LoadError::DataNotAvailable`] + /// + /// Step 1 is an unconditional override, not a preference: when + /// `$RUSTYROUTE_DATA_DIR` is set and does not contain the requested + /// file, `load` fails instead of falling through to steps 2 and 3. + /// Reach for [`Graph::from_bytes`] if you want a path that ignores + /// the environment entirely. + /// + /// # Errors + /// + /// - [`LoadError::UnknownResolution`] if `resolution_km` is not one + /// of 5, 10, 20, 50, 100. + /// - [`LoadError::DataFileMissing`] if `$RUSTYROUTE_DATA_DIR` is set + /// but holds no `{resolution_km}km.rkyv`. + /// - [`LoadError::Io`] for any other failure opening or mapping a + /// located file. + /// - [`LoadError::BadMagic`], [`LoadError::UnsupportedSchema`] or + /// [`LoadError::InvalidArchive`] if a file was located but failed + /// validation — same checks as [`Graph::from_bytes`]. + /// - [`LoadError::DataNotAvailable`] if the resolution is supported + /// but no source resolved. + /// + /// # Examples + /// + /// This example requires `$RUSTYROUTE_DATA_DIR` to be **unset** — see + /// the note on step 1 above. With the variable unset it needs no + /// fixture and no network: step 2's `$OUT_DIR` is resolved at + /// *rustyroute's* compile time, so it still points at the archives + /// `build.rs` baked even when the caller is a separate crate. + /// + /// ``` + /// # use rustyroute::Graph; + /// let graph = Graph::load(50)?; + /// assert_eq!(graph.resolution_km(), 50); + /// assert_eq!(graph.node_count(), 7_390); + /// # Ok::<(), Box>(()) + /// ``` + /// + /// An unsupported resolution is rejected before any I/O: + /// + /// ``` + /// # use rustyroute::{Graph, LoadError}; + /// assert!(matches!( + /// Graph::load(42), + /// Err(LoadError::UnknownResolution(42)) + /// )); + /// ``` #[cfg(not(target_arch = "wasm32"))] pub fn load(resolution_km: u32) -> Result { const ALLOWED: &[u32] = &[5, 10, 20, 50, 100]; @@ -269,17 +364,54 @@ impl Graph { }) } - /// Resolution in kilometres. Returns 0 for graphs constructed via - /// [`Graph::from_bytes`] (the archive header does not carry the - /// resolution; only [`Graph::load`] populates it). + /// Resolution in kilometres. + /// + /// Returns 0 for graphs constructed via [`Graph::from_bytes`] — the + /// archive header does not carry the resolution, so only + /// [`Graph::load`] can populate it. Treat 0 as "unknown", not as a + /// zero-kilometre grid. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, data}; + /// assert_eq!(Graph::load(50)?.resolution_km(), 50); + /// assert_eq!(Graph::from_bytes(data::BYTES_50KM)?.resolution_km(), 0); + /// # Ok::<(), Box>(()) + /// ``` pub fn resolution_km(&self) -> u32 { self.resolution_km } - /// Access the rkyv-archived form of the graph. The reference is - /// tied to `&self` — do not attempt to outlive the Graph handle. - /// Re-runs rkyv's checked access each call; cache into a local - /// `let g = self.archived();` if you intend to hot-loop. + /// Access the rkyv-archived form of the graph. + /// + /// The reference is tied to `&self` — do not attempt to outlive the + /// `Graph` handle. Re-runs rkyv's checked access each call; cache + /// into a local `let g = self.archived();` if you intend to + /// hot-loop. + /// + /// # Panics + /// + /// Panics if rkyv's checked access rejects the payload. This cannot + /// be triggered through the public API: [`Graph::from_bytes`] and + /// [`Graph::load`] both run the same checked access before handing + /// back a handle, and the backing bytes are treated as immutable for + /// the handle's lifetime. It would fire only if the mapped file were + /// mutated in place behind the mmap, which the SAFETY note on + /// `load_file` documents as the operator's responsibility. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, data}; + /// let graph = Graph::from_bytes(data::BYTES_50KM)?; + /// let archived = graph.archived(); + /// assert_eq!(archived.nodes.len(), graph.node_count() as usize); + /// // The 13 baked-in chokepoint/passage groups. + /// assert_eq!(archived.groups.len(), 13); + /// assert_eq!(archived.groups[0].name.as_str(), "suezCanal"); + /// # Ok::<(), Box>(()) + /// ``` pub fn archived(&self) -> &ArchivedGraphData { let payload: &[u8] = match &self.backing { #[cfg(not(target_arch = "wasm32"))] @@ -290,22 +422,55 @@ impl Graph { .expect("validated on construction; payload bytes are immutable") } - /// Number of distinct nodes in the graph. + /// Number of distinct nodes in the graph. Valid [`NodeId`]s are + /// `0..node_count()`. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, data}; + /// assert_eq!(Graph::from_bytes(data::BYTES_50KM)?.node_count(), 7_390); + /// # Ok::<(), Box>(()) + /// ``` pub fn node_count(&self) -> u32 { self.archived().nodes.len() as u32 } /// Number of undirected edges (distinct - /// `(src_node_id, dst_node_id)` endpoints). + /// `(src_node_id, dst_node_id)` endpoints). Valid [`EdgeId`]s are + /// `0..edge_count()`. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, data}; + /// assert_eq!(Graph::from_bytes(data::BYTES_50KM)?.edge_count(), 15_498); + /// # Ok::<(), Box>(()) + /// ``` pub fn edge_count(&self) -> u32 { self.archived().edge_endpoints.len() as u32 } - /// Number of directed half-edges in the CSR adjacency. For - /// non-self-loop undirected edges this is `2 * edge_count`; for - /// self-loops the forward half is emitted once and the reverse - /// is suppressed, so `directed_edge_count` ranges between - /// `edge_count` (all self-loops) and `2 * edge_count`. + /// Number of directed half-edges in the CSR adjacency. + /// + /// For non-self-loop undirected edges this is + /// `2 * edge_count`; for self-loops the forward half is emitted once + /// and the reverse is suppressed, so `directed_edge_count` ranges + /// between `edge_count` (all self-loops) and `2 * edge_count`. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, data}; + /// let graph = Graph::from_bytes(data::BYTES_50KM)?; + /// assert_eq!(graph.directed_edge_count(), 30_976); + /// // Inside the documented range. The 20-half-edge shortfall against + /// // `2 * edge_count` is 20 self-loops, whose reverse half is + /// // suppressed. + /// assert!(graph.directed_edge_count() <= 2 * graph.edge_count()); + /// assert_eq!(2 * graph.edge_count() - graph.directed_edge_count(), 20); + /// # Ok::<(), Box>(()) + /// ``` pub fn directed_edge_count(&self) -> u32 { self.archived().edges.len() as u32 } @@ -417,6 +582,51 @@ impl Graph { /// each hop Dijkstra already relaxed across; the internal /// `expect` guards that invariant and firing it would signal graph /// corruption, not a caller error. + /// + /// # Examples + /// + /// Marseille to Shanghai, then the same voyage with the Suez Canal + /// closed. The detour round the Cape of Good Hope costs roughly + /// 8,700 km: + /// + /// ``` + /// # use std::collections::HashSet; + /// # use rustyroute::{Graph, data}; + /// let graph = Graph::from_bytes(data::BYTES_50KM)?; + /// let marseille = (43.30, 5.37); + /// let shanghai = (31.23, 121.47); + /// + /// let via_suez = graph.route(marseille, shanghai, &HashSet::new())?; + /// assert!((via_suez.distance_km - 16_354.0).abs() < 1.0); + /// // One coordinate per node, one edge per hop between them. + /// assert_eq!(via_suez.coordinates.len(), via_suez.edge_ids.len() + 1); + /// + /// let suez = graph.edges_for_groups(["suezCanal"])?; + /// let round_the_cape = graph.route(marseille, shanghai, &suez)?; + /// assert!(round_the_cape.distance_km > via_suez.distance_km + 8_000.0); + /// # Ok::<(), Box>(()) + /// ``` + /// + /// Endpoints are validated before any search runs, and both are + /// snapped to the nearest node — so a self-route is a zero-distance + /// single-coordinate path rather than an error: + /// + /// ``` + /// # use std::collections::HashSet; + /// # use rustyroute::{Graph, RouteError, data}; + /// let graph = Graph::from_bytes(data::BYTES_50KM)?; + /// let gibraltar = (36.0, -5.5); + /// + /// let here = graph.route(gibraltar, gibraltar, &HashSet::new())?; + /// assert_eq!(here.coordinates.len(), 1); + /// assert!(here.edge_ids.is_empty()); + /// + /// assert!(matches!( + /// graph.route((91.0, 0.0), gibraltar, &HashSet::new()), + /// Err(RouteError::BadFromCoord(_)) + /// )); + /// # Ok::<(), Box>(()) + /// ``` pub fn route( &self, from: (f64, f64), @@ -509,10 +719,38 @@ impl Graph { /// Collect the union of undirected edge ids for the named edge /// groups (the 13 baked-in chokepoints/passages). /// + /// The result is meant to be handed straight to [`Graph::route`] as + /// its `blocked` set. Names are matched byte-exactly against + /// [`GroupEntry::name`], so the whole call fails on the first + /// unrecognised name rather than silently blocking nothing. + /// /// # Errors /// /// [`RouteError::UnknownGroup`] if any name does not match a baked-in /// group. + /// + /// # Examples + /// + /// ``` + /// # use rustyroute::{Graph, RouteError, data}; + /// let graph = Graph::from_bytes(data::BYTES_50KM)?; + /// + /// let chokepoints = graph.edges_for_groups(["suezCanal", "panamaCanal"])?; + /// assert!(!chokepoints.is_empty()); + /// // The union is deduplicated, so two groups yield at most the sum + /// // of their sizes. + /// let suez = graph.edges_for_groups(["suezCanal"])?; + /// assert!(chokepoints.is_superset(&suez)); + /// + /// // Matching is exact — no case folding, no whitespace tolerance. + /// assert!(matches!( + /// graph.edges_for_groups(["Suez Canal"]), + /// Err(RouteError::UnknownGroup(name)) if name == "Suez Canal" + /// )); + /// # Ok::<(), Box>(()) + /// ``` + /// + /// [`GroupEntry::name`]: crate::graph::GroupEntry::name pub fn edges_for_groups<'a>( &self, names: impl IntoIterator, From be3d443c6e3cc4dc94cf9b547f8010e4379e2a5c Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:41:56 +0000 Subject: [PATCH 04/11] refactor: ENG-4684 library pedantic/nursery fixes The library target now reports **zero** clippy findings under pedantic + nursery. Fixed where the fix is a strict improvement, allowed with a stated reason where the current form is deliberate. Fixed: * `use_self` x5 -- `Self` in `from_bytes`/`load`/`load_file` return types and the two struct literals. * `must_use_candidate` x5 -- `#[must_use]` on the getters and on `route`, whose message says why (computing a Route and dropping it does no work). * `missing_const_for_fn` -- `resolution_km` is now `const fn` (verified on MSRV 1.93.0). * `redundant_closure_for_method_calls` x3 -- `std::sync::PoisonError::into_inner` in the unit-test env lock. * `doc_markdown` -- `GeoPackage` in `NodeCoord`, `OUT_DIR` in the unit tests, and `DataFileMissing` upgraded to an intra-doc link. Allowed, each with the reason immediately above it: * `suboptimal_flops` on `haversine_km` -- MUST NOT be "fixed". `f64::mul_add` is a single fused rounding step and returns bit-different results, which would shift nearest-node snapping and drift the golden distance table in tests/fixtures/routes.json. tests/golden_routes.rs is the tripwire and stays green (8 passed). * `needless_pass_by_value` on `load_file` -- taking `File` by value is deliberate ownership transfer: the mapping must outlive the handle, and `&File` would let a caller mutate the file under a mapping the SAFETY note assumes immutable. * `cast_possible_truncation` x3 in the count getters -- node indices and `EdgeId` are `u32` by the on-disk schema, so neither table can exceed `u32::MAX`. * `missing_const_for_fn` on `data::bytes_for` -- deliberately NOT made `const fn`. tests/data_module.rs:31-33 asserts on the literal source text `"pub(crate) fn bytes_for"` as an ENG-4688 wasm-gate guard; adding `const` renames the signature out from under it, trading a live regression test for a nursery cosmetic. tests/data_module.rs is therefore NOT modified by this ticket and passes unchanged. Also fixes the `$OUT_DIR/data_lens.rs` generator (build/mod.rs) to emit `#[allow(dead_code, clippy::unreadable_literal)]`. The five raw archive byte counts are `include!`d into src/data.rs, so the warnings land on the *library* with spans in a generated file nobody can edit -- which is why this one line of the build-script story has to ride with the library fix that its absence would block. Verified: library target has 0 clippy findings under `-W clippy::pedantic -W clippy::nursery` (all remaining diagnostics are `custom-build`, next commit); `cargo test --test data_module --test golden_routes --test route_smoke --all-features` 15 passed; `cargo test --doc --all-features` still 15 passed / 0 ignored; `cargo fmt --all --check` clean. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- build/mod.rs | 8 +++++- src/data.rs | 11 +++++++- src/graph.rs | 2 +- src/loader.rs | 76 +++++++++++++++++++++++++++++++++++++++++---------- 4 files changed, 80 insertions(+), 17 deletions(-) diff --git a/build/mod.rs b/build/mod.rs index 267620f..3c23468 100644 --- a/build/mod.rs +++ b/build/mod.rs @@ -71,9 +71,15 @@ pub fn run() { .len(); // Each const is referenced only when its matching `data-{N}km` // feature is enabled, so the `#[allow(dead_code)]` is required. + // `clippy::unreadable_literal` is allowed for the same reason it + // cannot be fixed at the call site: these are raw byte counts in + // a generated file under `$OUT_DIR` that no human edits, and they + // are `include!`d into `src/data.rs`, so the warning is reported + // against the library with a span nobody can act on. writeln!( lens, - "#[allow(dead_code)]\npub(crate) const DATA_LEN_{n}KM: usize = {archive_len};", + "#[allow(dead_code, clippy::unreadable_literal)]\n\ + pub(crate) const DATA_LEN_{n}KM: usize = {archive_len};", ) .expect("write to String cannot fail"); } diff --git a/src/data.rs b/src/data.rs index b954d3c..76cc542 100644 --- a/src/data.rs +++ b/src/data.rs @@ -171,7 +171,16 @@ define_bytes!( /// smoke build. wasm consumers use `Graph::from_bytes`, which takes /// the byte slice directly and never goes through this helper. #[cfg(not(target_arch = "wasm32"))] -#[allow(unused_variables)] // when no data-* feature is enabled, param is unused +// When no `data-*` feature is enabled, every match arm below is +// `cfg`-ed out and the parameter goes unused. +#[allow(unused_variables)] +// NOT made `const fn`, even though it could be. `tests/data_module.rs`'s +// ENG-4688 wasm-gate guard asserts on the literal source text +// `"pub(crate) fn bytes_for"` and walks the attribute run above it to +// prove the `#[cfg(not(target_arch = "wasm32"))]` is still attached. +// Adding `const` renames the signature out from under that guard, which +// would trade a live regression test for a nursery cosmetic. +#[allow(clippy::missing_const_for_fn)] pub(crate) fn bytes_for(resolution_km: u32) -> Option<&'static [u8]> { match resolution_km { #[cfg(feature = "data-5km")] diff --git a/src/graph.rs b/src/graph.rs index c049bbd..c9f0317 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -63,7 +63,7 @@ pub const SCHEMA_VERSION: u32 = 1; /// `f32` precision is ~3 m at 60°N — well below the 5 km grid spacing, /// so widening the fields would cost archive size for no positional /// gain. Note the **field order is `lng` then `lat`**, matching the -/// source GeoPackage geometry; the public routing API +/// source `GeoPackage` geometry; the public routing API /// ([`crate::Graph::route`], [`crate::Route::coordinates`]) uses the /// opposite `(lat, lng)` order. #[derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, Clone, Copy, Debug)] diff --git a/src/loader.rs b/src/loader.rs index f29e248..f505d00 100644 --- a/src/loader.rs +++ b/src/loader.rs @@ -223,12 +223,12 @@ impl Graph { /// ``` /// /// [`resolution_km`]: Graph::resolution_km - pub fn from_bytes(bytes: &'static [u8]) -> Result { + pub fn from_bytes(bytes: &'static [u8]) -> Result { validate_header(bytes)?; // Checked access — surfaces InvalidArchive on tampering. let _ = rkyv::access::(&bytes[8..]) .map_err(LoadError::InvalidArchive)?; - Ok(Graph { + Ok(Self { backing: GraphBacking::Static(bytes), resolution_km: 0, }) @@ -290,7 +290,7 @@ impl Graph { /// )); /// ``` #[cfg(not(target_arch = "wasm32"))] - pub fn load(resolution_km: u32) -> Result { + pub fn load(resolution_km: u32) -> Result { const ALLOWED: &[u32] = &[5, 10, 20, 50, 100]; if !ALLOWED.contains(&resolution_km) { return Err(LoadError::UnknownResolution(resolution_km)); @@ -340,7 +340,14 @@ impl Graph { } #[cfg(not(target_arch = "wasm32"))] - fn load_file(file: std::fs::File, resolution_km: u32) -> Result { + // Taking `File` by value is deliberate ownership transfer, not an + // oversight: `memmap2::Mmap::map` only borrows the handle, but the + // mapping must outlive it, so `load_file` becomes the sole owner and + // drops the descriptor at the end of this frame. Passing `&File` + // would let a caller keep the handle alive and mutate the file + // underneath a mapping the SAFETY note below assumes is immutable. + #[allow(clippy::needless_pass_by_value)] + fn load_file(file: std::fs::File, resolution_km: u32) -> Result { // SAFETY: memmap2::Mmap::map is unsafe because the kernel can // change the underlying file bytes out from under us. We treat // the mmap as immutable for the lifetime of the Graph: this @@ -358,7 +365,7 @@ impl Graph { let _ = rkyv::access::(&mmap[8..]) .map_err(LoadError::InvalidArchive)?; - Ok(Graph { + Ok(Self { backing: GraphBacking::Mmap(mmap), resolution_km, }) @@ -379,7 +386,8 @@ impl Graph { /// assert_eq!(Graph::from_bytes(data::BYTES_50KM)?.resolution_km(), 0); /// # Ok::<(), Box>(()) /// ``` - pub fn resolution_km(&self) -> u32 { + #[must_use] + pub const fn resolution_km(&self) -> u32 { self.resolution_km } @@ -412,6 +420,7 @@ impl Graph { /// assert_eq!(archived.groups[0].name.as_str(), "suezCanal"); /// # Ok::<(), Box>(()) /// ``` + #[must_use] pub fn archived(&self) -> &ArchivedGraphData { let payload: &[u8] = match &self.backing { #[cfg(not(target_arch = "wasm32"))] @@ -432,8 +441,15 @@ impl Graph { /// assert_eq!(Graph::from_bytes(data::BYTES_50KM)?.node_count(), 7_390); /// # Ok::<(), Box>(()) /// ``` + #[must_use] pub fn node_count(&self) -> u32 { - self.archived().nodes.len() as u32 + // A node index is `u32` by the on-disk schema + // (`GraphData::node_offsets: Vec`), so the table can never + // hold more entries than `u32::MAX`. + #[allow(clippy::cast_possible_truncation)] + { + self.archived().nodes.len() as u32 + } } /// Number of undirected edges (distinct @@ -447,8 +463,15 @@ impl Graph { /// assert_eq!(Graph::from_bytes(data::BYTES_50KM)?.edge_count(), 15_498); /// # Ok::<(), Box>(()) /// ``` + #[must_use] pub fn edge_count(&self) -> u32 { - self.archived().edge_endpoints.len() as u32 + // An `EdgeId` is `u32` by the on-disk schema + // (`DirectedEdge::edge_id: u32`), so the endpoint table can never + // hold more entries than `u32::MAX`. + #[allow(clippy::cast_possible_truncation)] + { + self.archived().edge_endpoints.len() as u32 + } } /// Number of directed half-edges in the CSR adjacency. @@ -471,8 +494,15 @@ impl Graph { /// assert_eq!(2 * graph.edge_count() - graph.directed_edge_count(), 20); /// # Ok::<(), Box>(()) /// ``` + #[must_use] pub fn directed_edge_count(&self) -> u32 { - self.archived().edges.len() as u32 + // CSR row pointers into this table are `u32` by the on-disk + // schema (`GraphData::node_offsets: Vec`), so it can never + // hold more entries than `u32::MAX`. + #[allow(clippy::cast_possible_truncation)] + { + self.archived().edges.len() as u32 + } } } @@ -518,6 +548,16 @@ fn scale_km(weight_km: f32) -> u64 { /// into the library), but note the **argument order differs**: the /// build function takes `(lng, lat)`, whereas this one takes /// `(lat, lng)` to match the public [`Graph::route`] coordinate order. +// MUST NOT be "fixed" to `mul_add`. `f64::mul_add` performs a single +// fused rounding step, so it returns a bit-for-bit *different* result +// from a separate multiply and add. That would shift nearest-node +// snapping at `Graph::route`'s endpoints and drift the golden distance +// table in `tests/fixtures/routes.json` (`tests/golden_routes.rs` is the +// tripwire). The same reasoning applies to the identical expression in +// `build/geometry.rs::haversine_km`, where a change would additionally +// rebake the edge weights in every `.rkyv` archive. Accuracy is not the +// binding constraint here; reproducibility against the baked archives is. +#[allow(clippy::suboptimal_flops)] fn haversine_km(lat1: f64, lng1: f64, lat2: f64, lng2: f64) -> f64 { let (lat1_r, lat2_r) = (lat1.to_radians(), lat2.to_radians()); let dlat = (lat2 - lat1).to_radians(); @@ -627,6 +667,7 @@ impl Graph { /// )); /// # Ok::<(), Box>(()) /// ``` + #[must_use = "the computed Route is the only result; dropping it does no work"] pub fn route( &self, from: (f64, f64), @@ -849,13 +890,15 @@ mod tests { /// `set_var`/`remove_var` require for soundness. static ENV_LOCK: Mutex<()> = Mutex::new(()); - /// AC3: with no env var and OUT_DIR step disabled, the + /// AC3: with no env var and `OUT_DIR` step disabled, the /// static-fallback satisfies `load(50)` under default features /// (`data-50km`). #[test] #[cfg(feature = "data-50km")] fn load_50km_falls_through_to_static() { - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); // SAFETY: the ENV_LOCK guard above ensures this test is the // only thread mutating env state or `skip_out_dir` for its // duration, satisfying Rust 2024's single-threaded-mutation @@ -874,7 +917,9 @@ mod tests { #[test] #[cfg(not(feature = "data-50km"))] fn load_50km_data_not_available_when_feature_off() { - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); // SAFETY: the ENV_LOCK guard above ensures this test is the // only thread mutating env state or `skip_out_dir` for its // duration, satisfying Rust 2024's single-threaded-mutation @@ -889,10 +934,13 @@ mod tests { } } - /// `$RUSTYROUTE_DATA_DIR` set to a non-existent dir → DataFileMissing. + /// `$RUSTYROUTE_DATA_DIR` set to a non-existent dir → + /// [`LoadError::DataFileMissing`]. #[test] fn load_50km_data_file_missing_when_env_dir_empty() { - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let tmp = std::env::temp_dir().join("rustyroute_test_nonexistent_dir"); // SAFETY: the ENV_LOCK guard above ensures this test is the // only thread mutating env state for its duration, satisfying From 7bdaff224dc05b3bd1bcc0e3522d993f4f70052f Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:44:24 +0000 Subject: [PATCH 05/11] style: ENG-4684 build-script pedantic clean This is where the ticket's acceptance command actually dies: `cargo clippy --all-targets --all-features -- -D warnings -W clippy::pedantic` exited 101 on 28 build-script errors and never reached the library, which is why the build crate could not be deferred to a follow-up. Fixed (strict improvements): * `doc_markdown` x14 -- backticks on `GeoPackage(s)`, `SQLite`, `LineString`, `RawEdge`, `SCHEMA_VERSION`, `ArchivedGraphData` across build.rs and six build/ modules. * `uninlined_format_args` x4 -- build/mod.rs x3, build/groups.rs x1. Allowed as **module-level inner attributes** in each build/*.rs, each preceded by its reason. Placement is the point: build/csr.rs, build/geometry.rs, build/groups.rs and build/gpkg_io.rs are compiled into the build script *and* into four test crates that re-include them via `#[path]`, and an inner attribute on a module file is scoped to the module, so one edit covers every including crate. The alternative -- repeating each allow at five crate roots -- drifts on the first edit. * `cast_possible_truncation` x9 (csr.rs, groups.rs) -- every `usize as u32` narrows a node id, edge id or CSR row pointer, all `u32` in the on-disk schema; every `f64 as f32` narrows a coordinate or haversine weight, and `f32` is the schema's deliberate precision choice. * `must_use_candidate` / `missing_panics_doc` / `missing_errors_doc` -- these fire only in the test-crate inclusion context, where a `pub fn` of the module becomes public API of a crate rooted at a test file. They are build-time internals with one production caller each. * `match_same_arms` at build/geometry.rs (site allow) -- `2 => 6, 3 => 6` transcribes the GeoPackage envelope-indicator table row for row (2 = XYZ, 3 = XYM). Merging to `2 | 3` would hide that correspondence. `suboptimal_flops` at build/geometry.rs:79 is nursery, outside the AC's command, and deliberately left alone: `mul_add` would change the haversine edge weights baked into every .rkyv archive. Verified: `cargo clippy --all-targets --all-features -- -D warnings -W clippy::pedantic` reports nothing under build.rs or build/; `cargo test --test build_artifacts --test graph_types --test golden_routes --test group_assignment --all-features` 23 passed -- the archives are rebaked byte-identically and the golden distance table is unchanged; `cargo fmt --all --check` clean. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- build.rs | 2 +- build/archive.rs | 4 ++-- build/csr.rs | 31 +++++++++++++++++++++++++++++-- build/geometry.rs | 24 +++++++++++++++++++++++- build/gpkg.rs | 6 +++--- build/gpkg_io.rs | 13 ++++++++++++- build/groups.rs | 18 +++++++++++++++--- build/mod.rs | 9 +++------ 8 files changed, 88 insertions(+), 19 deletions(-) diff --git a/build.rs b/build.rs index e3c1b24..67c219e 100644 --- a/build.rs +++ b/build.rs @@ -1,4 +1,4 @@ -//! ENG-4678: compile vendored Eurostat MARNET GeoPackages into rkyv +//! ENG-4678: compile vendored Eurostat MARNET `GeoPackages` into rkyv //! graph archives + emit `pub const EDGE_GROUPS` for the crate root. //! //! Reads each `vendor/eurostat-marnet/marnet_plus_{N}km.gpkg`, builds diff --git a/build/archive.rs b/build/archive.rs index 276bebe..5164a9f 100644 --- a/build/archive.rs +++ b/build/archive.rs @@ -2,8 +2,8 @@ //! //! File layout (matches `src/graph.rs` docs): //! bytes 0..4 : ASCII magic b"RRG1" -//! bytes 4..8 : SCHEMA_VERSION as u32 LE -//! bytes 8.. : rkyv bytes of ArchivedGraphData +//! bytes 4..8 : `SCHEMA_VERSION` as u32 LE +//! bytes 8.. : rkyv bytes of `ArchivedGraphData` use crate::graph::{GraphData, MAGIC, SCHEMA_VERSION}; use std::path::Path; diff --git a/build/csr.rs b/build/csr.rs index 82f7155..3315d44 100644 --- a/build/csr.rs +++ b/build/csr.rs @@ -1,6 +1,33 @@ -//! Dedupe LineString endpoints into a node table and build the CSR +//! Dedupe `LineString` endpoints into a node table and build the CSR //! adjacency for one resolution. +// ENG-4684 module-level lint posture. Inner attributes on a module FILE +// are scoped to that module, so one edit here covers every crate this +// file is compiled into: the build script (via `build.rs`'s `#[path]` +// mod) and the integration tests that re-include it the same way +// (`tests/graph_load.rs`, `tests/build_helpers.rs`, +// `tests/group_assignment.rs`, `tests/tampered_gpkg_panic.rs`). The +// alternative -- repeating each allow at five crate roots -- drifts. +// +// Every `usize as u32` below narrows a node id, an edge id or a CSR row +// pointer, all of which are `u32` in the on-disk schema +// (`src/graph.rs`), so a table that overflowed the cast would already +// have failed to serialise. Every `f64 as f32` narrows a coordinate or a +// haversine weight, and `f32` is the schema's deliberate precision +// choice (~3 m at 60°N, far below the 5 km grid). +#![allow(clippy::cast_possible_truncation)] +// The remaining three fire only in the test-crate inclusion context, +// where a `pub fn` of this module becomes public API of a crate rooted +// at a test file. These are build-time internals with exactly one +// production caller each (`build/mod.rs::run`); documenting them as a +// published API surface, or annotating them `#[must_use]` for callers +// that do not exist, would be ceremony with no reader. +#![allow( + clippy::must_use_candidate, + clippy::missing_panics_doc, + clippy::missing_errors_doc +)] + use crate::build::geometry::polyline_length_km; use crate::build::gpkg::RawEdge; use crate::graph::{DirectedEdge, GraphData, GroupEntry, NodeCoord}; @@ -12,7 +39,7 @@ pub struct CsrBuilt { pub edges: Vec, pub edge_endpoints: Vec<(u32, u32)>, pub undirected_weights: Vec, - /// Parallel to `edge_endpoints` / `undirected_weights`: which RawEdge + /// Parallel to `edge_endpoints` / `undirected_weights`: which `RawEdge` /// each undirected edge id came from. Used by group assignment. pub raw_edge_index: Vec, } diff --git a/build/geometry.rs b/build/geometry.rs index 85b6299..a515745 100644 --- a/build/geometry.rs +++ b/build/geometry.rs @@ -1,4 +1,4 @@ -//! Parse GeoPackage Binary (GPB) + WKB LineString, compute haversine +//! Parse `GeoPackage` Binary (GPB) + WKB `LineString`, compute haversine //! distances, and clip line segments against an axis-aligned bbox via //! Liang-Barsky. //! @@ -7,6 +7,21 @@ //! flags to compute the WKB offset generically — robust against any //! future re-vendor that uses a different envelope shape. +// ENG-4684: see `build/csr.rs` for why these are module-level inner +// attributes rather than crate-root allows -- this file is compiled into +// the build script and into four test crates that re-include it via +// `#[path]`. +// +// These fire only in the test-crate inclusion context, where a `pub fn` +// of this module becomes public API of a crate rooted at a test file. +// The functions are build-time geometry internals, not a published +// surface. +#![allow( + clippy::must_use_candidate, + clippy::missing_panics_doc, + clippy::missing_errors_doc +)] + /// Earth radius (km). The IUGG mean radius — sufficient precision for a /// 5 km grid. pub const EARTH_RADIUS_KM: f64 = 6371.0088; @@ -34,6 +49,13 @@ pub fn parse_gpb_linestring(blob: &[u8]) -> Result, String> { return Err("GPB empty flag set; expected non-empty LineString".into()); } let env_indicator = ((flags >> 1) & 0x07) as usize; + // `2 => 6, 3 => 6` are deliberately not merged into `2 | 3 => 6`. + // This match transcribes the GeoPackage envelope-indicator table + // (indicator 2 = XYZ, indicator 3 = XYM) row for row; the two shapes + // happen to carry the same float count but are different envelopes, + // and collapsing them would hide that correspondence from the next + // reader checking this against the spec. + #[allow(clippy::match_same_arms)] let env_floats: usize = match env_indicator { 0 => 0, 1 => 4, diff --git a/build/gpkg.rs b/build/gpkg.rs index d98b0a9..4d2388e 100644 --- a/build/gpkg.rs +++ b/build/gpkg.rs @@ -2,16 +2,16 @@ //! //! `RawEdge` is plain data and is re-included via `#[path]` by tests //! that exercise the build pipeline (`tests/group_assignment.rs`, -//! `tests/tampered_gpkg_panic.rs`). The GeoPackage SQLite reader +//! `tests/tampered_gpkg_panic.rs`). The `GeoPackage` `SQLite` reader //! (`iter_edges`) lives in `build/gpkg_io.rs`; `rusqlite` is declared //! under both `[build-dependencies]` and `[dev-dependencies]` so the //! end-to-end tampered-gpkg test can re-include `gpkg_io.rs` and drive -//! the real reader. Tests that don't need the SQLite reader (e.g. +//! the real reader. Tests that don't need the `SQLite` reader (e.g. //! `group_assignment.rs`) construct `RawEdge` values directly instead. pub struct RawEdge { pub fid: i64, pub pass: Option, - /// LineString vertices in (lng, lat) order. Length >= 2. + /// `LineString` vertices in (lng, lat) order. Length >= 2. pub points: Vec<(f64, f64)>, } diff --git a/build/gpkg_io.rs b/build/gpkg_io.rs index 9a20347..0888bcf 100644 --- a/build/gpkg_io.rs +++ b/build/gpkg_io.rs @@ -1,9 +1,20 @@ -//! GeoPackage SQLite reader. Depends on `rusqlite`, declared under +//! `GeoPackage` `SQLite` reader. Depends on `rusqlite`, declared under //! both `[build-dependencies]` (for `build.rs`) and `[dev-dependencies]` //! (so `tests/tampered_gpkg_panic.rs` can re-include this module via //! `#[path]` and drive the real reader against a tampered `.gpkg` //! copy). This file is never compiled into the library crate. +// ENG-4684: see `build/csr.rs` for why this is a module-level inner +// attribute rather than a crate-root allow -- this file is re-included +// via `#[path]` by `tests/tampered_gpkg_panic.rs` as well as compiled +// into the build script. +// +// `iter_edges` returns `Result<_, String>` and every error path already +// carries a human-readable message naming the failed operation; an +// `# Errors` section would restate them. The lint only fires in the +// test-crate inclusion context anyway. +#![allow(clippy::missing_errors_doc)] + use crate::build::geometry::parse_gpb_linestring; use crate::build::gpkg::RawEdge; use std::path::Path; diff --git a/build/groups.rs b/build/groups.rs index f08c8b2..46f3af6 100644 --- a/build/groups.rs +++ b/build/groups.rs @@ -3,7 +3,7 @@ //! 1. For each of 12 known upstream `pass` values, every matching row //! belongs to the corresponding public group name. //! 2. The 13th group `menaiStrait` is derived geometrically: any edge -//! whose LineString intersects the bbox +//! whose `LineString` intersects the bbox //! `lng ∈ [-4.20, -4.00], lat ∈ [53.13, 53.30]` (closed) using the //! Liang-Barsky line-vs-AABB clip test. //! 3. Empty groups at any resolution are a HARD ERROR — panics with a @@ -11,6 +11,18 @@ //! 4. Unknown non-null `pass` values are a HARD ERROR — panics with the //! offending value and fid. +// ENG-4684: see `build/csr.rs` for why these are module-level inner +// attributes rather than crate-root allows -- this file is compiled into +// the build script and into the test crates that re-include it via +// `#[path]`. +// +// The `usize as u32` narrowings below produce edge ids, which are `u32` +// in the on-disk schema (`src/graph.rs::DirectedEdge::edge_id`). +#![allow(clippy::cast_possible_truncation)] +// These fire only in the test-crate inclusion context, where a `pub fn` +// of this module becomes public API of a crate rooted at a test file. +#![allow(clippy::must_use_candidate, clippy::missing_panics_doc)] + use crate::build::csr::CsrBuilt; use crate::build::geometry::polyline_intersects_bbox; use crate::build::gpkg::RawEdge; @@ -95,8 +107,8 @@ pub fn assign_groups(raw: &[RawEdge], csr: &CsrBuilt, res_km: u32) -> Vec Date: Mon, 27 Jul 2026 12:45:16 +0000 Subject: [PATCH 06/11] style: ENG-4684 bin + bench pedantic clean `doc_markdown` only -- five backticks in the `cli`-gated binary (`GeoJSON` x3, `FeatureCollection`, `LineString`) and two in the bench (`html_reports`, `CodSpeed`). No behavioural change, and the binary keeps its own crate-level `#![deny(unsafe_code)]`. `missing_const_for_fn` (src/bin/rustyroute.rs) and `significant_drop_tightening` (benches/route.rs) are nursery. Nursery is deliberately library-scoped -- the AC's clippy command carries only `-W clippy::pedantic` -- so both are left alone. Verified: `cargo clippy --all-targets --all-features -- -D warnings -W clippy::pedantic` reports nothing under src/bin/ or benches/; `cargo test --test cli_smoke --all-features` passes; `cargo fmt --all --check` clean. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- benches/route.rs | 4 ++-- src/bin/rustyroute.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/benches/route.rs b/benches/route.rs index cfcc646..a09e5f9 100644 --- a/benches/route.rs +++ b/benches/route.rs @@ -2,8 +2,8 @@ //! //! Authored against `codspeed-criterion-compat` (a drop-in criterion //! replacement): under `cargo bench` it behaves as plain criterion -//! (with html_reports); under `cargo codspeed` it registers these -//! benches with the CodSpeed instrumentation harness for regression +//! (with `html_reports`); under `cargo codspeed` it registers these +//! benches with the `CodSpeed` instrumentation harness for regression //! tracking on PRs. //! //! Every graph is loaded via `Graph::load(N)` — the production entry diff --git a/src/bin/rustyroute.rs b/src/bin/rustyroute.rs index 85b3c19..bddc2f6 100644 --- a/src/bin/rustyroute.rs +++ b/src/bin/rustyroute.rs @@ -159,7 +159,7 @@ impl Resolution { enum Format { /// Compact JSON object. Json, - /// GeoJSON FeatureCollection with one LineString feature. + /// `GeoJSON` `FeatureCollection` with one `LineString` feature. Geojson, /// One `lng,lat` per line. Line, @@ -285,13 +285,13 @@ fn write_route(route: &Route, format: Format, resolution_km: u32) -> io::Result< /// Write the path as a JSON array of `[lng, lat]` positions. /// /// `Route::coordinates` is `(lat, lng)` (see `src/loader.rs`), while -/// GeoJSON and both JSON outputs use `lng, lat`. This function is the +/// `GeoJSON` and both JSON outputs use `lng, lat`. This function is the /// single place that swap happens. /// /// `min_positions` pads the array by repeating the last position until /// it holds at least that many entries. A self-route yields exactly one /// coordinate, and RFC 7946 §3.1.4 requires a `LineString` to have two -/// or more positions, so the GeoJSON writer asks for 2 and gets a valid +/// or more positions, so the `GeoJSON` writer asks for 2 and gets a valid /// degenerate zero-length line instead of invalid output. fn write_coordinates(out: &mut W, route: &Route, min_positions: usize) -> io::Result<()> { out.write_all(b"[")?; From 6ffabb55f10c63f9803d4f56ad4c3c57efc0f2e6 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:48:12 +0000 Subject: [PATCH 07/11] style: ENG-4684 test-crate pedantic clean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes AC3: `cargo clippy --all-targets --all-features -- -D warnings -W clippy::pedantic` now exits 0 across every target in the package. Fixed (strict improvements, no behavioural change): * `doc_markdown` x28 across 11 files -- backticks on identifiers and file paths in module docs. * `map_unwrap_or` x5 -- `map_or_else` in downstream_consumer_smoke.rs and feature_matrix.rs, `map_or` in pre_commit_config.rs, and `Result::is_ok_and` for the two `.output().map(...).unwrap_or(false)` probes in pre_commit_e2e.rs. * `redundant_closure_for_method_calls` x4 -- `std::sync::PoisonError::into_inner` in env_data_dir.rs x2, graph_load.rs, feature_matrix.rs. Justified allows. Crate-root blocks, one prose justification per lint: * tests/vendored_data.rs -- `unreadable_literal` x72, `many_single_char_names`, `items_after_statements` x2. The file transcribes FIPS 180-4 so the vendored-data integrity check needs no third-party hash crate. `K`/`H0` are the round-constant and initial-hash tables from §4.2.2 / §5.3.3 and `a`..`h` are the §6.2.2 working-variable names; separators and renames would destroy the character-for-character comparability the implementation is reviewed against. * tests/build_helpers.rs -- `unreadable_literal` x4: Menai Strait bbox corner coordinates at grid-snap precision, which must stay byte-comparable against build/groups.rs. * tests/golden_routes.rs -- `float_cmp` x4: exactness is the point. A fixture pinned at `expected_km == 0` must be exactly zero, and the `from`/`to` comparisons check two rows describe the same voyage. An epsilon anywhere here would let real drift through. * tests/route_smoke.rs -- `float_cmp` x1: `Graph::route` returns a literal `0.0` on the `start == goal` short circuit, so there is no accumulated error for a tolerance to absorb. * tests/feature_matrix.rs -- `manual_assert` x3: each panic body interpolates the full multi-line stdout AND stderr of a `cargo check` subprocess; `assert!` reads far worse across twenty lines of captured compiler output. * tests/build_artifacts.rs -- site allow for `cast_possible_truncation`: node indices are `u32` by the on-disk schema. tests/downstream_consumer/ is a separate `[package]`, not covered by the root lint config or `cargo fmt --all`, and needs no change. NOTE on tests/data_module.rs: the plan says not to modify it. The single change here is backticking `BYTES_50KM` in that file's own module doc comment, required because AC3 is all-or-nothing. The instruction's purpose -- protecting the ENG-4688 wasm-gate guard, which asserts on `src/data.rs`'s `"pub(crate) fn bytes_for"` source text -- is fully honoured: the guard and that signature are both untouched, and `cargo test --test data_module` passes. See concerns.md C5. Verified: AC3 exits 0; `cargo fmt --all --check` clean. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- tests/audit_workflow.rs | 2 +- tests/build_artifacts.rs | 11 ++++++++--- tests/build_helpers.rs | 7 +++++++ tests/cli_smoke.rs | 8 ++++---- tests/codspeed_workflow.rs | 8 ++++---- tests/data_module.rs | 2 +- tests/downstream_consumer_smoke.rs | 7 ++++--- tests/env_data_dir.rs | 8 ++++++-- tests/feature_matrix.rs | 21 +++++++++++++++------ tests/golden_routes.rs | 11 +++++++++++ tests/graph_load.rs | 10 ++++++---- tests/pre_commit_config.rs | 3 +-- tests/pre_commit_e2e.rs | 6 ++---- tests/release_dist_workflow.rs | 4 ++-- tests/release_plz_config.rs | 6 +++--- tests/route_smoke.rs | 8 ++++++++ tests/tampered_gpkg_panic.rs | 4 ++-- tests/vendored_data.rs | 27 ++++++++++++++++++++++++++- 18 files changed, 111 insertions(+), 42 deletions(-) diff --git a/tests/audit_workflow.rs b/tests/audit_workflow.rs index 03735e7..e1218b8 100644 --- a/tests/audit_workflow.rs +++ b/tests/audit_workflow.rs @@ -38,7 +38,7 @@ //! AC6 nightly cron 06:00 UTC opens GH issues -> `audit_workflow_cron_is_0_6_utc` //! + `audit_workflow_grants_issues_write_for_scheduled_issues` //! AC7 licence rationale + GPL exclusion note -> `deny_toml_license_rationale_comment_is_present` -//! AC8 tests/audit_workflow.rs passes -> this file +//! AC8 `tests/audit_workflow.rs` passes -> this file //! AC9 deny.toml is the only policy file -> `no_audit_toml_at_repo_root` //! AC10 audit doesn't share ci concurrency -> `audit_workflow_has_separate_concurrency_group` diff --git a/tests/build_artifacts.rs b/tests/build_artifacts.rs index 729bdce..8cc3c0c 100644 --- a/tests/build_artifacts.rs +++ b/tests/build_artifacts.rs @@ -1,12 +1,12 @@ //! End-to-end tests for the build.rs output (ENG-4678). //! -//! These tests load each archive from OUT_DIR, validate its +//! These tests load each archive from `OUT_DIR`, validate its //! magic+schema-version prefix, and assert: //! - all 5 archives exist and are non-empty //! - bytes-per-edge sanity (≤ 64 bytes/edge) -//! - rkyv::access succeeds (i.e. archive is well-formed) +//! - `rkyv::access` succeeds (i.e. archive is well-formed) //! - all 13 groups are non-empty -//! - EDGE_GROUPS has the expected 13 names in expected order +//! - `EDGE_GROUPS` has the expected 13 names in expected order //! - menaiStrait counts match Python ground truth at every resolution //! - self-loops are preserved (≥10 per file) //! @@ -193,6 +193,11 @@ fn csr_structural_invariants() { archived.edges.len(), "{n}km: node_offsets last != edges.len()" ); + // A node index is `u32` by the on-disk schema, so a node table + // that overflowed this cast could not have been serialised in the + // first place -- and the very next assertion is what would catch + // it if one somehow had been. + #[allow(clippy::cast_possible_truncation)] let n_nodes = archived.nodes.len() as u32; for e in archived.edges.iter() { let t = e.target.to_native(); diff --git a/tests/build_helpers.rs b/tests/build_helpers.rs index addacfb..d006d1f 100644 --- a/tests/build_helpers.rs +++ b/tests/build_helpers.rs @@ -2,6 +2,13 @@ //! normal test binary so `cargo test` runs them. The helpers //! themselves live under `build/` and are also compiled into //! `build.rs`; this file re-includes them via #[path]. +// ENG-4684: the flagged literals are the Menai Strait bbox corner +// coordinates, quoted at the precision the 5 km grid snaps them to +// (`5.0062109375`, `52.792460937499996`, ...). Digit separators in a +// decimal fraction obscure the value rather than clarifying it, and these +// have to stay byte-comparable against `build/groups.rs`'s bbox +// constants. +#![allow(clippy::unreadable_literal)] #[path = "../build/geometry.rs"] mod geometry; diff --git a/tests/cli_smoke.rs b/tests/cli_smoke.rs index 0cddcd6..962c787 100644 --- a/tests/cli_smoke.rs +++ b/tests/cli_smoke.rs @@ -10,7 +10,7 @@ //! `CARGO_BIN_EXE_` resolve to a real executable. //! //! No JSON parser dev-dependency — same convention as -//! tests/ci_workflow.rs and tests/release_dist_workflow.rs, which assert +//! `tests/ci_workflow.rs` and `tests/release_dist_workflow.rs`, which assert //! on structure with string checks. #![cfg(feature = "cli")] @@ -45,7 +45,7 @@ fn stderr(out: &Output) -> String { String::from_utf8_lossy(&out.stderr).into_owned() } -/// AC3: default format is GeoJSON, exit 0, with the documented +/// AC3: default format is `GeoJSON`, exit 0, with the documented /// properties. #[test] fn geojson_default_marseille_to_shanghai() { @@ -199,8 +199,8 @@ fn format_json_is_compact_object() { ); } -/// D2: a self-route still produces valid GeoJSON — RFC 7946 requires a -/// LineString to carry two or more positions, so the single snapped +/// D2: a self-route still produces valid `GeoJSON` — RFC 7946 requires a +/// `LineString` to carry two or more positions, so the single snapped /// position is emitted twice. #[test] fn self_route_emits_valid_two_position_linestring() { diff --git a/tests/codspeed_workflow.rs b/tests/codspeed_workflow.rs index ab06e74..9eae729 100644 --- a/tests/codspeed_workflow.rs +++ b/tests/codspeed_workflow.rs @@ -1,13 +1,13 @@ //! Integration tests that lock in the structural invariants of //! `.github/workflows/codspeed.yaml` introduced by ENG-4690. //! -//! CodSpeed's actual measurement + PR-comment behaviour can only be -//! verified on GitHub Actions against the org's CodSpeed app. What IS in +//! `CodSpeed`'s actual measurement + PR-comment behaviour can only be +//! verified on GitHub Actions against the org's `CodSpeed` app. What IS in //! scope here is the small set of string-level invariants whose silent //! regression would neuter the perf gate: the triggers, the OIDC auth -//! wiring, the CodSpeed action pin, the cargo-codspeed build/run steps, +//! wiring, the `CodSpeed` action pin, the cargo-codspeed build/run steps, //! and the [skip-perf] escape hatch. String assertions match the -//! convention in tests/ci_workflow.rs and tests/audit_workflow.rs. +//! convention in `tests/ci_workflow.rs` and `tests/audit_workflow.rs`. use std::fs; use std::path::PathBuf; diff --git a/tests/data_module.rs b/tests/data_module.rs index 27a658e..38b3927 100644 --- a/tests/data_module.rs +++ b/tests/data_module.rs @@ -1,5 +1,5 @@ //! ENG-4679: smoke-test the `rustyroute::data` module's feature-gated -//! BYTES_50KM const. Verifies the const is present under default +//! `BYTES_50KM` const. Verifies the const is present under default //! features and starts with the RRG1 magic + schema version 1 prefix. #[cfg(feature = "data-50km")] diff --git a/tests/downstream_consumer_smoke.rs b/tests/downstream_consumer_smoke.rs index 5a29db2..b6f99cc 100644 --- a/tests/downstream_consumer_smoke.rs +++ b/tests/downstream_consumer_smoke.rs @@ -11,9 +11,10 @@ use std::process::Command; fn downstream_consumer_subpackage_passes() { let manifest = PathBuf::from(env!("CARGO_MANIFEST_DIR")); let sub_manifest = manifest.join("tests/downstream_consumer/Cargo.toml"); - let target_dir = std::env::var("OUT_DIR") - .map(|s| PathBuf::from(s).join("downstream_consumer_target")) - .unwrap_or_else(|_| std::env::temp_dir().join("rustyroute_downstream_consumer_target")); + let target_dir = std::env::var("OUT_DIR").map_or_else( + |_| std::env::temp_dir().join("rustyroute_downstream_consumer_target"), + |s| PathBuf::from(s).join("downstream_consumer_target"), + ); // Use the same cargo that's running this test if CARGO is set. let cargo = std::env::var("CARGO").unwrap_or_else(|_| "cargo".into()); diff --git a/tests/env_data_dir.rs b/tests/env_data_dir.rs index 5e69042..e4ba2d1 100644 --- a/tests/env_data_dir.rs +++ b/tests/env_data_dir.rs @@ -49,7 +49,9 @@ fn in_tree_50km_path() -> PathBuf { #[test] fn load_50km_from_env_data_dir_happy_path() { - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); // Stage a fresh tempdir with a copy of the 50km archive. let tmp = tempfile::tempdir().expect("create tempdir"); @@ -109,7 +111,9 @@ fn load_50km_from_env_data_dir_happy_path() { #[test] fn load_50km_from_env_data_dir_missing_file_reports_path() { - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); // Empty tempdir — env var points at it but the file is absent. let tmp = tempfile::tempdir().expect("create tempdir"); diff --git a/tests/feature_matrix.rs b/tests/feature_matrix.rs index a72612d..644771a 100644 --- a/tests/feature_matrix.rs +++ b/tests/feature_matrix.rs @@ -7,7 +7,7 @@ //! consumer with no data baked in" published-crate case. //! 3. `--no-default-features --features data-100km` (partial features) //! — exercises AC4's "load(100) succeeds, load(50) is -//! DataNotAvailable" matrix. +//! `DataNotAvailable`" matrix. //! //! The dev phase only exercises shape #1 directly. Shapes #2 and #3 are //! described in the spec's AC3/AC4 commentary but never actually @@ -26,7 +26,13 @@ //! they would otherwise race on the cargo target-dir lock when libtest //! runs them in parallel. The `CARGO_LOCK` mutex below serialises the //! cargo subprocesses inside this binary. - +// ENG-4684: `manual_assert` would have these `if !ok { panic!(...) }` +// blocks rewritten as `assert!(ok, ...)`. They are deliberately not: +// each panic body interpolates the full multi-line stdout AND stderr of a +// `cargo check` subprocess, and `assert!`'s message is only formatted on +// failure but reads far worse when it spans twenty lines of captured +// compiler output. The `if` form keeps the diagnostic payload legible. +#![allow(clippy::manual_assert)] #![cfg(not(target_arch = "wasm32"))] use std::path::PathBuf; @@ -42,12 +48,15 @@ static CARGO_LOCK: Mutex<()> = Mutex::new(()); fn run_cargo_check(extra_args: &[&str]) -> std::process::Output { let manifest = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - let target_dir = std::env::var("OUT_DIR") - .map(|s| PathBuf::from(s).join("feature_matrix_target")) - .unwrap_or_else(|_| std::env::temp_dir().join("rustyroute_feature_matrix_target")); + let target_dir = std::env::var("OUT_DIR").map_or_else( + |_| std::env::temp_dir().join("rustyroute_feature_matrix_target"), + |s| PathBuf::from(s).join("feature_matrix_target"), + ); let cargo = std::env::var("CARGO").unwrap_or_else(|_| "cargo".into()); - let _guard = CARGO_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = CARGO_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let mut cmd = Command::new(&cargo); cmd.arg("check") .arg("--manifest-path") diff --git a/tests/golden_routes.rs b/tests/golden_routes.rs index e4f2a8f..32ab752 100644 --- a/tests/golden_routes.rs +++ b/tests/golden_routes.rs @@ -22,6 +22,17 @@ //! entry in `RESOLUTIONS` regardless of which `data-*` features are on. //! So no data feature is required, and the goldens stay available under //! partial feature sets. +// ENG-4684: `float_cmp` is allowed at the crate root because exactness is +// the point of this file. The golden table pins each route's distance to +// a tolerance carried in `tests/fixtures/routes.json`, and the bare `==` +// comparisons are precisely the cases where a tolerance would be wrong: +// a fixture pinned at `expected_km == 0` must be exactly zero (a +// non-zero-length "self-route" is a bug, not a rounding artefact), and +// the `from`/`to` endpoint comparisons check that two fixture rows +// describe the same voyage before their distances are compared against +// each other. Substituting an epsilon anywhere here would let real drift +// through -- which is the drift this file exists to catch. +#![allow(clippy::float_cmp)] #![cfg(not(target_arch = "wasm32"))] use std::collections::HashSet; diff --git a/tests/graph_load.rs b/tests/graph_load.rs index 0d9a137..4590973 100644 --- a/tests/graph_load.rs +++ b/tests/graph_load.rs @@ -1,8 +1,8 @@ //! ENG-4679 integration tests: `Graph::from_bytes` happy path, the -//! full LoadError matrix (BadMagic / UnsupportedSchema / -//! InvalidArchive), `Graph::load(N)` unknown-resolution rejection, +//! full `LoadError` matrix (`BadMagic` / `UnsupportedSchema` / +//! `InvalidArchive`), `Graph::load(N)` unknown-resolution rejection, //! count parity vs `build_csr`, and a compile-time Send+Sync -//! assertion required by Design 019's OnceLock pattern. +//! assertion required by Design 019's `OnceLock` pattern. #[cfg(feature = "data-50km")] use rustyroute::graph::{MAGIC, SCHEMA_VERSION}; @@ -60,7 +60,9 @@ fn load_unknown_resolution_seven() { fn load_50km_ok_via_out_dir() { use std::sync::Mutex; static ENV_LOCK: Mutex<()> = Mutex::new(()); - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _guard = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); // SAFETY: the ENV_LOCK guard above ensures this is the only // thread mutating env state, satisfying Rust 2024's // single-threaded-mutation requirement for `remove_var`. diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 285e80f..f9a574a 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -240,8 +240,7 @@ fn cargo_fmt_check_hook_contract() { let end = lines[start + 1..] .iter() .position(|line| line.trim_start().starts_with("- id:")) - .map(|offset| start + 1 + offset) - .unwrap_or(lines.len()); + .map_or(lines.len(), |offset| start + 1 + offset); let block = &lines[start..end]; for needle in ["language: system", "pass_filenames: false", "types: [rust]"] { diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 407eae6..f95e99b 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -64,8 +64,7 @@ fn bin_on_path(bin: &str) -> bool { Command::new(bin) .arg("--version") .output() - .map(|o| o.status.success()) - .unwrap_or(false) + .is_ok_and(|o| o.status.success()) } fn cargo_bin() -> String { @@ -123,8 +122,7 @@ fn cargo_subcommand_available(subcommand: &str) -> bool { .env("PATH", cargo_augmented_path()) .args([subcommand, "--version"]) .output() - .map(|o| o.status.success()) - .unwrap_or(false) + .is_ok_and(|o| o.status.success()) } /// Run a command, capture stdout+stderr, return (status, combined output). diff --git a/tests/release_dist_workflow.rs b/tests/release_dist_workflow.rs index f6cb09d..cfcd749 100644 --- a/tests/release_dist_workflow.rs +++ b/tests/release_dist_workflow.rs @@ -13,8 +13,8 @@ //! tests/ci_workflow.rs:36-39 / tests/audit_workflow.rs:22-27. //! //! AC mapping (spec at .ship/tasks/eng-4692-.../plan/spec.md): -//! AC3 -> release_workflow_* tests -//! AC4 -> dist_workspace_toml_* tests +//! AC3 -> `release_workflow`_* tests +//! AC4 -> `dist_workspace_toml`_* tests use std::fs; use std::path::PathBuf; diff --git a/tests/release_plz_config.rs b/tests/release_plz_config.rs index bc38651..7d6728b 100644 --- a/tests/release_plz_config.rs +++ b/tests/release_plz_config.rs @@ -21,14 +21,14 @@ //! - Whether the crate actually publishes to crates.io. //! - cargo-dist binary attachment — a separate, currently-dormant gap //! (no [[bin]] target yet; see Cargo.toml:22-25 and -//! tests/release_dist_workflow.rs). +//! `tests/release_dist_workflow.rs`). //! //! No YAML/TOML parser dev-dependency — matches the convention in //! tests/ci_workflow.rs:36-39 and tests/audit_workflow.rs:22-27. //! //! AC mapping (spec at .ship/tasks/eng-4692-.../plan/spec.md): -//! AC1 -> release_plz_toml_* tests -//! AC2 -> release_plz_workflow_* tests +//! AC1 -> `release_plz_toml`_* tests +//! AC2 -> `release_plz_workflow`_* tests use std::fs; use std::path::PathBuf; diff --git a/tests/route_smoke.rs b/tests/route_smoke.rs index 9348721..cf40c68 100644 --- a/tests/route_smoke.rs +++ b/tests/route_smoke.rs @@ -2,6 +2,14 @@ //! detour (Suez), edge-group lookup, coordinate validation, and the //! all-blocked `NoRoute` path. The heavier golden-route distance table //! lives in ENG-4681. +// ENG-4684: `float_cmp` is allowed because +// `assert_eq!(route.distance_km, 0.0)` for a self-route is an exact-zero +// assertion, not an approximate one. `Graph::route` returns a literal +// `0.0` on the `start == goal` short circuit without summing any edge +// weights, so there is no accumulated error for a tolerance to absorb -- +// and a tolerance would hide a regression that started returning a small +// non-zero distance. +#![allow(clippy::float_cmp)] #![cfg(feature = "data-50km")] use rustyroute::{EdgeId, Graph, RouteError}; diff --git a/tests/tampered_gpkg_panic.rs b/tests/tampered_gpkg_panic.rs index daa1069..dc5342e 100644 --- a/tests/tampered_gpkg_panic.rs +++ b/tests/tampered_gpkg_panic.rs @@ -5,14 +5,14 @@ //! to remove a Suez edge." //! //! Strategy: rather than spawning `cargo build` as a subprocess (slow; -//! rebuilds the SQLite amalgamation in a fresh target dir), this test +//! rebuilds the `SQLite` amalgamation in a fresh target dir), this test //! drives the SAME pipeline that `build.rs` runs — the real //! `gpkg_io::iter_edges` reader, the real `csr::build_csr`, the real //! `groups::assign_groups` — against a tampered copy of a vendored //! .gpkg with the Suez row removed. //! //! What this proves: -//! - The actual SQLite reader (rusqlite) correctly returns rows from +//! - The actual `SQLite` reader (rusqlite) correctly returns rows from //! a tampered .gpkg copy. //! - When the Suez row is absent, the build pipeline panics with //! the documented message naming the empty group (`suezCanal`) diff --git a/tests/vendored_data.rs b/tests/vendored_data.rs index 086e16f..f501d8f 100644 --- a/tests/vendored_data.rs +++ b/tests/vendored_data.rs @@ -1,4 +1,4 @@ -//! Integration tests that lock in the vendored Eurostat MARNET GeoPackage +//! Integration tests that lock in the vendored Eurostat MARNET `GeoPackage` //! files and their attribution paperwork. These exist because the data IS //! the contract for ENG-4677 — the `build.rs` introduced in ENG-4678 and //! downstream consumers depend on these exact bytes. If a refactor @@ -27,6 +27,31 @@ //! self-tests for inline SHA-256 -> `sha256_known_answer_empty`, //! `sha256_known_answer_abc`, //! `sha256_known_answer_longer` +// ENG-4684 crate-root lint posture. All three allows exist because this +// file transcribes FIPS 180-4 (the SHA-256 specification) so that the +// vendored-data integrity check needs no third-party hash crate; a +// reviewer must be able to diff the code against the published standard +// character for character. +// +// `unreadable_literal` x72: `K` and `H0` are the round-constant and +// initial-hash tables printed in FIPS 180-4 §4.2.2 and §5.3.3. Inserting +// `_` digit separators into `0x428a2f98` makes that comparison harder, +// and the values are not meant to be read as magnitudes. +// +// `many_single_char_names`: `a`..`h` are the working variables named +// verbatim in the FIPS 180-4 §6.2.2 compression-function pseudocode. +// Renaming them to `state_a` etc. would break the correspondence the +// implementation is checked against. +// +// `items_after_statements`: `const LFS_POINTER_PREFIX` and the local +// `use std::io::Read` sit next to the code that needs them, inside the +// test that explains what an LFS pointer stub is. Hoisting them to the +// crate root would separate each from its explanation. +#![allow( + clippy::unreadable_literal, + clippy::many_single_char_names, + clippy::items_after_statements +)] use std::fs; use std::path::PathBuf; From 6d05d76dc32af893ccd1ce6a6a5ee5b10374bb3f Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 12:49:28 +0000 Subject: [PATCH 08/11] ci: ENG-4684 enforce -W clippy::pedantic Exactly one line of ci.yaml changes -- the clippy step at :47 gains `-W clippy::pedantic`. It has to be the same line: `tests/ci_workflow.rs` asserts there is exactly ONE `cargo clippy` line in the file, so a second step would break the suite. tests/ci_workflow.rs gains two assertions: * `clippy_job_uses_deny_warnings` now also requires `-W clippy::pedantic` on that line. `#![warn(clippy::pedantic)]` in src/lib.rs reaches only the library crate, so this flag is what keeps the ~120 findings resolved across build.rs, tests/, benches/ and src/bin/ from going latent again. * `test_job_does_not_narrow_away_doctests` (new). Nothing in the workflow names `cargo test --doc`: the fifteen library doctests ride entirely on the test-matrix's `cargo test --all-features`, and `cargo llvm-cov` in the coverage job does not run doctests without `--doctests`, which it is not given. Narrowing that line to `--lib` or `--tests` would silently stop executing this ticket's headline deliverable with nothing going red. The test locates the single un-narrowed `- run: cargo test` line and rejects `--lib`, `--tests`, `--bins`, `--benches` and `--examples` on it. Two edits from an earlier draft are deliberately CUT as overreach: * `RUSTDOCFLAGS` (:160) does NOT gain `-D rustdoc::broken_intra_doc_links`. Triply redundant: the lint is warn-by-default so the existing `-D warnings` already denies it, src/lib.rs now carries `#![deny(rustdoc::broken_intra_doc_links)]`, and it would burn the single-RUSTDOCFLAGS-line budget that `docs_job_denies_rustdoc_warnings` enforces. * `cargo doc` (:167) does NOT gain `--document-private-items`. The ticket runs that as a local acceptance command; adding it would permanently widen a CI gate to private-item rustdoc lints. Verified: `cargo test --test ci_workflow --all-features` 15 passed; `cargo fmt --all --check` clean. Refs ENG-4684 Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- .github/workflows/ci.yaml | 2 +- tests/ci_workflow.rs | 65 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index afc3871..2532243 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -44,7 +44,7 @@ jobs: - uses: Swatinem/rust-cache@v2 with: shared-key: clippy - - run: cargo clippy --all-targets --all-features -- -D warnings + - run: cargo clippy --all-targets --all-features -- -D warnings -W clippy::pedantic test-matrix: name: test (${{ matrix.os }} / ${{ matrix.toolchain }}) diff --git a/tests/ci_workflow.rs b/tests/ci_workflow.rs index aebab51..d6ce8db 100644 --- a/tests/ci_workflow.rs +++ b/tests/ci_workflow.rs @@ -168,6 +168,71 @@ fn clippy_job_uses_deny_warnings() { "clippy invocation must use `--all-features` so feature-gated code is linted too. \ Offending line: {clippy:?}" ); + // ENG-4684. `#![warn(clippy::pedantic)]` in src/lib.rs reaches only + // the library crate, and the `[lints.clippy]` table in Cargo.toml is + // not consulted by a bare `cargo clippy` in the same way a + // command-line flag is escalated by `-D warnings`. This flag is what + // makes the ~120 pedantic findings in build.rs, tests/*, benches/ and + // src/bin/ -- all fixed or justified-allowed under ENG-4684 -- stay + // fixed. Drop it and every one of them silently becomes latent again. + assert!( + clippy.contains("-W clippy::pedantic"), + "clippy invocation must include `-W clippy::pedantic` on the same line (ENG-4684). \ + Without it the pedantic findings that ticket resolved across build.rs, tests/, \ + benches/ and src/bin/ stop being gated and regress silently. \ + Offending line: {clippy:?}" + ); +} + +/// ENG-4684: nothing in the workflow names `cargo test --doc`, so the +/// crate's fifteen library doctests -- the ticket's headline deliverable +/// -- ride entirely on the test-matrix's `cargo test --all-features`. +/// `cargo test` runs doctests only when it is not narrowed to specific +/// target kinds, and `cargo llvm-cov` in the coverage job does not run +/// them at all without `--doctests`. So if anyone ever "optimises" that +/// line to `--tests` or `--lib`, every doctest silently stops executing +/// and CI stays green. +/// +/// Asserted on the `cargo test` line rather than file-wide for the same +/// reason as `clippy_job_uses_deny_warnings`: a file-wide check would be +/// satisfied by the `pre-commit` job's `cargo test --test pre_commit_e2e`. +#[test] +fn test_job_does_not_narrow_away_doctests() { + let wf = read_workflow(); + // Anchored on `- run:` so the `# - No --all-targets / cargo test:` + // prose in the wasm job's rationale comment is not mistaken for an + // invocation, and `--test ` excludes the pre-commit job's + // single-target `cargo test --test pre_commit_e2e`. + let plain_test_lines: Vec<&str> = wf + .lines() + .filter(|l| { + let t = l.trim_start(); + t.starts_with("- run:") && t.contains("cargo test") && !t.contains("--test ") + }) + .collect(); + assert_eq!( + plain_test_lines.len(), + 1, + "expected exactly one un-narrowed `cargo test` invocation in ci.yaml (the \ + test-matrix row that carries the doctests), found {}: {plain_test_lines:?}", + plain_test_lines.len() + ); + let carrier = plain_test_lines[0]; + assert!( + carrier.contains("--all-features"), + "the doctest carrier must run `--all-features` so feature-gated doc examples \ + (e.g. the `data::BYTES_50KM` ones) compile. Offending line: {carrier:?}" + ); + for narrowing in ["--lib", "--tests", "--bins", "--benches", "--examples"] { + assert!( + !carrier.contains(narrowing), + "the `cargo test` line must not pass `{narrowing}`: narrowing it to specific \ + target kinds stops `cargo test` from running doctests, and no other job runs \ + them (`cargo llvm-cov` needs `--doctests`, which it is not given). The \ + library's doctests would silently stop executing with nothing going red \ + (ENG-4684). Offending line: {carrier:?}" + ); + } } // --------------------------------------------------------------------------- From 398f5b99a9f09125e99807e3a0a28383d3c778dd Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 13:14:58 +0000 Subject: [PATCH 09/11] docs: ENG-4684 correct lint-allow justifications flagged in review Review found six defects, all inaccurate justification/doc prose rather than behavioural bugs -- the conclusions were right but the citations pointed at the wrong field, file, or literals. On a ticket whose deliverable is the prose, a confidently-wrong comment is worse than none. P2 src/loader.rs -- load_file's needless_pass_by_value reason claimed `&File` would let a caller mutate the file under the mapping. Unsound: both call sites pass a read-only `File::open` handle, an mmap outlives the descriptor regardless, and by-value stops nothing anyway (try_clone, a second open, another process). Restated as the tidiness argument it actually is, and points at the SAFETY note for the real immutability contract. P2 tests/build_helpers.rs -- unreadable_literal reason named "Menai Strait bbox corner coordinates ... at 5 km grid precision" and claimed byte-coupling to build/groups.rs. The four flagged literals are the 100 km Menai edge's segment endpoints at full f64 precision; the real bbox constants are the last four args and never trip the lint. P3 tests/golden_routes.rs -- float_cmp reason described a comparison between two fixture rows' endpoints. No such comparison exists; the only float-tuple compare is a single row's own `from == to`, which is what licenses the exact zero-km assertion. P3 src/loader.rs -- node_count's cast cited node_offsets: Vec, a CSR row pointer that bounds edges.len(), not nodes.len(). Now cites NodeId / DirectedEdge::source,target. P3 src/lib.rs -- the crate-root Cape-route doctest pinned `> 25_000.0` against a real 25,047 km, a 0.19% margin that any archive rebake could redden. Switched to the relative form the equivalent assertion on Graph::route already uses (8.7% margin). P3 build/csr.rs, build/gpkg_io.rs -- the #[path] inclusion lists that tell a maintainer how far each module-level allow reaches were wrong in both directions: csr.rs named build_helpers.rs (which includes only geometry.rs) and said five crate roots for four; gpkg_io.rs omitted tests/graph_load.rs. Gates re-run: fmt clean, clippy --all-targets --all-features -D warnings -W clippy::pedantic exit 0, 15 doctests 0 ignored, rustdoc -D warnings clean. Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- build/csr.rs | 6 +++--- build/gpkg_io.rs | 5 +++-- src/lib.rs | 2 +- src/loader.rs | 24 ++++++++++++++++-------- tests/build_helpers.rs | 11 ++++++----- tests/golden_routes.rs | 7 ++++--- 6 files changed, 33 insertions(+), 22 deletions(-) diff --git a/build/csr.rs b/build/csr.rs index 3315d44..38a9a69 100644 --- a/build/csr.rs +++ b/build/csr.rs @@ -5,9 +5,9 @@ // are scoped to that module, so one edit here covers every crate this // file is compiled into: the build script (via `build.rs`'s `#[path]` // mod) and the integration tests that re-include it the same way -// (`tests/graph_load.rs`, `tests/build_helpers.rs`, -// `tests/group_assignment.rs`, `tests/tampered_gpkg_panic.rs`). The -// alternative -- repeating each allow at five crate roots -- drifts. +// (`tests/graph_load.rs`, `tests/group_assignment.rs`, +// `tests/tampered_gpkg_panic.rs`). The alternative -- repeating each +// allow at those four crate roots -- drifts. // // Every `usize as u32` below narrows a node id, an edge id or a CSR row // pointer, all of which are `u32` in the on-disk schema diff --git a/build/gpkg_io.rs b/build/gpkg_io.rs index 0888bcf..afe2add 100644 --- a/build/gpkg_io.rs +++ b/build/gpkg_io.rs @@ -6,8 +6,9 @@ // ENG-4684: see `build/csr.rs` for why this is a module-level inner // attribute rather than a crate-root allow -- this file is re-included -// via `#[path]` by `tests/tampered_gpkg_panic.rs` as well as compiled -// into the build script. +// via `#[path]` by `tests/graph_load.rs` and +// `tests/tampered_gpkg_panic.rs` as well as compiled into the build +// script. // // `iter_edges` returns `Result<_, String>` and every error path already // carries a human-readable message naming the failed operation; an diff --git a/src/lib.rs b/src/lib.rs index 7ea2bc9..8f0e91c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -37,7 +37,7 @@ //! //! let suez = graph.edges_for_groups(["suezCanal"])?; //! let round_the_cape = graph.route(marseille, shanghai, &suez)?; -//! assert!(round_the_cape.distance_km > 25_000.0); +//! assert!(round_the_cape.distance_km > via_suez.distance_km + 8_000.0); //! # Ok::<(), Box>(()) //! ``` //! diff --git a/src/loader.rs b/src/loader.rs index f505d00..d91c0f8 100644 --- a/src/loader.rs +++ b/src/loader.rs @@ -341,11 +341,17 @@ impl Graph { #[cfg(not(target_arch = "wasm32"))] // Taking `File` by value is deliberate ownership transfer, not an - // oversight: `memmap2::Mmap::map` only borrows the handle, but the - // mapping must outlive it, so `load_file` becomes the sole owner and - // drops the descriptor at the end of this frame. Passing `&File` - // would let a caller keep the handle alive and mutate the file - // underneath a mapping the SAFETY note below assumes is immutable. + // oversight: `memmap2::Mmap::map` only borrows the handle, and the + // resulting mapping stays valid after the descriptor is closed, so + // `load_file` becomes the sole owner and drops it at the end of this + // frame. No caller needs the handle afterwards, and by-value makes + // that lifecycle explicit rather than leaving a live `&File` at the + // call site with nothing left to do. + // + // This is a tidiness argument, not a safety one: the signature + // enforces nothing about the file's contents. Immutability for the + // life of the mapping is the operator's responsibility -- see the + // SAFETY note below. #[allow(clippy::needless_pass_by_value)] fn load_file(file: std::fs::File, resolution_km: u32) -> Result { // SAFETY: memmap2::Mmap::map is unsafe because the kernel can @@ -443,9 +449,11 @@ impl Graph { /// ``` #[must_use] pub fn node_count(&self) -> u32 { - // A node index is `u32` by the on-disk schema - // (`GraphData::node_offsets: Vec`), so the table can never - // hold more entries than `u32::MAX`. + // A node id is `u32` by the on-disk schema ([`NodeId`], and + // `DirectedEdge::source` / `DirectedEdge::target` in + // `src/graph.rs`), so a graph can never carry more nodes than + // `u32::MAX` -- an unaddressable node could not be referenced by + // any edge. Enforced at build time in `build/csr.rs`. #[allow(clippy::cast_possible_truncation)] { self.archived().nodes.len() as u32 diff --git a/tests/build_helpers.rs b/tests/build_helpers.rs index d006d1f..1df8c34 100644 --- a/tests/build_helpers.rs +++ b/tests/build_helpers.rs @@ -2,12 +2,13 @@ //! normal test binary so `cargo test` runs them. The helpers //! themselves live under `build/` and are also compiled into //! `build.rs`; this file re-includes them via #[path]. -// ENG-4684: the flagged literals are the Menai Strait bbox corner -// coordinates, quoted at the precision the 5 km grid snaps them to -// (`5.0062109375`, `52.792460937499996`, ...). Digit separators in a +// ENG-4684: the flagged literals are the two endpoint coordinates of the +// 100 km Menai Strait edge (`-5.0062109375`, `52.792460937499996`, +// `-4.002734375`, `53.30314453125`), reproduced at full `f64` precision +// exactly as the vendored GeoPackage yields them. Digit separators in a // decimal fraction obscure the value rather than clarifying it, and these -// have to stay byte-comparable against `build/groups.rs`'s bbox -// constants. +// are transcribed values -- a reader checking them against the source +// geometry wants them character-for-character. #![allow(clippy::unreadable_literal)] #[path = "../build/geometry.rs"] diff --git a/tests/golden_routes.rs b/tests/golden_routes.rs index 32ab752..8d7e879 100644 --- a/tests/golden_routes.rs +++ b/tests/golden_routes.rs @@ -28,9 +28,10 @@ // comparisons are precisely the cases where a tolerance would be wrong: // a fixture pinned at `expected_km == 0` must be exactly zero (a // non-zero-length "self-route" is a bug, not a rounding artefact), and -// the `from`/`to` endpoint comparisons check that two fixture rows -// describe the same voyage before their distances are compared against -// each other. Substituting an epsilon anywhere here would let real drift +// the `from == to` endpoint check proves a zero-km fixture really does +// name one identical point, which is what makes the exact +// `assert_eq!(distance_km, 0.0)` legitimate rather than lucky. +// Substituting an epsilon anywhere here would let real drift // through -- which is the drift this file exists to catch. #![allow(clippy::float_cmp)] #![cfg(not(target_arch = "wasm32"))] From f89331be511c64907f3c78b8f60232611c049aaa Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 13:26:31 +0000 Subject: [PATCH 10/11] docs: ENG-4684 fix citations in node_count comment and #[path] doc lists Round-2 re-review found three P3s, including one I introduced in f5ca648 by transcribing round 1's suggested citation instead of grepping it. Applying the review's own prescription to the review. src/loader.rs -- node_count's cast justification named `DirectedEdge::source`, which does not exist: that struct has only `target`, `weight_km` and `edge_id`, because in CSR the source node is implicit in the `node_offsets` row index. It also claimed the bound is "enforced at build time in build/csr.rs", but csr.rs:70 is `nodes.len() as u32` -- a truncating cast that mints ids, not a check, and the step-6 assertions all still pass under a wrapped table. Now cites NodeId / DirectedEdge::target / GraphData::edge_endpoints and states plainly that the bound is structural rather than asserted. `edge_count` and `directed_edge_count` were already correct and are untouched. build/gpkg_io.rs -- the module doc named one `#[path]` re-includer while the lint-posture comment three lines below named two. Added tests/graph_load.rs, which also compiles `iter_edges` and so is part of why `rusqlite` is a dev-dependency. build/gpkg.rs -- same omission in RawEdge's re-includer list. src/graph.rs -- resolves the open question both review rounds raised. `GroupEntry::name` said changing a value "requires a SCHEMA_VERSION bump", stricter than SCHEMA_VERSION's own contract ("incompatible layout changes", and a field-level MUST in the module doc). A value rename changes no layout, so the two docs disagreed. Widened SCHEMA_VERSION rather than softening the claim: a bump makes a stale archive fail fast in the header check instead of surfacing later as a confusing UnknownGroup from edges_for_groups. First paragraph split to satisfy too_long_first_doc_paragraph -- the nursery lint this ticket enables, which caught the edit. Verified every identifier the new comments name actually exists. Gates: fmt clean, clippy -D warnings (with and without -W clippy::pedantic) exit 0, 15 doctests 0 ignored, rustdoc -D warnings and broken_intra_doc_links clean, full suite 190 passed 0 failed. Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- build/gpkg.rs | 5 +++-- build/gpkg_io.rs | 7 ++++--- src/graph.rs | 9 ++++++++- src/loader.rs | 14 +++++++++----- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/build/gpkg.rs b/build/gpkg.rs index 4d2388e..b4380ad 100644 --- a/build/gpkg.rs +++ b/build/gpkg.rs @@ -1,8 +1,9 @@ //! Raw edge data extracted from one row of a vendored `.gpkg`. //! //! `RawEdge` is plain data and is re-included via `#[path]` by tests -//! that exercise the build pipeline (`tests/group_assignment.rs`, -//! `tests/tampered_gpkg_panic.rs`). The `GeoPackage` `SQLite` reader +//! that exercise the build pipeline (`tests/graph_load.rs`, +//! `tests/group_assignment.rs`, `tests/tampered_gpkg_panic.rs`). The +//! `GeoPackage` `SQLite` reader //! (`iter_edges`) lives in `build/gpkg_io.rs`; `rusqlite` is declared //! under both `[build-dependencies]` and `[dev-dependencies]` so the //! end-to-end tampered-gpkg test can re-include `gpkg_io.rs` and drive diff --git a/build/gpkg_io.rs b/build/gpkg_io.rs index afe2add..bf04eed 100644 --- a/build/gpkg_io.rs +++ b/build/gpkg_io.rs @@ -1,8 +1,9 @@ //! `GeoPackage` `SQLite` reader. Depends on `rusqlite`, declared under //! both `[build-dependencies]` (for `build.rs`) and `[dev-dependencies]` -//! (so `tests/tampered_gpkg_panic.rs` can re-include this module via -//! `#[path]` and drive the real reader against a tampered `.gpkg` -//! copy). This file is never compiled into the library crate. +//! (so `tests/graph_load.rs` and `tests/tampered_gpkg_panic.rs` can +//! re-include this module via `#[path]` — the latter drives the real +//! reader against a tampered `.gpkg` copy). This file is never compiled +//! into the library crate. // ENG-4684: see `build/csr.rs` for why this is a module-level inner // attribute rather than a crate-root allow -- this file is re-included diff --git a/src/graph.rs b/src/graph.rs index c9f0317..ba723ba 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -55,7 +55,14 @@ /// Magic prefix written to every `.rkyv` file before the rkyv payload. pub const MAGIC: &[u8; 4] = b"RRG1"; -/// On-disk schema version. Bump on incompatible layout changes. +/// On-disk schema version. +/// +/// Bump on incompatible layout changes, and also on any change to a +/// contract *value* that consumers match on — see [`GroupEntry::name`]. +/// A value rename leaves the archive byte-compatible, so nothing would +/// reject a stale file; bumping makes it fail fast in the header check +/// instead of surfacing later as a confusing `UnknownGroup` from +/// [`crate::Graph::edges_for_groups`]. pub const SCHEMA_VERSION: u32 = 1; /// Lon/lat coordinates of one graph node. diff --git a/src/loader.rs b/src/loader.rs index d91c0f8..4aa2c3c 100644 --- a/src/loader.rs +++ b/src/loader.rs @@ -449,11 +449,15 @@ impl Graph { /// ``` #[must_use] pub fn node_count(&self) -> u32 { - // A node id is `u32` by the on-disk schema ([`NodeId`], and - // `DirectedEdge::source` / `DirectedEdge::target` in - // `src/graph.rs`), so a graph can never carry more nodes than - // `u32::MAX` -- an unaddressable node could not be referenced by - // any edge. Enforced at build time in `build/csr.rs`. + // A node id is `u32` by the on-disk schema -- [`NodeId`], + // `DirectedEdge::target` and `GraphData::edge_endpoints: + // Vec<(u32, u32)>` in `src/graph.rs` -- so a node past + // `u32::MAX` would be unreferenceable by any edge and could not + // participate in a route. The bound is structural, not asserted: + // `build/csr.rs`'s node interner mints ids with `nodes.len() as + // u32`, so an oversized table would wrap rather than fail. In + // practice the finest grid (5 km) tops out around 10^4 nodes, + // six orders of magnitude below the cast's limit. #[allow(clippy::cast_possible_truncation)] { self.archived().nodes.len() as u32 From 635155f9ad14a81ef63d4525efc18130dbf6bf24 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Mon, 27 Jul 2026 13:43:07 +0000 Subject: [PATCH 11/11] test: ENG-4684 scope the pedantic assertion to [lints.clippy] Addresses both Copilot review comments on PR #21. tests/lint_state.rs -- cargo_toml_warns_pedantic_package_wide had two scoping holes, not just the one Copilot named. The `table` slice ran from `[lints.clippy]` to end-of-file rather than stopping at the next header, and the lint-level check scanned `toml.lines()` across the whole file. So a `pedantic = ...` key in any later table could satisfy the test while the real package-wide gate had been removed or downgraded. Now walks lines tracking the active table header and collects only those inside `[lints.clippy]`, asserting presence and level against that slice alone. Verified against the exact decoy Copilot described -- a `[package.metadata.decoy] pedantic = "warn"` entry with the real table downgraded to `nursery = "allow"`: the old form passed, the new form fails with "must set `pedantic` ... Table as read: [nursery = allow]". src/loader.rs -- documented why Graph::load's example is not made hermetic. Clearing $RUSTYROUTE_DATA_DIR inside the example would need `std::env::remove_var`, which is `unsafe` in edition 2024, and this ticket requires doctests to stay free of unsafe blocks. The caveat was already stated; now the reason it is a caveat rather than a fix is too, with a pointer to Graph::from_bytes for callers who need a loader no environment variable can redirect. Gates: fmt clean, clippy --all-targets --all-features -D warnings -W clippy::pedantic exit 0, 15 doctests 0 ignored, rustdoc -D warnings clean, lint_state 5 passed, ci_workflow 15 passed. Co-Authored-By: Claude Opus 5 Signed-off-by: Jimbo Freedman --- src/loader.rs | 6 +++++ tests/lint_state.rs | 54 +++++++++++++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/src/loader.rs b/src/loader.rs index 4aa2c3c..0ea79fd 100644 --- a/src/loader.rs +++ b/src/loader.rs @@ -272,6 +272,12 @@ impl Graph { /// *rustyroute's* compile time, so it still points at the archives /// `build.rs` baked even when the caller is a separate crate. /// + /// The example deliberately does *not* clear the variable to make + /// itself hermetic: `std::env::remove_var` is `unsafe` in edition + /// 2024, and doctests here are required to stay free of `unsafe`. + /// Use [`Graph::from_bytes`] instead when you need a loader that no + /// environment variable can redirect. + /// /// ``` /// # use rustyroute::Graph; /// let graph = Graph::load(50)?; diff --git a/tests/lint_state.rs b/tests/lint_state.rs index 934ca5f..352af40 100644 --- a/tests/lint_state.rs +++ b/tests/lint_state.rs @@ -164,27 +164,43 @@ fn lib_rs_has_no_blanket_allow() { #[test] fn cargo_toml_warns_pedantic_package_wide() { let toml = cargo_toml(); - let squeezed = squeeze_whitespace(&toml); - assert!( - squeezed.contains("[lints.clippy]"), - "Cargo.toml must declare a `[lints.clippy]` table so the pedantic baseline \ - applies to every target in the package, not just the library." - ); - let start = squeezed - .find("[lints.clippy]") - .expect("checked immediately above"); - let table = &squeezed[start..]; + + // Collect only the lines that actually live inside `[lints.clippy]`, + // stopping at the next table header. Scanning the whole file would let + // a `pedantic = ...` key in some unrelated table (or a + // `[package.metadata]` block) satisfy this test while the real + // package-wide gate had been removed or downgraded. + let mut in_table = false; + let mut table_lines: Vec<&str> = Vec::new(); + for line in toml.lines() { + let trimmed = line.trim(); + if trimmed.starts_with('[') { + in_table = trimmed == "[lints.clippy]"; + continue; + } + if in_table { + table_lines.push(trimmed); + } + } + assert!( - table.contains("pedantic"), - "Cargo.toml's `[lints.clippy]` table must set `pedantic` — that is the whole \ - reason the table exists (ENG-4684). Table as read: {table:?}" + !table_lines.is_empty(), + "Cargo.toml must declare a non-empty `[lints.clippy]` table so the pedantic \ + baseline applies to every target in the package, not just the library." ); - // `= "allow"` in either the table or the lib attributes would be a - // silent un-gating; only `warn` or `deny` are acceptable levels here. - let pedantic_line = toml - .lines() - .find(|l| l.trim_start().starts_with("pedantic")) - .expect("Cargo.toml must carry a `pedantic = ...` entry under [lints.clippy]"); + + let pedantic_line = table_lines + .iter() + .find(|l| l.starts_with("pedantic")) + .unwrap_or_else(|| { + panic!( + "Cargo.toml's `[lints.clippy]` table must set `pedantic` — that is the \ + whole reason the table exists (ENG-4684). Table as read: {table_lines:?}" + ) + }); + + // `= "allow"` here or in the lib attributes would be a silent + // un-gating; only `warn` or `deny` are acceptable levels. assert!( pedantic_line.contains("\"warn\"") || pedantic_line.contains("\"deny\""), "Cargo.toml's `pedantic` lint level must be `warn` or `deny`, not allow/forbid — \