enh: Error handling

[cedricchevalier19]

Discuss and design a coherent error handling.

Information

• [x] Enhancement of existing content
• [ ] Suggestion for a new feature
• [ ] Other: ...

Suggested changes

Current state

There are:

• some custom Error types
• some "warnings"
• some panic or unimplemented

See #169, #174

Proposal

I'd appreciate it if you could find a coherent way to deal with different errors. It should be lightweight, but it should be thought out thoroughly to make honeycomb usable as a library.

[imrn99]

good point for usability. I'll use this issue to gather relevant data. I'll ignore items from examples / benches since those don't affect the library.

[imrn99]

explicit panics, expects

Ignoring unreachable panics.

panics

honeycomb-core/src/cmap/builder/io/mod.rs:27:    Vtk::import(file_path).unwrap_or_else(|e| panic!("E: failed to load file: {e:?}"));
honeycomb-core/src/cmap/builder/io/mod.rs:49:    Vtk::import(value).unwrap_or_else(|e| panic!("E: failed to load file: {e:?}"));
honeycomb-kernels/src/grisubal/mod.rs:144:       Err(e) => panic!("E: could not open specified vtk file - {e}"),

expects

honeycomb-core/src/attributes/collections.rs:65:         .expect("E: cannot split attribute value - value not found in storage");
honeycomb-core/src/attributes/collections.rs:179:        .expect("E: cannot split attribute value - value not found in storage");
honeycomb-core/src/attributes/manager.rs:320:            .expect("E: could not find storage associated to the specified attribute type")
honeycomb-core/src/attributes/manager.rs:322:            .expect("E: could not downcast generic storage to specified attribute type");
honeycomb-core/src/attributes/manager.rs:335:            .expect("E: could not find storage associated to the specified attribute type")
honeycomb-core/src/attributes/manager.rs:337:            .expect("E: could not downcast generic storage to specified attribute type");
honeycomb-core/src/attributes/manager.rs:414:            .expect("E: could not downcast generic storage to specified attribute type")
honeycomb-core/src/cmap/builder/io/mod.rs:125:           .expect("failed to load piece data - is it not inlined?");
honeycomb-core/src/cmap/dim2/io.rs:49:                   .expect("Could not write data to writer");
honeycomb-core/src/cmap/dim2/io.rs:75:                   .expect("Could not write data to writer");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:67:         .expect("E: attempt to split an edge that is not fully defined in the first place");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:70:         .expect("E: attempt to split an edge that is not fully defined in the first place");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:89:         .expect("E: attempt to split an edge that is not fully defined in the first place");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:92:         .expect("E: attempt to split an edge that is not fully defined in the first place");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:192:        .expect("E: attempt to split an edge that is not fully defined in the first place");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:199:        .expect("E: attempt to split an edge that is not fully defined in the first place");
oneycomb-kernels/src/grisubal/model.rs:98:               .expect("E: failed to load piece data - is it not inlined?");
honeycomb-kernels/src/grisubal/model.rs:351:             .expect("E: open geometry?");
honeycomb-kernels/src/grisubal/model.rs:362:             .expect("E: open geometry?");
honeycomb-kernels/src/grisubal/kernel.rs:93:             .expect("E: could not build overlapping grid map");

[imrn99]

unwraps (fixed)

I removed unwrap from type conversion / cast, as well as unwrap from tests. ! indicates that I changed it

