refactor(api)!: normalize fallible constructors (#459)#464
Merged
Conversation
- Rename fallible Delaunay triangulation and convex hull constructors to explicit `try_*` forms while keeping infallible constructors for validated inputs. - Remove the raw-array validated-coordinate constructor path in favor of `Point::try_new` and validated coordinate proofs. - Update documentation, examples, benchmarks, and Semgrep guardrails for the constructor naming contract and panic-free public snippets. - Report generated simplex counts in the large-scale smoke benchmark output. BREAKING CHANGE: Fallible public constructors now use explicit `try_*` names. Use `DelaunayTriangulation::try_new*`, `DelaunayTriangulation::try_with_*`, `ConvexHull::try_from_triangulation`, and `Point::try_new` or validated `Point` values instead of the previous fallible `DelaunayTriangulation::new*/with_*` constructors, `ConvexHull::from_triangulation`, and `Point::from_validated_coords` raw-array path. Closes #459
Contributor
WalkthroughThe PR normalizes constructor naming and fallible construction flows across core geometry, Delaunay APIs, docs, tests, benchmarks, and tooling. It also adds validated-point helpers, fallible random-point generation, new Semgrep rules, and a fixture-config generation script. ChangesConstructor normalization and propagation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 85 |
🟢 Coverage 99.22% diff coverage · +0.07% coverage variation
Metric Results Coverage variation ✅ +0.07% coverage variation (-1.00%) Diff coverage ✅ 99.22% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (bda1cd5) 67983 62288 91.62% Head commit (c55a8b3) 68283 (+300) 62613 (+325) 91.70% (+0.07%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#464) 2572 2552 99.22% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #464 +/- ##
==========================================
+ Coverage 91.59% 91.67% +0.07%
==========================================
Files 72 72
Lines 67771 68063 +292
==========================================
+ Hits 62078 62395 +317
+ Misses 5693 5668 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
benches/profiling_suite.rs (1)
839-843:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast when the benchmark setup cannot build a triangulation.
if let Ok(dt)turns a construction failure into a no-op sample, so the allocation run can silently measure the wrong path and skew the numbers. Abort or skip the entry here the same way the other suites do.🛠️ Proposed fix
- let alloc_info = measure(|| { - if let Ok(dt) = DelaunayTriangulationBuilder::new(&vertices).build::<()>() { - black_box(dt); - } - }); + let alloc_info = measure(|| { + let dt = bench_result( + DelaunayTriangulationBuilder::new(&vertices).build::<()>(), + "failed to build triangulation for memory benchmark", + ); + black_box(dt); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benches/profiling_suite.rs` around lines 839 - 843, The DelaunayTriangulationBuilder::new(&vertices).build::<()>() call in the allocation measurement silently succeeds or fails without asserting on success, which causes benchmark samples to measure different code paths when construction fails. Remove the if let Ok guard and instead use a pattern that panics or early-returns on failure (similar to how other benchmark suites in the file handle this), ensuring the benchmark only measures the successful triangulation path.docs/diagnostics.md (1)
125-138:⚠️ Potential issue | 🟡 MinorAdd coordinate-conversion error variant to the example enum.
Vertex::try_new(...)?on lines 135–138 returnsCoordinateConversionError, butDiagnosticsExampleErroronly maps construction and validation errors. The example will not compile as written.Suggested fix
use delaunay::prelude::DelaunayValidationError; +use delaunay::prelude::geometry::CoordinateConversionError; #[derive(Debug, thiserror::Error)] enum DiagnosticsExampleError { + #[error(transparent)] + Coordinate(#[from] CoordinateConversionError), #[error(transparent)] Construction(#[from] DelaunayTriangulationConstructionError), #[error(transparent)] Validation(#[from] DelaunayValidationError), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/diagnostics.md` around lines 125 - 138, The DiagnosticsExampleError enum is missing a variant to handle CoordinateConversionError. Since Vertex::try_new(...) returns a CoordinateConversionError, the ? operator on lines 135-138 will fail to convert the error to DiagnosticsExampleError. Add a new variant to the DiagnosticsExampleError enum with #[error(transparent)] and #[from] attributes to handle CoordinateConversionError, similar to the existing Construction and Validation variants.src/geometry/point.rs (1)
369-374:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument signed-zero canonicalization in the
TryFrom<[T; D]>contract.
TryFrom<[T; D]>now normalizes-0.0to0.0(Line 415), but that behavior is not explicitly stated in the conversion doc block. Please add a short note there so sign normalization is explicit at the API boundary.As per coding guidelines: “No f64 operation may silently lose sign information …”.
Also applies to: 415-419
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/geometry/point.rs` around lines 369 - 374, The TryFrom<[T; D]> implementation for Point normalizes -0.0 to 0.0 during conversion at line 415-419, but this behavior is not documented in the docstring of the impl block starting at line 369. Add a note to the docstring explicitly documenting the signed-zero canonicalization behavior to make it clear at the API boundary that -0.0 values will be converted to 0.0 as part of the fallible conversion process.Source: Coding guidelines
src/core/tds.rs (1)
77-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the validation table to the fallible point constructor.
This row still points users to
Point::from()for coordinate validation, which conflicts with the newtry_new/try_fromparse boundary used throughout the updated examples. Use the fallible point constructor names here too.Suggested documentation update
-//! | **Vertex Validity** | `Point::from()` (coordinates) + UUID auto-gen + `vertex.is_valid()` | Construction + runtime validation | +//! | **Vertex Validity** | `Point::try_new()` / `Point::try_from()` (coordinates) + UUID auto-gen + `vertex.is_valid()` | Construction + runtime validation |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tds.rs` at line 77, The documentation table row for "Vertex Validity" still references the infallible Point::from() constructor for coordinate validation, but the updated examples throughout the codebase now use fallible constructors like try_new or try_from at the parse boundary. Update this table row to replace the Point::from() reference with the appropriate fallible constructor names (try_new or try_from) to maintain consistency with the new validation pattern.src/delaunay/construction.rs (1)
3907-3928:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate after entering the
Constructedstate.
finalize_bulk_constructionvalidates while the TDS is still in its prior construction state, then marks itConstructedon Line 3928. The shuffled retry paths return afteris_delaunay_via_flips()and do not run a fullis_valid()after this assignment, so any validation gated onTriangulationConstructionState::Constructedcan be skipped.Proposed fix
+ let prior_construction_state = self.tri.tds.construction_state().clone(); + self.tri.tds.construction_state = TriangulationConstructionState::Constructed; + tracing::debug!("post-construction: starting topology validation (finalize)"); let validation_started = Instant::now(); let validation_result = self.tri.validate(); @@ if let Err(err) = validation_result { + self.tri.tds.construction_state = prior_construction_state; return Err(TriangulationConstructionError::FinalTopologyValidation { message: "topology validation failed after construction".to_string(), source: err.into(), } .into()); } - self.tri.tds.construction_state = TriangulationConstructionState::Constructed; Ok(())As per coding guidelines, “Every mutating operation must preserve invariants checked by
Tds::is_valid(Level 1-3) andDelaunayTriangulation::is_valid(Level 4).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/delaunay/construction.rs` around lines 3907 - 3928, The topology validation in the finalize_bulk_construction method is currently performed before setting the construction_state to Constructed on line 3928, but validation should occur after entering the Constructed state to ensure invariants are checked in the final state. Move the line that sets self.tri.tds.construction_state to TriangulationConstructionState::Constructed to execute before the validation_result assignment and validation logic, so that any validation checks that depend on TriangulationConstructionState::Constructed will execute properly and all invariants will be validated in the final constructed state.Source: Coding guidelines
🧹 Nitpick comments (1)
benches/profiling_suite.rs (1)
92-113: 💤 Low valueDocument the new private benchmark helpers.
finite_point,retry_attempts,coordinate_range,wide_bounds,adversarial_bounds, andgenerated_points_in_rangeare private helpers with no///context. That makes the setup code harder to maintain.As per coding guidelines, private functions must have a brief doc comment explaining why they exist.
Also applies to: 307-316
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benches/profiling_suite.rs` around lines 92 - 113, The private helper functions `finite_point`, `retry_attempts`, `coordinate_range`, `wide_bounds`, and `adversarial_bounds` in benches/profiling_suite.rs at lines 92-113 (anchor) are missing documentation comments. Additionally, the `generated_points_in_range` function at lines 307-316 (sibling) also lacks documentation. Add brief `///` doc comments to each of these private functions explaining their purpose and why they exist, following the coding guidelines that require all private functions to have documentation context. This will improve maintainability of the benchmark setup code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/testing.md`:
- Around line 277-279: The code example in the testing documentation references
QueryError::MissingSimplex which does not actually exist in src/core/query.rs,
making the example non-compilable. Replace the reference to
QueryError::MissingSimplex with a locally-defined error type that is specific to
the example code block. Define a simple custom error enum within the example
itself (for instance, a local enum with a MissingSimplex variant) and use that
in the else block instead of relying on the non-existent QueryError variant.
This makes the example self-contained and demonstrable without requiring
external crate errors that don't exist.
In `@docs/validation.md`:
- Around line 112-115: The example code uses the ? operator on
Vertex::try_new(...) calls which return CoordinateConversionError, but the
ValidationExampleError enum does not implement From<CoordinateConversionError>,
causing the example to fail compilation. Add a From<CoordinateConversionError>
implementation to the ValidationExampleError enum (or add a variant that wraps
CoordinateConversionError if using an enum-based approach) so that the ?
operator can properly convert the error type when calling try_new() at the
specified lines.
In `@src/delaunay/construction.rs`:
- Around line 6576-6592: The test function
test_constructed_delaunay_exposes_constructed_tds_state is currently hardcoded
to test only D=3 (as seen in the Vertex<(), 3> and DelaunayTriangulation<_, (),
(), 3> declarations). To comply with coding guidelines, refactor this test to be
dimension-generic by creating a pastey macro that generates the same test cases
for D=2 through D=5, using standard-simplex fixtures for each dimension. Apply
this same pattern to all other new constructor/finalization/statistics test
functions in the same file that currently only cover D=3.
In `@src/geometry/kernel.rs`:
- Around line 666-669: The `expect("finite point coordinates")` calls in the
Point creation code will panic on user input in library runtime paths, violating
the coding guidelines that forbid panics on user input in library code. Replace
these `expect()` calls with proper error propagation using the `?` operator or
match expressions to return the `CoordinateConversionError` to the caller
instead. This issue occurs at multiple locations in the file (the primary
location and additional occurrences as noted in the comment), and each should be
fixed by removing the `expect()` and allowing the error to propagate through the
Result return type of the containing function.
In `@src/geometry/util/point_generation.rs`:
- Around line 151-156: Add a focused unit test to exercise the error path where
`Point::try_new` fails due to coordinate validation and maps to the new
`GeneratedPointCoordinateRejected` error variant. The test should verify that
when invalid point coordinates are provided, the error is properly caught and
the `GeneratedPointCoordinateRejected` variant is returned with the correct
`CoordinateValidationError` source. Include test cases that cover known invalid
coordinate values and verify dimension-generic correctness to prevent regression
of this boundary condition.
- Around line 307-315: The `random_range` method can panic when calculating
`high - low` overflows for floats, even though `CoordinateRange` validates
bounds are finite and min < max. Add an explicit overflow check in
`sample_point_in_range` before the call to
`rng.random_range(range.min()..range.max())` to detect when `max - min` would
overflow, and map the failure into `RandomPointGenerationError` instead of
allowing a panic. Apply the same overflow check to the ball-sampling code path
that also calls `random_range`. Additionally, add a test case that exercises the
`GeneratedPointCoordinateRejected` error variant to verify the error contract is
reachable.
In `@tests/proptest_convex_hull.rs`:
- Around line 56-60: The proptest cases are silently passing when constructor or
method calls fail by using if let Ok(...) patterns, which weakens invariant
coverage. For deterministic simplex cases at tests/proptest_convex_hull.rs lines
56-60 and 91-95, replace the if let Ok checks with prop_assert! calls to ensure
DelaunayTriangulation::try_new_with_topology_guarantee and
ConvexHull::try_from_triangulation succeed deterministically. For randomized
domain cases at lines 136-141, 187-196, 205-206, 246-251, and 288-294, use
prop_assume!(result.is_ok()) followed by unwrap() before assertions so that
invalid inputs are rejected rather than silently counted as passing test runs,
ensuring only valid invariant checks are counted toward test coverage.
---
Outside diff comments:
In `@benches/profiling_suite.rs`:
- Around line 839-843: The
DelaunayTriangulationBuilder::new(&vertices).build::<()>() call in the
allocation measurement silently succeeds or fails without asserting on success,
which causes benchmark samples to measure different code paths when construction
fails. Remove the if let Ok guard and instead use a pattern that panics or
early-returns on failure (similar to how other benchmark suites in the file
handle this), ensuring the benchmark only measures the successful triangulation
path.
In `@docs/diagnostics.md`:
- Around line 125-138: The DiagnosticsExampleError enum is missing a variant to
handle CoordinateConversionError. Since Vertex::try_new(...) returns a
CoordinateConversionError, the ? operator on lines 135-138 will fail to convert
the error to DiagnosticsExampleError. Add a new variant to the
DiagnosticsExampleError enum with #[error(transparent)] and #[from] attributes
to handle CoordinateConversionError, similar to the existing Construction and
Validation variants.
In `@src/core/tds.rs`:
- Line 77: The documentation table row for "Vertex Validity" still references
the infallible Point::from() constructor for coordinate validation, but the
updated examples throughout the codebase now use fallible constructors like
try_new or try_from at the parse boundary. Update this table row to replace the
Point::from() reference with the appropriate fallible constructor names (try_new
or try_from) to maintain consistency with the new validation pattern.
In `@src/delaunay/construction.rs`:
- Around line 3907-3928: The topology validation in the
finalize_bulk_construction method is currently performed before setting the
construction_state to Constructed on line 3928, but validation should occur
after entering the Constructed state to ensure invariants are checked in the
final state. Move the line that sets self.tri.tds.construction_state to
TriangulationConstructionState::Constructed to execute before the
validation_result assignment and validation logic, so that any validation checks
that depend on TriangulationConstructionState::Constructed will execute properly
and all invariants will be validated in the final constructed state.
In `@src/geometry/point.rs`:
- Around line 369-374: The TryFrom<[T; D]> implementation for Point normalizes
-0.0 to 0.0 during conversion at line 415-419, but this behavior is not
documented in the docstring of the impl block starting at line 369. Add a note
to the docstring explicitly documenting the signed-zero canonicalization
behavior to make it clear at the API boundary that -0.0 values will be converted
to 0.0 as part of the fallible conversion process.
---
Nitpick comments:
In `@benches/profiling_suite.rs`:
- Around line 92-113: The private helper functions `finite_point`,
`retry_attempts`, `coordinate_range`, `wide_bounds`, and `adversarial_bounds` in
benches/profiling_suite.rs at lines 92-113 (anchor) are missing documentation
comments. Additionally, the `generated_points_in_range` function at lines
307-316 (sibling) also lacks documentation. Add brief `///` doc comments to each
of these private functions explaining their purpose and why they exist,
following the coding guidelines that require all private functions to have
documentation context. This will improve maintainability of the benchmark setup
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 74e82b52-9e97-4dac-8c44-0c53e1acdb81
📒 Files selected for processing (118)
README.mdbenches/PERFORMANCE_RESULTS.mdbenches/README.mdbenches/allocation_hot_paths.rsbenches/boundary_uuid_iter.rsbenches/ci_performance_suite.rsbenches/circumsphere_containment.rsbenches/cold_path_predicates.rsbenches/common/flip_workflows.rsbenches/profiling_suite.rsbenches/remove_vertex.rsbenches/tds_clone.rsbenches/topology_guarantee_construction.rsdocs/api_design.mddocs/dev/rust.mddocs/dev/testing.mddocs/dev/tooling-alignment.mddocs/diagnostics.mddocs/numerical_robustness_guide.mddocs/production_review_remediation_checklist.mddocs/roadmap.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/numerical_robustness.rsexamples/triangulation_and_hull.rsjustfilescripts/semgrep_fixture_config.pyscripts/tests/test_semgrep_fixture_config.pysemgrep.yamlsrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/collections/aliases.rssrc/core/collections/key_maps.rssrc/core/collections/secondary_maps.rssrc/core/construction.rssrc/core/edge.rssrc/core/facet.rssrc/core/insertion.rssrc/core/query.rssrc/core/repair.rssrc/core/simplex.rssrc/core/tds.rssrc/core/tds_snapshot.rssrc/core/traits/boundary_analysis.rssrc/core/traits/facet_cache.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/deduplication.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/hilbert.rssrc/core/util/jaccard.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/locality.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/serialization.rssrc/delaunay/triangulation.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/point.rssrc/geometry/predicates.rssrc/geometry/quality.rssrc/geometry/robust_predicates.rssrc/geometry/sos.rssrc/geometry/traits/coordinate.rssrc/geometry/util/circumsphere.rssrc/geometry/util/conversions.rssrc/geometry/util/measures.rssrc/geometry/util/point_generation.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rstests/README.mdtests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/example_workflows.rstests/large_scale_debug.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_delaunay_triangulation.rstests/proptest_euler_characteristic.rstests/proptest_facet.rstests/proptest_flips.rstests/proptest_orientation.rstests/proptest_serialization.rstests/proptest_simplex.rstests/proptest_tds.rstests/proptest_triangulation.rstests/public_topology_api.rstests/regressions.rstests/semgrep/doctests/unwrap_expect.txttests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
- Replace public trusted vertex batch construction with try_vertices_from_points so point-to-vertex conversion re-enters the coordinate validation boundary. - Collapse trusted vertex construction onto one internal from_validated_point path that carries optional payload data. - Add repository guardrails that keep from_validated* constructors internal, scarce, and separate from public parsing APIs. - Harden random point generation against sampler range overflow with a typed CoordinateRangeWidthOverflow error. BREAKING CHANGE: Vertex::from_validated_points is no longer public. Callers converting Point values into vertices should use try_vertices_from_points and handle CoordinateConversionError.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
benches/profiling_suite.rs (1)
308-313: ⚡ Quick winPreserve vertex-conversion error context.
try_vertices_from_pointsis the fallible fixture conversion boundary; a rawstd::process::abort()makes failures untriageable. Route it throughbench_resultlike the other setup helpers.Suggested fix
fn benchmark_vertices_from_generated_points<const D: usize>( points: &[Point<D>], ) -> Vec<Vertex<(), D>> { - try_vertices_from_points(points).unwrap_or_else(|_| std::process::abort()) + bench_result( + try_vertices_from_points(points), + format!( + "failed to convert {} generated points into {D}D benchmark vertices", + points.len() + ), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benches/profiling_suite.rs` around lines 308 - 313, The `benchmark_vertices_from_generated_points` function uses a raw `std::process::abort()` when `try_vertices_from_points` fails, discarding error context and making failures hard to diagnose. Instead of `unwrap_or_else(|_| std::process::abort())`, route the error through `bench_result` like the other setup helper functions in the file do, so that vertex conversion failures are properly captured and reported with their error context preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/diagnostics.md`:
- Around line 123-133: The `DiagnosticsExampleError` enum is using the incorrect
error type for coordinate operations. Replace `CoordinateValidationError` with
`CoordinateConversionError` in both the import statement and the enum variant
definition. The `Vertex::try_new(...)` calls throw `CoordinateConversionError`,
so using the correct error type will ensure the `?` operator properly maps
errors to `DiagnosticsExampleError` variants without conversion issues.
---
Nitpick comments:
In `@benches/profiling_suite.rs`:
- Around line 308-313: The `benchmark_vertices_from_generated_points` function
uses a raw `std::process::abort()` when `try_vertices_from_points` fails,
discarding error context and making failures hard to diagnose. Instead of
`unwrap_or_else(|_| std::process::abort())`, route the error through
`bench_result` like the other setup helper functions in the file do, so that
vertex conversion failures are properly captured and reported with their error
context preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ce11f9c9-0b21-4206-be1f-d43f4b9e487a
📒 Files selected for processing (40)
benches/allocation_hot_paths.rsbenches/boundary_uuid_iter.rsbenches/ci_performance_suite.rsbenches/profiling_suite.rsbenches/remove_vertex.rsbenches/tds_clone.rsdocs/dev/rust.mddocs/dev/testing.mddocs/diagnostics.mddocs/validation.mdexamples/triangulation_and_hull.rssemgrep.yamlsrc/core/boundary.rssrc/core/insertion.rssrc/core/tds.rssrc/core/util/deduplication.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/query.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/point.rssrc/geometry/util/point_generation.rssrc/geometry/util/triangulation_generation.rssrc/lib.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_delaunay_triangulation.rstests/proptest_facet.rstests/proptest_flips.rstests/proptest_orientation.rstests/proptest_serialization.rstests/proptest_simplex.rstests/proptest_tds.rstests/proptest_triangulation.rstests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rs
✅ Files skipped from review due to trivial changes (1)
- docs/dev/rust.md
🚧 Files skipped from review as they are similar to previous changes (12)
- benches/allocation_hot_paths.rs
- examples/triangulation_and_hull.rs
- benches/tds_clone.rs
- docs/validation.md
- docs/dev/testing.md
- src/core/validation.rs
- src/core/insertion.rs
- src/delaunay/delaunayize.rs
- benches/ci_performance_suite.rs
- src/core/util/deduplication.rs
- src/delaunay/query.rs
- src/delaunay/construction.rs
- Route profiling benchmark vertex conversion failures through bench_result instead of aborting without the underlying error. - Update the diagnostics example to use CoordinateConversionError for Vertex::try_new failures.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/validation.md (1)
164-187:⚠️ Potential issue | 🔴 CriticalFix examples that use
Vertex::try_new()with incompatible error types.Code examples at lines 164-187, 315-344, 400-423, and 472-494 use
Vertex::<(), _>::try_new(...)?which returnsCoordinateConversionError, but the function signatures returnDelaunayTriangulationConstructionError. NoFrom<CoordinateConversionError>implementation exists onDelaunayTriangulationConstructionError, so the?operator will fail to compile.All other doc examples in the codebase handle this by wrapping both error types in a custom error enum with
#[from]attributes. Apply the same pattern to these validation.md examples, or change the return type toResult<(), CoordinateConversionError>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/validation.md` around lines 164 - 187, The example code blocks using Vertex::try_new() have a type mismatch error: Vertex::try_new() returns CoordinateConversionError but the function signatures return DelaunayTriangulationConstructionError, and no From implementation exists between these types so the ? operator will fail to compile. Fix all four affected example code blocks in validation.md (the main function shown and the other examples at the specified line ranges) by either wrapping both error types in a custom error enum with #[from] attributes (as other doc examples in the codebase do) or by changing the return type to Result<(), CoordinateConversionError>. Ensure the chosen approach is applied consistently across all affected examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/validation.md`:
- Around line 164-187: The example code blocks using Vertex::try_new() have a
type mismatch error: Vertex::try_new() returns CoordinateConversionError but the
function signatures return DelaunayTriangulationConstructionError, and no From
implementation exists between these types so the ? operator will fail to
compile. Fix all four affected example code blocks in validation.md (the main
function shown and the other examples at the specified line ranges) by either
wrapping both error types in a custom error enum with #[from] attributes (as
other doc examples in the codebase do) or by changing the return type to
Result<(), CoordinateConversionError>. Ensure the chosen approach is applied
consistently across all affected examples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 21374842-d872-4cba-8f35-8693e2051947
📒 Files selected for processing (1)
docs/validation.md
This was referenced Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
try_*forms while keeping infallible constructors for validated inputs.Point::try_newand validated coordinate proofs.BREAKING CHANGE: Fallible public constructors now use explicit
try_*names. UseDelaunayTriangulation::try_new*,DelaunayTriangulation::try_with_*,ConvexHull::try_from_triangulation, andPoint::try_newor validatedPointvalues instead of the previous fallibleDelaunayTriangulation::new*/with_*constructors,ConvexHull::from_triangulation, andPoint::from_validated_coordsraw-array path.Closes #459