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/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/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/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..38a9a69 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/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 +// (`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..b4380ad 100644 --- a/build/gpkg.rs +++ b/build/gpkg.rs @@ -1,17 +1,18 @@ //! 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 -//! 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..bf04eed 100644 --- a/build/gpkg_io.rs +++ b/build/gpkg_io.rs @@ -1,8 +1,21 @@ -//! 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. +//! (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 +// 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 +// `# 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; 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 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"[")?; diff --git a/src/data.rs b/src/data.rs index 40a780e..76cc542 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" ); @@ -139,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 8b1a8f6..ba723ba 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -55,25 +55,44 @@ /// 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. 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 +105,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 +148,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, diff --git a/src/lib.rs b/src/lib.rs index e92e355..8f0e91c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,8 +18,68 @@ //! 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 > via_suez.distance_km + 8_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)] +#![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/src/loader.rs b/src/loader.rs index 492d963..0ea79fd 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,29 +182,121 @@ 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. - pub fn from_bytes(bytes: &'static [u8]) -> Result { + /// + /// 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. let _ = rkyv::access::(&bytes[8..]) .map_err(LoadError::InvalidArchive)?; - Ok(Graph { + Ok(Self { backing: GraphBacking::Static(bytes), resolution_km: 0, }) } - /// 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. + /// + /// 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)?; + /// 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 { + 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)); @@ -245,7 +346,20 @@ 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, 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 // change the underlying file bytes out from under us. We treat // the mmap as immutable for the lifetime of the Graph: this @@ -263,23 +377,62 @@ impl Graph { let _ = rkyv::access::(&mmap[8..]) .map_err(LoadError::InvalidArchive)?; - Ok(Graph { + Ok(Self { backing: GraphBacking::Mmap(mmap), resolution_km, }) } - /// 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). - pub fn resolution_km(&self) -> u32 { + /// 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>(()) + /// ``` + #[must_use] + pub const 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>(()) + /// ``` + #[must_use] pub fn archived(&self) -> &ArchivedGraphData { let payload: &[u8] = match &self.backing { #[cfg(not(target_arch = "wasm32"))] @@ -290,24 +443,84 @@ 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>(()) + /// ``` + #[must_use] pub fn node_count(&self) -> u32 { - self.archived().nodes.len() as u32 + // 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 + } } /// 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>(()) + /// ``` + #[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. 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>(()) + /// ``` + #[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 + } } } @@ -353,6 +566,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(); @@ -417,6 +640,52 @@ 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>(()) + /// ``` + #[must_use = "the computed Route is the only result; dropping it does no work"] pub fn route( &self, from: (f64, f64), @@ -509,10 +778,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, @@ -611,13 +908,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 @@ -636,7 +935,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 @@ -651,10 +952,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 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..1df8c34 100644 --- a/tests/build_helpers.rs +++ b/tests/build_helpers.rs @@ -2,6 +2,14 @@ //! 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 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 +// are transcribed values -- a reader checking them against the source +// geometry wants them character-for-character. +#![allow(clippy::unreadable_literal)] #[path = "../build/geometry.rs"] mod geometry; 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:?}" + ); + } } // --------------------------------------------------------------------------- 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..8d7e879 100644 --- a/tests/golden_routes.rs +++ b/tests/golden_routes.rs @@ -22,6 +22,18 @@ //! 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 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"))] 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/lint_state.rs b/tests/lint_state.rs index f15a4b2..352af40 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,133 @@ 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(); + + // 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_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." + ); + + 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 — \ + CI escalates warnings with `-D warnings`. Offending line: {pedantic_line:?}" + ); +} 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;