Guard float overlay entry points against non-finite coordinates

[LucaCappelletti94]

Float boolean ops and mesh offsetters aborted on NaN or infinite input: the adapter's FloatRect inherited NaN bounds, and i_float's FloatPointAdapter::float_to_int debug assertion then fired on the first point converted. A downstream geo fuzz report against i_overlay 4.5.2 and i_float 1.16.0 hit this out of BooleanOps and Buffer.

Every public entry point that builds an adapter from user paths now filters non-finite points before FloatPointAdapter::with_iter (or the mesh FloatRect::with_iter), and every per-contour loop skips contours containing any non-finite point before touching float_to_int or unsafe_int_area. Both guards are needed: the first keeps the rect finite, the second keeps NaN out of the integer conversion. unsafe_add_contour stays ungated on purpose, its safety contract already excludes points outside the rect.

Eleven regression tests in tests/float_point_adapter.rs cover NaN, f64::INFINITY, f64::NEG_INFINITY, f32 NaN, mixed and degenerate contours, from_subj_and_clip combinations, and an empty contour. They pass under both debug (with the i_float adapter assertions active) and release alongside the 667 existing tests.

Also folds in a few unrelated clippy cleanups.

[NailxSharipov]

Thanks for the detailed fuzz report and for covering the failure modes with tests.

I agree that non-finite coordinates should not be able to reach FloatPointAdapter::float_to_int, but I don't think silently filtering them inside iOverlay is the right contract for the core API.

iOverlay is designed around valid geometric input. NaN, +inf, and -inf are not meaningful geometry coordinates: they break bounds computation, ordering, area checks, and float-to-int conversion assumptions. Dropping individual points or entire contours also changes the user geometry silently, which can hide upstream data issues and produce surprising topology.

I would prefer to handle this as an explicit input precondition:

• all public float APIs should document that input coordinates must be finite;
• debug builds should assert this precondition before building FloatPointAdapter or converting contours;
• release builds should keep the current fast path without extra per-point filtering cost;
• unsafe_add_contour should remain governed by its existing safety contract.

To make this clean across custom point types, I propose first adding an is_finite API to i_float:

pub trait FloatNumber {
    fn is_finite(self) -> bool;
}

pub trait FloatPointCompatible: Copy {
    type Scalar: FloatNumber;

    fn from_xy(x: Self::Scalar, y: Self::Scalar) -> Self;
    fn x(&self) -> Self::Scalar;
    fn y(&self) -> Self::Scalar;

    #[inline(always)]
    fn is_finite(&self) -> bool {
        self.x().is_finite() && self.y().is_finite()
    }
}

[michaelkirk]

For what it's worth, that seems reasonable to me.

[LucaCappelletti94]

The panic came from fuzzing an app that reaches i_overlay transitively via geo's BooleanOps and Buffer. Documenting the finite-input precondition on i_overlay helps geo, but does not propagate to every crate downstream. Each layer would need to duplicate the same guard, which is what this PR does once at the boundary.

The debug_assert-only proposal is also worse than the current abort. In release, <f64 as iN> saturates: NaN becomes 0, INFINITY becomes iN::MAX, and i_float's to_round_i32/to_round_i64 inherit that. Without the assertion, NaN silently lands on the adapter's offset and INFINITY lands on the far corner of the integer grid, and the sweep runs on corrupted geometry: no panic, no error, wrong result. That hides upstream data issues harder than filtering does. The debug abort itself is not much better in shape, aborting the caller's process on user input rather than surfacing a Result variant they can react to. Panics in Rust are for programmer-bug invariants, not for user-supplied floats.

I would rather make the checked constructors (from_subj, from_subj_and_clip, the scaled and predicate variants, the string overlay ones, and the mesh offsetters) return Result<Self, InvalidInput> and error on any non-finite input. The unsafe_add_* methods keep their caller-attested precondition. A separate cleanup could rename them to add_*_unchecked, which reads more truthfully than unsafe_ since the precondition is a logic invariant, not memory safety in the Rust sense. In that design, the is_finite addition on FloatPointCompatible you proposed is exactly what the checked constructors would call. Happy to rework in that direction if you agree.

[michaelkirk]

Could you please stop AI pumping your comments? I think it will be more productive to talk in your own voice rather than dumping paragraphs of inflated text.

[LucaCappelletti94]

Could you please stop AI pumping your comments? I think it will be more productive to talk in your own voice rather than dumping paragraphs of inflated text.

That is just me being precise. If you prefer an even shorter TLDR: methods that fail should return Result. Methods that are unchecked should be called unchecked, and checked by the caller. A prefix like unsafe should be reserved for actually memory meddling methods.

[NailxSharipov]

Sorry I do not want iOverlay to silently modify invalid geometry. NaN/inf coordinates are invalid input, not geometry that can be safely repaired.
Dropping points or contours changes the user's data and don't give a correct result because there is no correct result for invalid data.

But if you d' like to add some Shape<P> validation api I don't mind.
We could place it in iShape crate

[LucaCappelletti94]

Nothing forces geo (or any middle crate) to call validate, so the panic or in release mangled data still reaches the leaf app. from_subj -> Result<Self, InvalidInput> puts the failure into the type system, and geo could still possibly.unwrap(), but the choice is explicit and leads much more intuitively to just propagate the error upwards. Validation belongs at the layer that knows the failure mode, not on callers who cannot be omniscient about the transitive stack.

What I am working on is SqliteGIS, a pure-Rust reimplementation of PostGIS on top of SQLite and geo. It accepts arbitrary SQL where NaN or infinity can arise from operations on row data. Per the Rust Book:

returning Result is a good default choice when you're defining a function that might fail.

[NailxSharipov]

ok Result api could be useful in some rare case.

I do not want to change the existing fast constructors. For the main use case this is redundant overhead.

But I don't mind to add additional api with safe guarantees

Lets first add this api to iFloat and iShape

FloatNumber::is_finite()
FloatPointCompatible::is_finite() with the default impl for trait

and for iShape probably better place will be here:

enum InvalidGeometry {
    NonFinitePoint {
        path_index: usize,
        point_index: usize,
    },
}


pub trait ShapeResource<P>
where
    P: FloatPointCompatible,
{
    type ResourceIter<'a>: Iterator<Item = &'a [P]>
    where
        P: 'a,
        Self: 'a;

    fn iter_paths(&self) -> Self::ResourceIter<'_>;

    fn validate_finite_points(&self) -> Result<(), InvalidGeometry> {
        for (path_index, path) in self.iter_paths().enumerate() {
            for (point_index, p) in path.iter().enumerate() {
                if !p.is_finite() {
                    return Err(InvalidGeometry::NonFinitePoint {
                        path_index,
                        point_index,
                    });
                }
            }
        }
        Ok(())
    }
}

Do we need the detailed information about contour index?

and after we can duplicate some api with this exception for iOverlay