Merge pull request #1505 from hannobraun/validate

Don't stop on first validation error

crates/fj-kernel/src/objects/object.rs

@@ -66,14 +66,12 @@ macro_rules! object {
             }
 
             /// Validate the object
-            pub fn validate(&self) -> Result<(), ValidationError> {
+            pub fn validate(&self, errors: &mut Vec<ValidationError>) {
                 match self {
                     $(
-                        Self::$ty((_, object)) => object.validate()?,
+                        Self::$ty((_, object)) => object.validate(errors),
                     )*
                 }
-
-                Ok(())
             }
         }
 

crates/fj-kernel/src/services/validation.rs

@@ -37,9 +37,12 @@ impl State for Validation {
     type Event = ValidationFailed;
 
     fn decide(&self, command: Self::Command, events: &mut Vec<Self::Event>) {
-        if let Err(err) = command.object.validate() {
+        let mut errors = Vec::new();
+        command.object.validate(&mut errors);
+
+        for err in errors {
             events.push(ValidationFailed {
-                object: command.object.into(),
+                object: command.object.clone().into(),
                 err,
             });
         }

crates/fj-kernel/src/validate/curve.rs

@@ -1,27 +1,21 @@
-use std::convert::Infallible;
-
 use crate::objects::{Curve, GlobalCurve};
 
-use super::{Validate, ValidationConfig};
+use super::{Validate, ValidationConfig, ValidationError};
 
 impl Validate for Curve {
-    type Error = Infallible;
-
     fn validate_with_config(
         &self,
         _: &ValidationConfig,
-    ) -> Result<(), Self::Error> {
-        Ok(())
+        _: &mut Vec<ValidationError>,
+    ) {
     }
 }
 
 impl Validate for GlobalCurve {
-    type Error = Infallible;
-
     fn validate_with_config(
         &self,
         _: &ValidationConfig,
-    ) -> Result<(), Self::Error> {
-        Ok(())
+        _: &mut Vec<ValidationError>,
+    ) {
     }
 }

crates/fj-kernel/src/validate/cycle.rs

@@ -5,22 +5,19 @@ use crate::{
     storage::Handle,
 };
 
-use super::{Validate, ValidationConfig};
+use super::{Validate, ValidationConfig, ValidationError};
 
 impl Validate for Cycle {
-    type Error = CycleValidationError;
-
     fn validate_with_config(
         &self,
         _: &ValidationConfig,
-    ) -> Result<(), Self::Error> {
-        CycleValidationError::check_half_edge_connections(self)?;
+        errors: &mut Vec<ValidationError>,
+    ) {
+        CycleValidationError::check_half_edge_connections(self, errors);
 
         // We don't need to check that all half-edges are defined in the same
         // surface. We already check that they are connected by identical
         // surface vertices, so that would be redundant.
-
-        Ok(())
     }
 }
 
@@ -43,7 +40,10 @@ pub enum CycleValidationError {
 }
 
 impl CycleValidationError {
-    fn check_half_edge_connections(cycle: &Cycle) -> Result<(), Self> {
+    fn check_half_edge_connections(
+        cycle: &Cycle,
+        errors: &mut Vec<ValidationError>,
+    ) {
         for (a, b) in cycle.half_edges().circular_tuple_windows() {
             let [_, prev] = a.vertices();
             let [next, _] = b.vertices();
@@ -52,14 +52,15 @@ impl CycleValidationError {
             let next = next.surface_form();
 
             if prev.id() != next.id() {
-                return Err(Self::HalfEdgeConnection {
-                    prev: prev.clone(),
-                    next: next.clone(),
-                });
+                errors.push(
+                    Self::HalfEdgeConnection {
+                        prev: prev.clone(),
+                        next: next.clone(),
+                    }
+                    .into(),
+                );
             }
         }
-
-        Ok(())
     }
 }
 
@@ -109,8 +110,8 @@ mod tests {
             Cycle::new(half_edges)
         };
 
-        valid.validate()?;
-        assert!(invalid.validate().is_err());
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
 
         Ok(())
     }

crates/fj-kernel/src/validate/edge.rs

@@ -1,5 +1,3 @@
-use std::convert::Infallible;
-
 use fj_interop::ext::ArrayExt;
 use fj_math::{Point, Scalar};
 
@@ -11,36 +9,31 @@ use crate::{
     storage::Handle,
 };
 
-use super::{Validate, ValidationConfig};
+use super::{Validate, ValidationConfig, ValidationError};
 
 impl Validate for HalfEdge {
-    type Error = HalfEdgeValidationError;
-
     fn validate_with_config(
         &self,
         config: &ValidationConfig,
-    ) -> Result<(), Self::Error> {
-        HalfEdgeValidationError::check_curve_identity(self)?;
-        HalfEdgeValidationError::check_global_curve_identity(self)?;
-        HalfEdgeValidationError::check_global_vertex_identity(self)?;
-        HalfEdgeValidationError::check_vertex_positions(self, config)?;
+        errors: &mut Vec<ValidationError>,
+    ) {
+        HalfEdgeValidationError::check_curve_identity(self, errors);
+        HalfEdgeValidationError::check_global_curve_identity(self, errors);
+        HalfEdgeValidationError::check_global_vertex_identity(self, errors);
+        HalfEdgeValidationError::check_vertex_positions(self, config, errors);
 
         // We don't need to check anything about surfaces here. We already check
         // curves, which makes sure the vertices are consistent with each other,
         // and the validation of those vertices checks the surfaces.
-
-        Ok(())
     }
 }
 
 impl Validate for GlobalEdge {
-    type Error = Infallible;
-
     fn validate_with_config(
         &self,
         _: &ValidationConfig,
-    ) -> Result<(), Self::Error> {
-        Ok(())
+        _: &mut Vec<ValidationError>,
+    ) {
     }
 }
 
@@ -128,38 +121,49 @@ pub enum HalfEdgeValidationError {
 }
 
 impl HalfEdgeValidationError {
-    fn check_curve_identity(half_edge: &HalfEdge) -> Result<(), Self> {
+    fn check_curve_identity(
+        half_edge: &HalfEdge,
+        errors: &mut Vec<ValidationError>,
+    ) {
         let back_curve = half_edge.back().curve();
         let front_curve = half_edge.front().curve();
 
         if back_curve.id() != front_curve.id() {
-            return Err(Self::CurveMismatch {
-                back_curve: back_curve.clone(),
-                front_curve: front_curve.clone(),
-                half_edge: half_edge.clone(),
-            });
+            errors.push(
+                Self::CurveMismatch {
+                    back_curve: back_curve.clone(),
+                    front_curve: front_curve.clone(),
+                    half_edge: half_edge.clone(),
+                }
+                .into(),
+            );
         }
-
-        Ok(())
     }
 
-    fn check_global_curve_identity(half_edge: &HalfEdge) -> Result<(), Self> {
+    fn check_global_curve_identity(
+        half_edge: &HalfEdge,
+        errors: &mut Vec<ValidationError>,
+    ) {
         let global_curve_from_curve = half_edge.curve().global_form();
         let global_curve_from_global_form = half_edge.global_form().curve();
 
         if global_curve_from_curve.id() != global_curve_from_global_form.id() {
-            return Err(Self::GlobalCurveMismatch {
-                global_curve_from_curve: global_curve_from_curve.clone(),
-                global_curve_from_global_form: global_curve_from_global_form
-                    .clone(),
-                half_edge: half_edge.clone(),
-            });
+            errors.push(
+                Self::GlobalCurveMismatch {
+                    global_curve_from_curve: global_curve_from_curve.clone(),
+                    global_curve_from_global_form:
+                        global_curve_from_global_form.clone(),
+                    half_edge: half_edge.clone(),
+                }
+                .into(),
+            );
         }
-
-        Ok(())
     }
 
-    fn check_global_vertex_identity(half_edge: &HalfEdge) -> Result<(), Self> {
+    fn check_global_vertex_identity(
+        half_edge: &HalfEdge,
+        errors: &mut Vec<ValidationError>,
+    ) {
         let global_vertices_from_vertices = {
             let (global_vertices_from_vertices, _) =
                 VerticesInNormalizedOrder::new(
@@ -183,35 +187,38 @@ impl HalfEdgeValidationError {
             .map(|global_vertex| global_vertex.id());
 
         if ids_from_vertices != ids_from_global_form {
-            return Err(Self::GlobalVertexMismatch {
-                global_vertices_from_vertices,
-                global_vertices_from_global_form,
-                half_edge: half_edge.clone(),
-            });
+            errors.push(
+                Self::GlobalVertexMismatch {
+                    global_vertices_from_vertices,
+                    global_vertices_from_global_form,
+                    half_edge: half_edge.clone(),
+                }
+                .into(),
+            );
         }
-
-        Ok(())
     }
 
     fn check_vertex_positions(
         half_edge: &HalfEdge,
         config: &ValidationConfig,
-    ) -> Result<(), Self> {
+        errors: &mut Vec<ValidationError>,
+    ) {
         let back_position = half_edge.back().position();
         let front_position = half_edge.front().position();
 
         let distance = (back_position - front_position).magnitude();
 
         if distance < config.distinct_min_distance {
-            return Err(Self::VerticesAreCoincident {
-                back_position,
-                front_position,
-                distance,
-                half_edge: half_edge.clone(),
-            });
+            errors.push(
+                Self::VerticesAreCoincident {
+                    back_position,
+                    front_position,
+                    distance,
+                    half_edge: half_edge.clone(),
+                }
+                .into(),
+            );
         }
-
-        Ok(())
     }
 }
 
@@ -254,8 +261,8 @@ mod tests {
             HalfEdge::new(vertices, valid.global_form().clone())
         };
 
-        valid.validate()?;
-        assert!(invalid.validate().is_err());
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
 
         Ok(())
     }
@@ -280,8 +287,8 @@ mod tests {
             tmp.build(&mut services.objects)
         });
 
-        valid.validate()?;
-        assert!(invalid.validate().is_err());
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
 
         Ok(())
     }
@@ -312,8 +319,8 @@ mod tests {
             tmp.build(&mut services.objects)
         });
 
-        valid.validate()?;
-        assert!(invalid.validate().is_err());
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
 
         Ok(())
     }
@@ -349,8 +356,8 @@ mod tests {
             valid.global_form().clone(),
         );
 
-        valid.validate()?;
-        assert!(invalid.validate().is_err());
+        valid.validate_and_return_first_error()?;
+        assert!(invalid.validate_and_return_first_error().is_err());
 
         Ok(())
     }