From aa479f4f0b68ea51187677ce9271f7bf8a45df5f Mon Sep 17 00:00:00 2001 From: tachsin Date: Sat, 12 Sep 2026 10:00:53 +0300 Subject: [PATCH] fix: panic instead of returning a wrapped path cost Adding costs could overflow the cost type. Debug builds panicked; release builds wrapped and returned a small, plausible-looking, wrong cost. The cost type is only required to be Zero + Ord + Copy, so checked_add is not available and requiring CheckedAdd would be a breaking change. Costs are required to be non-negative, though, and that is enough to spot a wrapped sum without a new bound: adding a non-negative value can never produce a smaller one, so a sum that compares less than the value it was added to must have wrapped. Release builds now agree with debug builds. Cost types whose addition saturates, and floating point costs which reach infinity rather than wrapping, compare greater and are left alone, so the documented workaround of wrapping the cost type keeps working. Also documents the panic on the affected functions, and drops five expect(clippy::missing_panics_doc) attributes that those docs make unnecessary. --- src/directed/astar.rs | 32 +++++++-- src/directed/dijkstra.rs | 48 +++++++++++-- src/directed/fringe.rs | 13 +++- src/directed/idastar.rs | 21 +++++- src/directed/yen.rs | 10 ++- src/lib.rs | 36 ++++++++++ tests/cost_overflow.rs | 152 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 292 insertions(+), 20 deletions(-) create mode 100644 tests/cost_overflow.rs diff --git a/src/directed/astar.rs b/src/directed/astar.rs index d6503d46..2b63d66b 100644 --- a/src/directed/astar.rs +++ b/src/directed/astar.rs @@ -1,6 +1,7 @@ //! Compute a shortest path (or all shorted paths) using the [A* search //! algorithm](https://en.wikipedia.org/wiki/A*_search_algorithm). +use crate::add_costs; use indexmap::map::Entry::{Occupied, Vacant}; use num_traits::Zero; use std::cmp::Ordering; @@ -30,6 +31,13 @@ use crate::FxIndexMap; /// /// The returned path comprises both the start and end node. /// +/// # Panics +/// +/// This function panics if the cost of a path, or the sum of a path cost and a heuristic +/// estimate, does not fit into `C`. Silently returning a wrapped, and therefore wrong, cost +/// would be worse than failing loudly. If your costs can come close to the limits of the +/// type, use a wider type, or a wrapper type whose addition saturates. +/// /// # Example /// /// We will search the shortest path on a chess board to go from (1, 1) to (4, 6) doing only knight @@ -77,7 +85,6 @@ use crate::FxIndexMap; /// |&p| p == GOAL); /// assert_eq!(result.expect("no path found").1, 4); /// ``` -#[expect(clippy::missing_panics_doc)] pub fn astar( start: &N, mut successors: FN, @@ -116,7 +123,7 @@ where successors(node) }; for (successor, move_cost) in successors { - let new_cost = cost + move_cost; + let new_cost = add_costs(cost, move_cost); let h; // heuristic(&successor) let n; // index for successor match parents.entry(successor) { @@ -137,7 +144,7 @@ where } to_see.push(SmallestCostHolder { - estimated_cost: new_cost + h, + estimated_cost: add_costs(new_cost, h), cost: new_cost, index: n, }); @@ -169,7 +176,13 @@ where /// /// Each path comprises both the start and an end node. Note that while every path shares the same /// start node, different paths may have different end nodes. -#[expect(clippy::missing_panics_doc)] +/// +/// # Panics +/// +/// This function panics if the cost of a path, or the sum of a path cost and a heuristic +/// estimate, does not fit into `C`. Silently returning a wrapped, and therefore wrong, cost +/// would be worse than failing loudly. If your costs can come close to the limits of the +/// type, use a wider type, or a wrapper type whose addition saturates. pub fn astar_bag( start: &N, mut successors: FN, @@ -223,7 +236,7 @@ where successors(node) }; for (successor, move_cost) in successors { - let new_cost = cost + move_cost; + let new_cost = add_costs(cost, move_cost); let h; // heuristic(&successor) let n; // index for successor match parents.entry(successor) { @@ -255,7 +268,7 @@ where } to_see.push(SmallestCostHolder { - estimated_cost: new_cost + h, + estimated_cost: add_costs(new_cost, h), cost: new_cost, index: n, }); @@ -289,6 +302,13 @@ where /// ### Warning /// /// The number of results with the same value might be very large in some graphs. Use with caution. +/// +/// # Panics +/// +/// This function panics if the cost of a path, or the sum of a path cost and a heuristic +/// estimate, does not fit into `C`. Silently returning a wrapped, and therefore wrong, cost +/// would be worse than failing loudly. If your costs can come close to the limits of the +/// type, use a wider type, or a wrapper type whose addition saturates. pub fn astar_bag_collect( start: &N, successors: FN, diff --git a/src/directed/dijkstra.rs b/src/directed/dijkstra.rs index 60f03497..a985a0e7 100644 --- a/src/directed/dijkstra.rs +++ b/src/directed/dijkstra.rs @@ -3,6 +3,7 @@ use super::reverse_path; use crate::FxIndexMap; +use crate::add_costs; use indexmap::map::Entry::{Occupied, Vacant}; use num_traits::Zero; use std::cmp::Ordering; @@ -26,6 +27,13 @@ use std::hash::Hash; /// /// The returned path comprises both the start and end node. /// +/// # Panics +/// +/// This function panics if the cost of a path does not fit into `C`. Silently returning a +/// wrapped, and therefore wrong, cost would be worse than failing loudly. If your costs can +/// come close to the limits of the type, use a wider type, or a wrapper type whose addition +/// saturates. +/// /// # Example /// /// We will search the shortest path on a chess board to go from (1, 1) to (4, 6) doing only knight @@ -129,6 +137,13 @@ where /// /// The returned path comprises both the start and end node. /// +/// # Panics +/// +/// This function panics if the cost of a path does not fit into `C`. Silently returning a +/// wrapped, and therefore wrong, cost would be worse than failing loudly. If your costs can +/// come close to the limits of the type, use a wider type, or a wrapper type whose addition +/// saturates. +/// /// # Example /// /// We search the shortest path on a chess board to go from (1, 1) to (4, 6) doing only knight @@ -147,7 +162,6 @@ where /// let result = dijkstra_bidirectional(&(1, 1), &(4, 6), neighbours, neighbours); /// assert_eq!(result.expect("no path found").1, 4); /// ``` -#[expect(clippy::missing_panics_doc)] pub fn dijkstra_bidirectional( start: &N, end: &N, @@ -200,7 +214,7 @@ where }; // Any path the two searches have not joined up yet costs at least as much as the sum of // the two frontier costs, so once that reaches the best known path nothing better is left. - if best.is_some_and(|(cost, ..)| forward_min.cost + backward_min.cost >= cost) { + if best.is_some_and(|(cost, ..)| add_costs(forward_min.cost, backward_min.cost) >= cost) { break; } expand_bidirectional( @@ -283,7 +297,7 @@ fn expand_bidirectional( neighbours(node) }; for (neighbour, move_cost) in neighbours { - let new_cost = cost + move_cost; + let new_cost = add_costs(cost, move_cost); let n; match parents.entry(neighbour) { Vacant(e) => { @@ -307,7 +321,7 @@ fn expand_bidirectional( // complete path; keep it if it is the cheapest one seen so far. let neighbour = parents.get_index(n).unwrap().0; if let Some((opposite_index, _, &(_, opposite_cost))) = opposite.get_full(neighbour) { - let total = new_cost + opposite_cost; + let total = add_costs(new_cost, opposite_cost); if best.is_none_or(|(current, ..)| total < current) { *best = Some(if is_forward { (total, n, opposite_index) @@ -334,6 +348,13 @@ fn expand_bidirectional( /// The [`build_path`] function can be used to build a full path from the starting point to one /// of the reachable targets. /// +/// # Panics +/// +/// This function panics if the cost of a path does not fit into `C`. Silently returning a +/// wrapped, and therefore wrong, cost would be worse than failing loudly. If your costs can +/// come close to the limits of the type, use a wider type, or a wrapper type whose addition +/// saturates. +/// /// # Example /// /// We use a graph of integer nodes from 1 to 9, each node leading to its double and the value @@ -383,7 +404,13 @@ where /// /// The [`build_path`] function can be used to build a full path from the starting point to one /// of the reachable targets. -#[expect(clippy::missing_panics_doc)] +/// +/// # Panics +/// +/// This function panics if the cost of a path does not fit into `C`. Silently returning a +/// wrapped, and therefore wrong, cost would be worse than failing loudly. If your costs can +/// come close to the limits of the type, use a wider type, or a wrapper type whose addition +/// saturates. pub fn dijkstra_partial( start: &N, mut successors: FN, @@ -443,7 +470,7 @@ where successors(node) }; for (successor, move_cost) in successors { - let new_cost = cost + move_cost; + let new_cost = add_costs(cost, move_cost); let n; match parents.entry(successor) { Vacant(e) => { @@ -585,7 +612,7 @@ where (self.successors)(node) }; for (successor, move_cost) in successors { - let new_cost = cost + move_cost; + let new_cost = add_costs(cost, move_cost); let n; match self.parents.entry(successor) { Vacant(e) => { @@ -620,6 +647,13 @@ where /// /// The `successors` function receives the current node, and returns /// an iterator of successors associated with their move cost. +/// +/// # Panics +/// +/// This function panics if the cost of a path does not fit into `C`. Silently returning a +/// wrapped, and therefore wrong, cost would be worse than failing loudly. If your costs can +/// come close to the limits of the type, use a wider type, or a wrapper type whose addition +/// saturates. pub fn dijkstra_reach(start: &N, successors: FN) -> DijkstraReachable where N: Eq + Hash + Clone, diff --git a/src/directed/fringe.rs b/src/directed/fringe.rs index bb4947a8..cb1547fb 100644 --- a/src/directed/fringe.rs +++ b/src/directed/fringe.rs @@ -3,6 +3,7 @@ use super::reverse_path; use crate::FxIndexMap; +use crate::add_costs; use indexmap::map::Entry::{Occupied, Vacant}; use num_traits::{Bounded, Zero}; use std::collections::VecDeque; @@ -28,6 +29,13 @@ use std::mem; /// /// The returned path comprises both the start and end node. /// +/// # Panics +/// +/// This function panics if the cost of a path, or the sum of a path cost and a heuristic +/// estimate, does not fit into `C`. Silently returning a wrapped, and therefore wrong, cost +/// would be worse than failing loudly. If your costs can come close to the limits of the +/// type, use a wider type, or a wrapper type whose addition saturates. +/// /// # Example /// /// We will search the shortest path on a chess board to go from (1, 1) to (4, 6) doing only knight @@ -77,7 +85,6 @@ use std::mem; /// |&p| p == GOAL); /// assert_eq!(result.expect("no path found").1, 4); /// ``` -#[expect(clippy::missing_panics_doc)] pub fn fringe( start: &N, mut successors: FN, @@ -117,7 +124,7 @@ where } let (g, successors) = { let (node, &(_, g)) = parents.get_index(i).unwrap(); // Cannot fail - let f = g + heuristic(node); + let f = add_costs(g, heuristic(node)); if f > flimit { if f < fmin { fmin = f; @@ -132,7 +139,7 @@ where (g, successors(node)) }; for (successor, cost) in successors { - let g_successor = g + cost; + let g_successor = add_costs(g, cost); let n; // index for successor match parents.entry(successor) { Vacant(e) => { diff --git a/src/directed/idastar.rs b/src/directed/idastar.rs index f3d18983..2a2b8431 100644 --- a/src/directed/idastar.rs +++ b/src/directed/idastar.rs @@ -2,6 +2,7 @@ //! algorithm](https://en.wikipedia.org/wiki/Iterative_deepening_A*). use crate::FxIndexSet; +use crate::add_costs; use num_traits::Zero; use std::{hash::Hash, ops::ControlFlow}; @@ -24,6 +25,13 @@ use std::{hash::Hash, ops::ControlFlow}; /// /// The returned path comprises both the start and end node. /// +/// # Panics +/// +/// This function panics if the cost of a path, or the sum of a path cost and a heuristic +/// estimate, does not fit into `C`. Silently returning a wrapped, and therefore wrong, cost +/// would be worse than failing loudly. If your costs can come close to the limits of the +/// type, use a wider type, or a wrapper type whose addition saturates. +/// /// # Example /// /// We will search the shortest path on a chess board to go from (1, 1) to (4, 6) doing only knight @@ -124,7 +132,7 @@ where { let neighbs = { let start = &path[path.len() - 1]; - let f = cost + heuristic(start); + let f = add_costs(cost, heuristic(start)); if f > bound { return ControlFlow::Continue(Some(f)); } @@ -136,7 +144,7 @@ where .filter_map(|(n, c)| { (!path.contains(&n)).then(|| { let h = heuristic(&n); - (n, c, c + h) + (n, c, add_costs(c, h)) }) }) .collect::>(); @@ -146,7 +154,14 @@ where let mut min = None; for (node, extra, _) in neighbs { let (idx, _) = path.insert_full(node); - match search(path, cost + extra, bound, successors, heuristic, success)? { + match search( + path, + add_costs(cost, extra), + bound, + successors, + heuristic, + success, + )? { Some(m) if min.is_none_or(|n| n >= m) => min = Some(m), _ => (), } diff --git a/src/directed/yen.rs b/src/directed/yen.rs index 908a1441..cd7b54d6 100644 --- a/src/directed/yen.rs +++ b/src/directed/yen.rs @@ -1,5 +1,6 @@ //! Compute k-shortest paths using [Yen's search //! algorithm](https://en.wikipedia.org/wiki/Yen%27s_algorithm). +use crate::add_costs; use num_traits::Zero; use rustc_hash::FxHashSet; use std::cmp::Ordering; @@ -61,6 +62,13 @@ where /// starting with the lowest cost. If there exist less paths than requested, only the existing /// ones (if any) are returned. /// +/// # Panics +/// +/// This function panics if the cost of a path does not fit into `C`. Silently returning a +/// wrapped, and therefore wrong, cost would be worse than failing loudly. If your costs can +/// come close to the limits of the type, use a wider type, or a wrapper type whose addition +/// saturates. +/// /// # Example /// We will search the 3 shortest paths from node C to node H. See /// for a visualization. @@ -178,7 +186,7 @@ where // Build a min-heap k_routes.push(Reverse(Path { nodes, - cost: root_costs[i] + spur_cost, + cost: add_costs(root_costs[i], spur_cost), spur_index: i, })); } diff --git a/src/lib.rs b/src/lib.rs index 95dfc73c..284c4857 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -129,6 +129,42 @@ use std::hash::BuildHasherDefault; type FxIndexMap = IndexMap>; type FxIndexSet = IndexSet>; +/// Report a cost addition that wrapped. +/// +/// Kept out of line and marked cold so that [`add_costs`] stays small enough to inline: the +/// check sits in the innermost loop of every cost-based search, and a panic formatted inline +/// there would cost more than the check itself. +#[cold] +#[inline(never)] +fn cost_overflow() -> ! { + panic!("cost overflow: the total path cost does not fit in the cost type"); +} + +/// Add two costs, panicking if the sum wrapped around instead of growing. +/// +/// Cost types are only required to be `Zero + Ord + Copy`, so `checked_add` is not available +/// here and requiring `num_traits::CheckedAdd` would be a breaking change. Costs are however +/// required to be non-negative, and that is enough: adding a non-negative value can never +/// produce a smaller one, so a sum that compares less than the value it was added to must +/// have wrapped. +/// +/// This makes release builds agree with debug builds, which already panic on overflow, rather +/// than silently returning a wrapped and therefore wrong cost. Cost types that saturate, and +/// floating point costs which reach infinity rather than wrapping, compare greater and are +/// left untouched. Negative addends are left alone as well, since they are outside what these +/// algorithms support and this check cannot say anything useful about them. +#[inline] +pub(crate) fn add_costs(a: C, b: C) -> C +where + C: num_traits::Zero + Ord + Copy, +{ + let sum = a + b; + if sum < a && b >= C::zero() { + cost_overflow(); + } + sum +} + /// Export all public functions and structures for an easy access. pub mod prelude { pub use crate::directed::astar::*; diff --git a/tests/cost_overflow.rs b/tests/cost_overflow.rs new file mode 100644 index 00000000..b3f3defd --- /dev/null +++ b/tests/cost_overflow.rs @@ -0,0 +1,152 @@ +//! A path cost that does not fit in the cost type must not be reported as if it did. +//! +//! Debug builds have always panicked on such an addition. Release builds used to wrap, which +//! turned an impossible-to-represent cost into a small plausible-looking one. These tests pin +//! the behaviour down in both profiles. + +use pathfinding::prelude::*; + +// The panic message differs by profile: debug builds trip the built-in "attempt to add with +// overflow" inside `+` before the library's own check runs, while release builds reach the +// check and report "cost overflow". Both contain "overflow", which is what these tests pin. + +/// Three of these do not fit in a `u32`: the wrapped sum would be 2147483645. +const HUGE: u32 = u32::MAX / 2; + +// The successor and success callbacks are handed a reference by the search functions. +#[expect(clippy::trivially_copy_pass_by_ref)] +fn expensive(n: &u32) -> Vec<(u32, u32)> { + vec![(n + 1, HUGE)] +} + +// The successor and success callbacks are handed a reference by the search functions. +#[expect(clippy::trivially_copy_pass_by_ref)] +fn cheap(n: &u32) -> Vec<(u32, u32)> { + vec![(n + 1, 10)] +} + +// The successor and success callbacks are handed a reference by the search functions. +#[expect(clippy::trivially_copy_pass_by_ref)] +const fn at_three(n: &u32) -> bool { + *n == 3 +} + +#[test] +#[should_panic(expected = "overflow")] +fn astar_refuses_to_wrap() { + astar(&0, expensive, |_| 0, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn astar_bag_refuses_to_wrap() { + astar_bag(&0, expensive, |_| 0, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn dijkstra_refuses_to_wrap() { + dijkstra(&0, expensive, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn dijkstra_all_refuses_to_wrap() { + dijkstra_all(&0, |n: &u32| if *n < 4 { expensive(n) } else { vec![] }); +} + +#[test] +#[should_panic(expected = "overflow")] +fn dijkstra_reach_refuses_to_wrap() { + dijkstra_reach(&0, |n: &u32| if *n < 4 { expensive(n) } else { vec![] }).for_each(drop); +} + +#[test] +#[should_panic(expected = "overflow")] +fn fringe_refuses_to_wrap() { + fringe(&0, expensive, |_| 0, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn idastar_refuses_to_wrap() { + idastar(&0, expensive, |_| 0, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn yen_refuses_to_wrap() { + yen(&0, expensive, at_three, 2); +} + +/// The reported case: the path cost is representable, but adding the heuristic is not. +#[test] +#[should_panic(expected = "overflow")] +fn astar_checks_the_heuristic_too() { + astar(&0, cheap, |_| u32::MAX, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn fringe_checks_the_heuristic_too() { + fringe(&0, cheap, |_| u32::MAX, at_three); +} + +#[test] +#[should_panic(expected = "overflow")] +fn idastar_checks_the_heuristic_too() { + idastar(&0, cheap, |_| u32::MAX, at_three); +} + +/// A cost type whose addition saturates is the documented way to opt out of the panic. It +/// must keep working: a saturating sum never compares smaller, so it never trips the check. +mod saturating { + use super::{HUGE, at_three}; + use pathfinding::prelude::*; + + // The panic message differs by profile: debug builds trip the built-in "attempt to add with + // overflow" inside `+` before the library's own check runs, while release builds reach the + // check and report "cost overflow". Both contain "overflow", which is what these tests pin. + + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] + struct Saturating(u32); + + impl std::ops::Add for Saturating { + type Output = Self; + fn add(self, other: Self) -> Self { + Self(self.0.saturating_add(other.0)) + } + } + + impl num_traits::Zero for Saturating { + fn zero() -> Self { + Self(0) + } + fn is_zero(&self) -> bool { + self.0 == 0 + } + } + + #[test] + fn a_saturating_cost_type_does_not_panic() { + let (path, cost) = astar( + &0, + |n: &u32| vec![(n + 1, Saturating(HUGE))], + |_| Saturating(0), + at_three, + ) + .unwrap(); + assert_eq!(path, vec![0, 1, 2, 3]); + assert_eq!(cost, Saturating(u32::MAX)); + } +} + +/// Costs that fit must be entirely unaffected. +#[test] +fn ordinary_costs_are_untouched() { + assert_eq!(astar(&0, cheap, |_| 0, at_three).unwrap().1, 30); + assert_eq!(dijkstra(&0, cheap, at_three).unwrap().1, 30); + assert_eq!(fringe(&0, cheap, |_| 0, at_three).unwrap().1, 30); + assert_eq!(idastar(&0, cheap, |_| 0, at_three).unwrap().1, 30); + assert_eq!(yen(&0, cheap, at_three, 1)[0].1, 30); +}