router,autorouter: improve error handling

Avoid unwrapping and use `Result<...>` instead as well-written code
should.

src/autorouter/autorouter.rs

@@ -10,6 +10,7 @@ use petgraph::{
     visit::{EdgeRef, IntoEdgeReferences},
 };
 use spade::InsertionError;
+use thiserror::Error;
 
 use crate::{
     autorouter::{
@@ -22,10 +23,28 @@ use crate::{
         rules::RulesTrait,
     },
     layout::{Layout, NodeIndex},
-    router::{navmesh::Navmesh, Router, RouterError, RouterObserverTrait},
+    router::{
+        navmesh::{Navmesh, NavmeshError},
+        Router, RouterError, RouterObserverTrait,
+    },
     triangulation::GetVertexIndex,
 };
 
+#[derive(Error, Debug, Clone)]
+pub enum AutorouterError {
+    #[error("nothing to route")]
+    NothingToRoute,
+    #[error(transparent)]
+    Navmesh(#[from] NavmeshError),
+    #[error(transparent)]
+    Router(#[from] RouterError),
+}
+
+pub enum AutorouterStatus {
+    Running,
+    Finished,
+}
+
 pub struct Autoroute {
     ratlines_iter: Box<dyn Iterator<Item = EdgeIndex<usize>>>,
     navmesh: Option<Navmesh>, // Useful for debugging.
@@ -36,16 +55,16 @@ impl Autoroute {
     pub fn new(
         ratlines: impl IntoIterator<Item = EdgeIndex<usize>> + 'static,
         autorouter: &Autorouter<impl RulesTrait>,
-    ) -> Option<Self> {
+    ) -> Result<Self, AutorouterError> {
         let mut ratlines_iter = Box::new(ratlines.into_iter());
 
         let Some(cur_ratline) = ratlines_iter.next() else {
-            return None;
+            return Err(AutorouterError::NothingToRoute);
         };
 
         let (source, target) = Self::ratline_endpoints(autorouter, cur_ratline);
         let layout = autorouter.layout.lock().unwrap();
-        let navmesh = Some(Navmesh::new(&layout, source, target).ok()?);
+        let navmesh = Some(Navmesh::new(&layout, source, target)?);
 
         let this = Self {
             ratlines_iter,
@@ -53,14 +72,14 @@ impl Autoroute {
             cur_ratline: Some(cur_ratline),
         };
 
-        Some(this)
+        Ok(this)
     }
 
     pub fn step<R: RulesTrait>(
         &mut self,
         autorouter: &mut Autorouter<R>,
         observer: &mut impl RouterObserverTrait<R>,
-    ) -> bool {
+    ) -> Result<AutorouterStatus, AutorouterError> {
         let (new_navmesh, new_ratline) = if let Some(cur_ratline) = self.ratlines_iter.next() {
             let (source, target) = Self::ratline_endpoints(autorouter, cur_ratline);
 
@@ -78,16 +97,21 @@ impl Autoroute {
             std::mem::replace(&mut self.navmesh, new_navmesh).unwrap(),
         );
 
-        let Ok(band) = router.route_band(100.0, observer) else {
+        match router.route_band(100.0, observer) {
-            return false;
+            Ok(band) => {
-        };
-
                 autorouter
                     .ratsnest
                     .assign_band_to_ratline(self.cur_ratline.unwrap(), band);
                 self.cur_ratline = new_ratline;
 
-        self.navmesh.is_some()
+                if self.navmesh.is_some() {
+                    Ok(AutorouterStatus::Running)
+                } else {
+                    Ok(AutorouterStatus::Finished)
+                }
+            }
+            Err(err) => Err(AutorouterError::Router(err)),
+        }
     }
 
     fn ratline_endpoints<R: RulesTrait>(
@@ -138,15 +162,26 @@ impl<R: RulesTrait> Autorouter<R> {
         Ok(Self { layout, ratsnest })
     }
 
-    pub fn autoroute(&mut self, selection: &Selection, observer: &mut impl RouterObserverTrait<R>) {
+    pub fn autoroute(
-        if let Some(mut autoroute) = self.autoroute_walk(selection) {
+        &mut self,
-            while autoroute.step(self, observer) {
+        selection: &Selection,
-                //
+        observer: &mut impl RouterObserverTrait<R>,
+    ) -> Result<(), AutorouterError> {
+        let mut autoroute = self.autoroute_walk(selection)?;
+
+        loop {
+            let status = match autoroute.step(self, observer) {
+                Ok(status) => status,
+                Err(err) => return Err(err),
+            };
+
+            if let AutorouterStatus::Finished = status {
+                return Ok(());
             }
         }
     }
 
-    pub fn autoroute_walk(&self, selection: &Selection) -> Option<Autoroute> {
+    pub fn autoroute_walk(&self, selection: &Selection) -> Result<Autoroute, AutorouterError> {
         Autoroute::new(self.selected_ratlines(selection), self)
     }
 

src/autorouter/history.rs

@@ -1,7 +1,16 @@
 use serde::{Deserialize, Serialize};
+use thiserror::Error;
 
 use crate::autorouter::invoker::Command;
 
+#[derive(Error, Debug, Clone)]
+pub enum HistoryError {
+    #[error("no previous command")]
+    NoPreviousCommand,
+    #[error("no next command")]
+    NoNextCommand,
+}
+
 #[derive(Serialize, Deserialize)]
 pub struct History {
     done: Vec<Command>,
@@ -24,20 +33,36 @@ impl History {
         self.done.push(command);
     }
 
-    pub fn undo(&mut self) {
+    pub fn undo(&mut self) -> Result<(), HistoryError> {
-        let command = self.done.pop().unwrap();
+        let Some(command) = self.done.pop() else {
+            return Err(HistoryError::NoPreviousCommand);
+        };
+
         self.undone.push(command);
+        Ok(())
     }
 
-    pub fn redo(&mut self) {
+    pub fn redo(&mut self) -> Result<(), HistoryError> {
-        let command = self.undone.pop().unwrap();
+        let Some(command) = self.undone.pop() else {
+            return Err(HistoryError::NoNextCommand);
+        };
+
         self.done.push(command);
+        Ok(())
     }
 
     pub fn set_undone(&mut self, iter: impl IntoIterator<Item = Command>) {
         self.undone = Vec::from_iter(iter);
     }
 
+    pub fn last_done(&self) -> Result<&Command, HistoryError> {
+        self.done.last().ok_or(HistoryError::NoPreviousCommand)
+    }
+
+    pub fn last_undone(&self) -> Result<&Command, HistoryError> {
+        self.undone.last().ok_or(HistoryError::NoNextCommand)
+    }
+
     pub fn done(&self) -> &[Command] {
         &self.done
     }

src/autorouter/invoker.rs

@@ -1,12 +1,31 @@
 use core::fmt;
 
+use thiserror::Error;
+
 use crate::{
-    autorouter::{history::History, selection::Selection, Autoroute, Autorouter},
+    autorouter::{
+        history::{History, HistoryError},
+        selection::Selection,
+        Autoroute, Autorouter, AutorouterError, AutorouterStatus,
+    },
     drawing::rules::RulesTrait,
     layout::Layout,
     router::{EmptyRouterObserver, RouterObserverTrait},
 };
 
+#[derive(Error, Debug, Clone)]
+pub enum InvokerError {
+    #[error(transparent)]
+    History(#[from] HistoryError),
+    #[error(transparent)]
+    Autorouter(#[from] AutorouterError),
+}
+
+pub enum InvokerStatus {
+    Running,
+    Finished,
+}
+
 #[derive(serde::Serialize, serde::Deserialize)]
 pub enum Command {
     Autoroute(Selection),
@@ -17,13 +36,18 @@ pub enum Execute {
 }
 
 impl Execute {
-    pub fn next<R: RulesTrait>(
+    pub fn step<R: RulesTrait>(
         &mut self,
         invoker: &mut Invoker<R>,
         observer: &mut impl RouterObserverTrait<R>,
-    ) -> bool {
+    ) -> Result<InvokerStatus, InvokerError> {
         match self {
-            Execute::Autoroute(autoroute) => autoroute.step(&mut invoker.autorouter, observer),
+            Execute::Autoroute(autoroute) => {
+                match autoroute.step(&mut invoker.autorouter, observer)? {
+                    AutorouterStatus::Running => Ok(InvokerStatus::Running),
+                    AutorouterStatus::Finished => Ok(InvokerStatus::Finished),
+                }
+            }
         }
     }
 }
@@ -41,14 +65,24 @@ impl<R: RulesTrait> Invoker<R> {
         }
     }
 
-    pub fn execute(&mut self, command: Command, observer: &mut impl RouterObserverTrait<R>) {
+    pub fn execute(
+        &mut self,
+        command: Command,
+        observer: &mut impl RouterObserverTrait<R>,
+    ) -> Result<(), InvokerError> {
         let mut execute = self.execute_walk(command);
 
-        while execute.next(self, observer) {
+        loop {
-            //
+            let status = match execute.step(self, observer) {
-        }
+                Ok(status) => status,
+                Err(err) => return Err(err),
+            };
 
+            if let InvokerStatus::Finished = status {
                 self.history.set_undone(std::iter::empty());
+                return Ok(());
+            }
+        }
     }
 
     pub fn execute_walk(&mut self, command: Command) -> Execute {
@@ -65,25 +99,30 @@ impl<R: RulesTrait> Invoker<R> {
         }
     }
 
-    pub fn undo(&mut self) {
+    pub fn undo(&mut self) -> Result<(), InvokerError> {
-        let command = self.history.done().last().unwrap();
+        let command = self.history.last_done()?;
 
         match command {
             Command::Autoroute(ref selection) => self.autorouter.undo_autoroute(selection),
         }
 
-        self.history.undo();
+        Ok(self.history.undo()?)
     }
 
-    pub fn redo(&mut self) {
+    pub fn redo(&mut self) -> Result<(), InvokerError> {
-        let command = self.history.undone().last().unwrap();
+        let command = self.history.last_undone()?;
         let mut execute = self.dispatch_command(command);
 
-        while execute.next(self, &mut EmptyRouterObserver) {
+        loop {
-            //
+            let status = match execute.step(self, &mut EmptyRouterObserver) {
-        }
+                Ok(status) => status,
+                Err(err) => return Err(err),
+            };
 
-        self.history.redo();
+            if let InvokerStatus::Finished = status {
+                return Ok(self.history.redo()?);
+            }
+        }
     }
 
     pub fn replay(&mut self, history: History) {

src/bin/topola-egui/app.rs

@@ -12,7 +12,7 @@ use std::{
 
 use topola::{
     autorouter::{
-        invoker::{Command, Execute, Invoker},
+        invoker::{Command, Execute, Invoker, InvokerStatus},
         Autorouter,
     },
     drawing::{
@@ -266,17 +266,26 @@ impl eframe::App for App {
                                 }
                             }
 
-                            while execute.next(
+                            let _ = loop {
+                                let status = match execute.step(
                                     &mut invoker,
                                     &mut DebugRouterObserver {
                                         shared_data: shared_data_arc_mutex.clone(),
                                     },
                                 ) {
+                                    Ok(status) => status,
+                                    Err(err) => return, //Err(err),
+                                };
+
                                 if let Execute::Autoroute(ref mut autoroute) = execute {
                                     shared_data_arc_mutex.lock().unwrap().navmesh =
                                         autoroute.navmesh().clone();
                                 }
+
+                                if let InvokerStatus::Finished = status {
+                                    break;
                                 }
+                            };
                         });
                     }
                 }
@@ -297,7 +306,9 @@ impl eframe::App for App {
                 {
                     if let Some(invoker_arc_mutex) = &self.invoker {
                         let invoker_arc_mutex = invoker_arc_mutex.clone();
-                        execute(async move { invoker_arc_mutex.lock().unwrap().redo() });
+                        execute(async move {
+                            invoker_arc_mutex.lock().unwrap().redo();
+                        });
                     }
                 }
 

src/router/astar.rs

@@ -133,7 +133,6 @@ where
 {
     Running,
     Finished(K, Vec<G::NodeId>, R),
-    Error(AstarError),
 }
 
 impl<G, K> Astar<G, K>
@@ -158,15 +157,18 @@ where
         this
     }
 
-    pub fn step<R>(&mut self, strategy: &mut impl AstarStrategy<G, K, R>) -> AstarStatus<G, K, R> {
+    pub fn step<R>(
+        &mut self,
+        strategy: &mut impl AstarStrategy<G, K, R>,
+    ) -> Result<AstarStatus<G, K, R>, AstarError> {
         let Some(MinScored(estimate_score, node)) = self.visit_next.pop() else {
-            return AstarStatus::Error(AstarError::NotFound);
+            return Err(AstarError::NotFound);
         };
 
         if let Some(result) = strategy.is_goal(node, &self.path_tracker) {
             let path = self.path_tracker.reconstruct_path_to(node);
             let cost = self.scores[&node];
-            return AstarStatus::Finished(cost, path, result);
+            return Ok(AstarStatus::Finished(cost, path, result));
         }
 
         // This lookup can be unwrapped without fear of panic since the node was
@@ -178,7 +180,7 @@ where
                 // If the node has already been visited with an equal or lower score than
                 // now, then we do not need to re-visit it.
                 if *entry.get() <= estimate_score {
-                    return AstarStatus::Running;
+                    return Ok(AstarStatus::Running);
                 }
                 entry.insert(estimate_score);
             }
@@ -197,7 +199,7 @@ where
                         // No need to add neighbors that we have already reached through a
                         // shorter path than now.
                         if *entry.get() <= next_score {
-                            return AstarStatus::Running;
+                            return Ok(AstarStatus::Running);
                         }
                         entry.insert(next_score);
                     }
@@ -212,7 +214,7 @@ where
             }
         }
 
-        AstarStatus::Running
+        Ok(AstarStatus::Running)
     }
 }
 
@@ -229,16 +231,13 @@ where
     let mut astar = Astar::new(graph, start, strategy);
 
     loop {
-        let status = astar.step(strategy);
+        let status = match astar.step(strategy) {
+            Ok(status) => status,
+            Err(err) => return Err(err),
+        };
 
-        /*if !matches!(status, AstarStatus::Running) {
+        if let AstarStatus::Finished(cost, path, band) = status {
-            return status;
+            return Ok((cost, path, band));
-        }*/
-
-        match status {
-            AstarStatus::Running => (),
-            AstarStatus::Finished(cost, path, band) => return Ok((cost, path, band)),
-            AstarStatus::Error(err) => return Err(err),
         }
     }
 }

src/router/router.rs

@@ -25,24 +25,6 @@ use crate::{
     },
 };
 
-/*#[derive(Error, Debug, Clone, Copy)]
-#[error("failed to route from {from:?} to {to:?}")] // this should eventually use Display
-pub struct RouterError {
-    from: FixedDotIndex,
-    to: FixedDotIndex,
-    source: RoutingErrorKind,
-}
-
-#[derive(Error, Debug, Clone, Copy)]
-pub enum RoutingErrorKind {
-    #[error(transparent)]
-    MeshInsertion(#[from] InsertionError),
-    // exposing more details here seems difficult
-    // TODO more descriptive message
-    #[error("A* found no path")]
-    AStar,
-}*/
-
 #[derive(Error, Debug, Clone)]
 #[error("routing failed")]
 pub enum RouterError {