interior_point() panics on a polygon that check_validation() accepts, when coordinates contain -0.0

[PlenoraETL]

geo 0.33.1. A polygon containing a negative zero coordinate is reported as valid by check_validation(), but interior_point() then trips its own internal debug_assert! in the sweep line.

Minimal reproducer — only geo = "=0.33.1", with debug-assertions = true:

use geo::algorithm::{InteriorPoint, Validation};
use geo::{Coord, LineString, Polygon};

fn main() {
let ring = LineString(vec![
Coord { x: 5.0, y: 22.0 },
Coord { x: 2.0, y: 7.0 },
Coord { x: -0.0, y: 423.0 }, // negative zero
Coord { x: 0.0, y: 3.0 },
Coord { x: 9.0, y: 0.0 },
Coord { x: 5.0, y: 22.0 },
]);
let poly = Polygon::new(ring, vec![]);

assert!(poly.check_validation().is_ok()); // passes
let _ = poly.interior_point();            // panics

}

assertion failed: intervals_overlap(current_interval, overlapping_interval)
geo-0.33.1/src/algorithm/sweep/mod.rs:169

Replacing -0.0 with 0.0, leaving every other coordinate untouched, makes it return POINT(0.5084134615384616 211.5). The two polygons are numerically identical, since -0.0 == 0.0.

Root cause. src/algorithm/sweep/mod.rs mixes two incompatible orderings:

• line 266 sorts with total_cmp, and intervals_overlap (lines 277-280) compares with total_cmp — a total order, where -0.0 < +0.0;
• line 163 guards with the IEEE operator: if overlapping_interval.inserted_x > current_interval.deleted_x.

With deleted_x == -0.0 and inserted_x == 0.0 the IEEE guard is false, so the pair is not skipped, while intervals_overlap returns false because (-0.0).total_cmp(&0.0) is Less. The assertion then fails. Signed zero is precisely where the two orderings disagree on equal values.

Suggested fix. Make line 163 use the same ordering as the rest of the module:

if overlapping_interval.inserted_x.total_cmp(&current_interval.deleted_x).is_gt() {

Why it matters beyond the panic. It is a debug_assert!, so in release builds the check is compiled out and the sweep proceeds with a violated invariant — interior_point() can then return a wrong point silently. We only noticed because cargo-fuzz enables debug assertions.

Found by fuzzing a WKT parsing and geometry operations pipeline; the input was POLYGON((5 22, 2 7, -0 423, 0 3, 9 0, 5 22)).

[urschrei]

There's related work in #1554, which causes your example to pass. That having been said, a segment whose maximum is -0.0 (e.g. both endpoints at x ≤ -0.0) will still cause the panic in debug mode.

Also, I don't think this causes incorrectness in release mode: if a coordinate pair is "missed" it falls through to the full line_intersection check, which will do the right thing.

Your fix is also wrong. Both of these will have their intersection missed in release mode with the proposed change, which is worse than the current situation:

// direct sweep
Intersections::from_iter(vec![
    Line::from([(-1.0, 0.0), (-0.0, 0.0)]),
    Line::from([(0.0, 0.0), (1.0, 0.0)]),
]).collect::<Vec<_>>();

// via interior_point
Polygon::new(LineString(vec![
    Coord { x: -5.0, y: 0.0 }, Coord { x: -0.0, y: 5.0 },
    Coord { x: 0.0, y: 10.0 }, Coord { x: 5.0, y: 0.0 },
    Coord { x: -5.0, y: 0.0 },
]), vec![]).interior_point();

Fable says this approach (fixing the assert) is sound for non-NaN input:

fn intervals_overlap<C: Cross>(s0: &SweepLineInterval<C>, s1: &SweepLineInterval<C>) -> bool {
    s0.inserted_x <= s1.inserted_x && s0.deleted_x >= s1.inserted_x
}

Details 🤖

the sort's total order implies IEEE <= between successive inserted_x values (clause 1), and reaching the assert means the guard found inserted_x <= deleted_x under IEEE (clause 2). Signed zero is the only place total_cmp and IEEE disagree on ordering of finite values, and IEEE is the geometrically correct semantics – -0.0 and +0.0 are the same position. The roles are then clean: total_cmp where a total order is required (the sort, line 251's endpoint selection), IEEE everywhere position semantics are meant. The doc comment on intervals_overlap should be rewritten to say exactly that; its current claim that total_cmp handles "very close values" is wrong anyway.

We could also "just" normalise -0.0 to +0.0 (x + T::zero()) when constructing SweepLineInterval, but that feels a bit fragile.

[PlenoraETL]

Thanks — that's a useful correction on both counts.

On release-mode correctness: you're right, and my report was wrong. I verified it directly, with debug-assertions off:

con -0.0: Some(POINT(0.5084134615384616 211.5))
con +0.0: Some(POINT(0.5084134615384616 211.5))
identical: true

So the fallthrough to the full line_intersection check does the right thing, exactly as you say. The issue is a debug-mode panic, not a source of wrong results. I've retracted that claim downstream.

On my suggested fix: also wrong, and your two counterexamples make it obvious why — tightening the guard with total_cmp would make genuine intersections be missed in release, which is strictly worse than an over-eager debug assert. The intervals_overlap change you quote reads correct to me for the reason you give: total_cmp belongs where a total order is required (the sort, the endpoint selection), IEEE belongs where the comparison means position, and signed zero is the only place finite values disagree. Agreed too that the current doc comment on intervals_overlap describes something it doesn't do.

I'd share your reservation about normalising -0.0 at SweepLineInterval construction: it fixes the symptom at one entry point and leaves the ordering/semantics mismatch in place for the next one.

Happy to help verify: this surfaced from a fuzzing harness that runs interior_point alongside a battery of other operations over arbitrary WKT input. If it's useful, I can run #1554 against that corpus and report anything that still trips the assert — including the x ≤ -0.0 segment case you mention, which my original example doesn't cover.