refactor(api)!: return fallible facet iterators#458
Merged
Conversation
- Make all-facet and boundary-facet traversal yield `Result<FacetView, FacetError>` so corrupted facet views and invalid boundary incidence are surfaced instead of skipped. - Route boundary-facet consumers, hull extraction, Euler counting, examples, benches, and prelude coverage through explicit item-error handling. - Use `SimplexKeyBuffer` for local repair and topology frontiers, and add a Semgrep guard for future raw `Vec<SimplexKey>` regressions. - Add a compact 2D-5D timing summary to `just perf-large-scale-smoke`. BREAKING CHANGE: facet iteration APIs now yield `Result<FacetView, FacetError>` items, `AllFacetsIter::try_new` is crate-private in favor of `Tds::facets()`, and `validate_ridge_links_for_simplices` now accepts `IntoIterator<Item = SimplexKey>`; slice callers should pass `.iter().copied()`.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesFallible Facet Iterators and SimplexKeyBuffer Migration
Perf Smoke Summary Table and Semgrep Rule
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Tds
participant AllFacetsIter
participant BoundaryFacetsIter
Caller->>Tds: facets() → Result<AllFacetsIter, TdsError>
Tds->>AllFacetsIter: try_new(simplex_key_iter())
AllFacetsIter-->>Caller: Ok(AllFacetsIter)
loop per simplex key
Caller->>AllFacetsIter: next()
alt facet-index overflow
AllFacetsIter-->>Caller: Some(Err(FacetError::InvalidFacetIndexOverflow))
else valid facet
AllFacetsIter-->>Caller: Some(Ok(FacetView))
end
end
Caller->>Tds: boundary_facets() → Result<BoundaryFacetsIter, TdsError>
Tds->>BoundaryFacetsIter: new(AllFacetsIter, facet_key_cache)
BoundaryFacetsIter-->>Caller: Ok(BoundaryFacetsIter)
loop per facet
Caller->>BoundaryFacetsIter: next()
BoundaryFacetsIter->>BoundaryFacetsIter: lookup facet key in cache
alt key missing from cache
BoundaryFacetsIter-->>Caller: Some(Err(FacetKeyNotFoundInCache))
else multiplicity ≠ 1
BoundaryFacetsIter-->>Caller: Some(Err(InvalidFacetMultiplicity))
else boundary facet
BoundaryFacetsIter-->>Caller: Some(Ok(FacetView))
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 27 |
🟢 Coverage 92.98% diff coverage · +0.08% coverage variation
Metric Results Coverage variation ✅ +0.08% coverage variation (-1.00%) Diff coverage ✅ 92.98% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (b41393e) 66469 60780 91.44% Head commit (893ec82) 66786 (+317) 61120 (+340) 91.52% (+0.08%) 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 (#458) 513 477 92.98% 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.
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/boundary.rs (1)
337-489: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a 5D case to the dimension-generic boundary-facet regression test.
This test is positioned as cross-dimensional coverage but currently stops at D=4. Please add a D=5 simplex case (or shared macro-driven case) so the Result-yielding facet path is validated through D=5 as required by repo policy.
As per coding guidelines, “Unit tests … dimension-generic tests MUST cover D=2 through D=5 whenever feasible using pastey macros.”
🤖 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/boundary.rs` around lines 337 - 489, Add a fifth test case for a 5D simplex to the test_boundary_facets_single_simplices function to achieve full dimension coverage through D=5 as required by repository policy. After Test Case 4 (empty triangulation), create a new test block that generates a 5D simplex using 6 points (the origin and 5 unit vectors along each axis), verify that the DelaunayTriangulation has 1 simplex and dimension 5, assert that boundary_facets returns exactly 6 boundary facets (facets of a 5D simplex), and verify all facets are confirmed as boundary facets using the is_boundary_facet_with_map method on the cached facet_to_simplices map, following the same validation pattern as the 4D test case.Source: Coding guidelines
🧹 Nitpick comments (2)
src/core/facet.rs (1)
1141-1149: 💤 Low valueDefensive skip when simplex not found from its own key iterator.
Lines 1147-1148 silently skip to the next simplex key when
tds.simplex(next_simplex_key)returnsNone. Sincesimplex_keysis derived from the TDS's own slotmap and the TDS is immutably borrowed, this path should be unreachable in a correctly-functioning system.If this path is hit, it would indicate internal inconsistency. Given the PR's goal of surfacing errors explicitly rather than silently skipping, consider whether this warrants a
FacetError::SimplexNotFoundInTriangulationerror instead of the silent skip.That said, this is defensive programming and the current behavior is safe—it just continues to the next key rather than panicking or returning stale data.
🤖 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/facet.rs` around lines 1141 - 1149, In the AllFacetsIterState transition logic, when the else branch is reached (where simplex_keys returns a key that does not exist in the TDS), replace the silent skip that sets state to AllFacetsIterState::PendingSimplex with explicit error handling. Instead, surface a FacetError::SimplexNotFoundInTriangulation error to indicate the internal inconsistency, aligning with the PR's goal of explicit error handling rather than silently skipping unreachable defensive paths.src/core/algorithms/incremental_insertion.rs (1)
3724-3729: ⚡ Quick winAdd coverage for the new 2D edge-split
InvalidPatchpaths.Both branches now classify edge-split ambiguity as
HullExtensionReason::InvalidPatch, which is retryable viaInsertionError::is_retryable(). Add focused tests for the bad boundary-count case and the “point on a hull vertex matches multiple boundary edges” case.As per coding guidelines, unit tests must cover known values and error paths.
Also applies to: 3935-3936
🤖 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/algorithms/incremental_insertion.rs` around lines 3724 - 3729, Add focused unit tests to cover the two new 2D edge-split error paths that now use HullExtensionReason::InvalidPatch. Create a test case for the bad boundary-count scenario that verifies when boundary_facets.len() does not equal 2, it properly returns InvalidPatch with appropriate details. Create another test case for the "point on hull vertex matches multiple boundary edges" scenario (also at line range 3935-3936). Both tests should verify that these InvalidPatch errors are properly retryable via InsertionError::is_retryable() and include the correct error details in the reason string.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 `@semgrep.yaml`:
- Around line 174-175: The pattern-regex for detecting frontier vector
allocations is incomplete because it only matches explicitly typed declarations
with Vec<SimplexKey> or turbofish syntax Vec::<SimplexKey>::new(), but misses
inferred type forms like let mut pending_seed_simplices = Vec::new() where the
type is inferred. Modify the pattern-regex to also match Vec::new() and
Vec::with_capacity() calls without type parameters when they are assigned to
variables matching the frontier vector naming patterns (pending_, repair_,
seed_, soft_fail_, touched_, affected_, frontier_, sample_, conflict_preview, or
new_simplices/removed_simplices). This can be done by adding an alternative
branch to the regex that matches these variable names followed by assignment to
a bare Vec::new() or Vec::with_capacity() call without requiring explicit type
annotation.
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 3762-3766: The total_boundary calculation in the
incremental_insertion.rs file is silently converting iterator failures to 0
using unwrap_or, which masks errors in the boundary-facet API. Instead of using
unwrap_or(0), propagate the Result returned by try_fold so that errors in the
boundary_facets iterator are not hidden. Change total_boundary to be a Result
type that can carry the error information, allowing the hull-extension logic to
properly detect and handle facet iterator failures rather than treating
corrupted iterators as empty boundaries.
---
Outside diff comments:
In `@src/core/boundary.rs`:
- Around line 337-489: Add a fifth test case for a 5D simplex to the
test_boundary_facets_single_simplices function to achieve full dimension
coverage through D=5 as required by repository policy. After Test Case 4 (empty
triangulation), create a new test block that generates a 5D simplex using 6
points (the origin and 5 unit vectors along each axis), verify that the
DelaunayTriangulation has 1 simplex and dimension 5, assert that boundary_facets
returns exactly 6 boundary facets (facets of a 5D simplex), and verify all
facets are confirmed as boundary facets using the is_boundary_facet_with_map
method on the cached facet_to_simplices map, following the same validation
pattern as the 4D test case.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 3724-3729: Add focused unit tests to cover the two new 2D
edge-split error paths that now use HullExtensionReason::InvalidPatch. Create a
test case for the bad boundary-count scenario that verifies when
boundary_facets.len() does not equal 2, it properly returns InvalidPatch with
appropriate details. Create another test case for the "point on hull vertex
matches multiple boundary edges" scenario (also at line range 3935-3936). Both
tests should verify that these InvalidPatch errors are properly retryable via
InsertionError::is_retryable() and include the correct error details in the
reason string.
In `@src/core/facet.rs`:
- Around line 1141-1149: In the AllFacetsIterState transition logic, when the
else branch is reached (where simplex_keys returns a key that does not exist in
the TDS), replace the silent skip that sets state to
AllFacetsIterState::PendingSimplex with explicit error handling. Instead,
surface a FacetError::SimplexNotFoundInTriangulation error to indicate the
internal inconsistency, aligning with the PR's goal of explicit error handling
rather than silently skipping unreachable defensive paths.
🪄 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: 35cdf3d6-d9af-46b4-a1c0-e8d9042c3e0a
📒 Files selected for processing (32)
benches/README.mdbenches/boundary_uuid_iter.rsdocs/dev/tooling-alignment.mdexamples/topology_editing.rsexamples/triangulation_and_hull.rsjustfilesemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/boundary.rssrc/core/facet.rssrc/core/insertion.rssrc/core/orientation.rssrc/core/query.rssrc/core/tds.rssrc/core/traits/boundary_analysis.rssrc/core/util/jaccard.rssrc/core/validation.rssrc/delaunay/construction.rssrc/delaunay/insertion.rssrc/delaunay/locality.rssrc/delaunay/query.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/util/measures.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/manifold.rstests/example_workflows.rstests/prelude_exports.rstests/semgrep/src/project_rules/rust_style.rstests/trait_bound_ergonomics.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #458 +/- ##
==========================================
+ Coverage 91.41% 91.49% +0.07%
==========================================
Files 72 72
Lines 66257 66574 +317
==========================================
+ Hits 60570 60910 +340
+ Misses 5687 5664 -23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Propagate boundary-facet iterator errors from hull-extension diagnostics instead of treating failed iteration as an empty boundary. - Report stale simplex keys from `AllFacetsIter` as `FacetError::SimplexNotFoundInTriangulation`. - Extend the local-frontier Semgrep guard to inferred `Vec::new` and `Vec::with_capacity` allocations.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/boundary.rs (1)
541-543: ⚡ Quick winUse tracing macro for committed test diagnostics
Line 541 keeps committed test diagnostics on
println!; switch this totracing::info!to align with the repo logging rule.Suggested patch
- println!( + tracing::info!( "✓ Single simplex boundary analysis works correctly in 2D, 3D, 4D, 5D, and empty cases" );As per coding guidelines,
**/*.rsrequirestracing::{debug,info,warn,error}!for committed diagnostics across tests/benchmarks.🤖 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/boundary.rs` around lines 541 - 543, Replace the println! macro call with tracing::info! macro to comply with the repository's logging guidelines for committed test diagnostics. The message string and its content can remain the same; only the logging mechanism needs to be changed from println! to tracing::info! for the test output at the single simplex boundary analysis validation.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.
Nitpick comments:
In `@src/core/boundary.rs`:
- Around line 541-543: Replace the println! macro call with tracing::info! macro
to comply with the repository's logging guidelines for committed test
diagnostics. The message string and its content can remain the same; only the
logging mechanism needs to be changed from println! to tracing::info! for the
test output at the single simplex boundary analysis validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b9201f40-72c1-422c-b485-e13f7cb4a3ea
📒 Files selected for processing (5)
semgrep.yamlsrc/core/algorithms/incremental_insertion.rssrc/core/boundary.rssrc/core/facet.rstests/semgrep/src/project_rules/rust_style.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- semgrep.yaml
- tests/semgrep/src/project_rules/rust_style.rs
- src/core/facet.rs
- src/core/algorithms/incremental_insertion.rs
- Allow dimension-generated incremental insertion properties to override their proptest case count. - Reduce the 5D incremental insertion validity property to a smaller default-suite sample while preserving active 5D coverage.
This was referenced Jun 16, 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.
Result<FacetView, FacetError>so corrupted facet views and invalid boundary incidence are surfaced instead of skipped.SimplexKeyBufferfor local repair and topology frontiers, and add a Semgrep guard for future rawVec<SimplexKey>regressions.just perf-large-scale-smoke.BREAKING CHANGE: facet iteration APIs now yield
Result<FacetView, FacetError>items,AllFacetsIter::try_newis crate-private in favor ofTds::facets(), andvalidate_ridge_links_for_simplicesnow acceptsIntoIterator<Item = SimplexKey>; slice callers should pass.iter().copied().