! honeycomb-core/src/cmap/builder/io/mod.rs:158:   cell_components.last_mut().unwrap().push(*vertex_id as usize);
! honeycomb-core/src/cmap/dim2/io.rs:95:           .map(|vid| map.vertex(*vid).unwrap())
! honeycomb-core/src/cmap/dim2/basic_ops.rs:269:   .unwrap() as VertexIdentifier
! honeycomb-core/src/cmap/dim2/basic_ops.rs:303:   .unwrap() as EdgeIdentifier
! honeycomb-core/src/cmap/dim2/basic_ops.rs:337:   .unwrap() as FaceIdentifier
! honeycomb-core/src/cmap/dim2/utils.rs:94:        let mut file = File::create(rootname.to_owned() + "_allocated.csv").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:95:        writeln!(file, "key, memory (bytes)").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:101:       writeln!(file, "beta_{beta_id}, {mem}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:104:       writeln!(file, "beta_total, {beta_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:110:       writeln!(file, "geometry_vertex, {geometry_vertex}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:111:       writeln!(file, "geometry_total, {geometry_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:117:       writeln!(file, "others_freedarts, {others_freedarts}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:118:       writeln!(file, "others_counters, {others_counters}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:119:       writeln!(file, "others_total, {others_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:166:       let mut file = File::create(rootname.to_owned() + "_effective.csv").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:167:       writeln!(file, "key, memory (bytes)").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:173:       writeln!(file, "beta_{beta_id}, {mem}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:176:       writeln!(file, "beta_total, {beta_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:182:       writeln!(file, "geometry_vertex, {geometry_vertex}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:183:       writeln!(file, "geometry_total, {geometry_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:189:       writeln!(file, "others_freedarts, {others_freedarts}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:190:       writeln!(file, "others_counters, {others_counters}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:191:       writeln!(file, "others_total, {others_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:240:       let mut file = File::create(rootname.to_owned() + "_used.csv").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:241:       writeln!(file, "key, memory (bytes)").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:249:       writeln!(file, "beta_{beta_id}, {mem}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:252:       writeln!(file, "beta_total, {beta_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:258:       writeln!(file, "geometry_vertex, {geometry_vertex}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:259:       writeln!(file, "geometry_total, {geometry_total}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:265:       writeln!(file, "others_freedarts, {others_freedarts}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:266:       writeln!(file, "others_counters, {others_counters}").unwrap();
! honeycomb-core/src/cmap/dim2/utils.rs:267:       writeln!(file, "others_total, {others_total}").unwrap();
! honeycomb-core/src/geometry/vertex.rs:212:       attr.unwrap()
! honeycomb-kernels/src/grisubal/clip.rs:90:       return Some((dart_id, cmap.vertex(cmap.vertex_id(dart_id)).unwrap()));
! honeycomb-kernels/src/grisubal/model.rs:134:     cell_components.last_mut().unwrap().push(*vertex_id as usize);
! honeycomb-kernels/src/grisubal/kernel.rs:194:    let v_dart = cmap.vertex(cmap.vertex_id(dart_id)).unwrap();
! honeycomb-kernels/src/grisubal/kernel.rs:248:    let v_dart = cmap.vertex(cmap.vertex_id(dart_id)).unwrap();
! honeycomb-kernels/src/grisubal/kernel.rs:290:    let v_dart = cmap.vertex(cmap.vertex_id(dart_id)).unwrap();
! honeycomb-kernels/src/grisubal/kernel.rs:343:    let v_vdart = cmap.vertex(cmap.vertex_id(vdart_id)).unwrap();
! honeycomb-kernels/src/grisubal/kernel.rs:344:    let v_hdart = cmap.vertex(cmap.vertex_id(hdart_id)).unwrap();
! honeycomb-kernels/src/grisubal/kernel.rs:405:    intersec_data.sort_by(|(s1, _, _), (s2, _, _)| s1.partial_cmp(s2).unwrap());
! honeycomb-kernels/src/grisubal/kernel.rs:472:    vs.sort_by(|(_, t1, _), (_, t2, _)| t1.partial_cmp(t2).unwrap());
! honeycomb-render/src/capture/mod.rs:75:          let v = cmap.vertex(*vid).unwrap();
honeycomb-render/src/capture/mod.rs:76:          Vec3::from((v.0.to_f32().unwrap(), v.1.to_f32().unwrap(), 0.0))

[imrn99]

non-crashing

warnings

honeycomb-core/src/attributes/manager.rs:372:           "W: Storage of attribute `{}` already exists in the attribute storage manager",
honeycomb-core/src/cmap/builder/grid/descriptor.rs:129: println!("W: All three grid parameters were specified, total lengths will be ignored");
honeycomb-core/src/cmap/dim2/io.rs:162:                 println!("W: unrecognized coordinate type -- cast to f64 might fail");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:55:        println!("W: vertex placement for split is not in ]0;1[ -- result may be incoherent");
honeycomb-core/src/cmap/dim2/advanced_ops.rs:214:       "W: vertex placement for split is not in ]0;1[ -- result may be incoherent"
honeycomb-kernels/src/grisubal/model.rs:272:             println!("W: land on corner: {on_corner} - reflect on an axis: {reflect}, shifting origin");

Result in returns

honeycomb-core/src/cmap/builder/grid/descriptor.rs:124:    pub(crate) fn parse_2d(self) -> Result<(Vertex2<T>, [usize; 2], [T; 2]), BuilderError> {
honeycomb-core/src/cmap/builder/io/mod.rs:107:             ) -> Result<CMap2<T>, BuilderError> {
honeycomb-core/src/cmap/builder/structure.rs:120:          pub fn build(self) -> Result<CMap2<T>, BuilderError> {
honeycomb-core/src/geometry/vector.rs:126:                 pub fn unit_dir(&self) -> Result<Self, CoordsError> {
honeycomb-core/src/geometry/vector.rs:151:                 pub fn normal_dir(&self) -> Result<Vector2<T>, CoordsError> {
honeycomb-kernels/src/grisubal/clip.rs:14:                 pub fn clip_left<T: CoordsFloat>(cmap: &mut CMap2<T>) -> Result<(), GrisubalError> {
honeycomb-kernels/src/grisubal/clip.rs:25:                 pub fn clip_right<T: CoordsFloat>(cmap: &mut CMap2<T>) -> Result<(), GrisubalError> {
honeycomb-kernels/src/grisubal/clip.rs:42:                 ) -> Result<HashSet<FaceIdentifier>, GrisubalError> {
honeycomb-kernels/src/grisubal/model.rs:76:                fn try_from(value: Vtk) -> Result<Self, Self::Error> {
honeycomb-kernels/src/grisubal/model.rs:202:               ) -> Result<(), GrisubalError> {
honeycomb-kernels/src/grisubal/model.rs:228:               ) -> Result<([usize; 2], Vertex2<T>), GrisubalError> {
honeycomb-kernels/src/grisubal/mod.rs:140:                 ) -> Result<CMap2<T>, GrisubalError> {

[imrn99]

I've come up with a list of principles that can be used to determine what should be done about a given problem at execution

• Limit overhead of frequently used methods (e.g. operators, getters, setters of CMap2).
  • Use Result only in high-level constructs and/or what is likely a one-time function call.
  • Use Option in lower-level methods. This can change if benchmarks show the cost is too high.
• Crash if:
  • an operation does not make sense / the issue most likely results from a greater problem (e.g. both vertices of an edge aren't defined before splitting, or the user tries to access values of an attribute that's not part of the map)
  • a major problem occurs during some form of init (e.g. an input file not found)
• Throw a warning the problem detected won't create an issue / can be ignored in 99.9% of cases.

With these rules:

• We can use Result to enforce hypothesis wherever needed (e.g. algorithm input)
• Crashes happen either when continuing doesn't make sense (e.g. missing input) or when continuing would make the code explode (e.g. trying to use data that doesn't exist / isn't defined)
• Overhead seems acceptable for a first iteration. The usage of Options can be discussed after further benchmarks, though I suspect it will only cause problems in geometry-heavy meshing algorithms (e.g. vertex relaxation)

From there, we can:

• add those rules to the doc
• adjust the API to fit these rules

Additionally, I can use the thiserror crate to keep boilerplate to a minimum.

@cedricchevalier19 we can talk about this approach today, as well as #172