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); +}