Uh oh!
There was an error while loading. Please reload this page.
Fix NaN handling in LinearLocation/EdgeIntersection compareTo - #1229
Fix NaN handling in LinearLocation/EdgeIntersection compareTo#1229bjornharrtell wants to merge 1 commit into
Conversation
8c31be5 to
d328bf5ComparecompareTo() used raw </> comparisons on segmentFraction/dist, which are always false for NaN and made compareTo (and thus equals(), added in locationtech#1201) fall through to 0 whenever NaN was compared to any other value on the same segment/index. This broke transitivity of equals() and its consistency with hashCode(). Use Double.compare() instead, matching Double's NaN ordering semantics already used by hashCode().
d328bf5 to
ff7abe9Compare
grootstebozewolf
left a comment
There was a problem hiding this comment.
LGTM. Double.compare is the right fix for the #1201equals / compareTo / hashCode contract on NaN. Nits below are optional follow-ups, not blockers.
There was a problem hiding this comment.
Same < / > fall-through still lives in compareLocationValues (instance and static). LocationIndexOfPoint.indexOfFromStart uses that for “strictly after minIndex”, so a NaN fraction still compares equal to every fraction on that segment. Same one-liner: return Double.compare(...).
| // same location | ||
| return 0; | ||
| // Double.compare is used to ensure consistency with equals/hashCode for NaN | ||
| return Double.compare(segmentFraction, other.segmentFraction); |
There was a problem hiding this comment.
Finite-vs-finite order is unchanged except signed zero: Double.compare(-0.0, +0.0) == -1, while the old path returned 0. -0.0 survives normalize() (-0.0 < 0.0 is false), and Objects.hash already hashed them differently, so this also closes an equals/hashCode hole. Worth a sentence in the PR; not a reason to revert.
| if (this.dist > dist) return 1; | ||
| return 0; | ||
| // Double.compare is used to ensure consistency with equals/hashCode for NaN | ||
| return Double.compare(this.dist, dist); |
There was a problem hiding this comment.
Same signed-zero note as on LinearLocation: Double.compare distinguishes -0.0 and +0.0. EdgeIntersectionList is a TreeMap keyed on this order, so the NaN collapse was not only a HashSet issue.
| public void testEqualsHashCodeConsistentWithNaN() throws Exception | ||
| { | ||
| LinearLocation zero = new LinearLocation(0, 0, 0.0); | ||
| LinearLocation nan = new LinearLocation(0, 0, Double.NaN); | ||
| LinearLocation half = new LinearLocation(0, 0, 0.5); | ||
| // NaN must only be equal to itself, not to arbitrary other values (transitivity) | ||
| assertFalse(zero.equals(nan)); | ||
| assertFalse(nan.equals(half)); | ||
| assertFalse(zero.equals(half)); | ||
| assertEquals(nan, new LinearLocation(0, 0, Double.NaN)); | ||
| // equals/hashCode contract must hold even when NaN is involved | ||
| assertEquals(nan.hashCode(), new LinearLocation(0, 0, Double.NaN).hashCode()); |
There was a problem hiding this comment.
assertFalse(zero.equals(half)) is not a NaN case. Stronger: put zero and nan in the same HashSet / TreeSet (that was the collection failure), and assertTrue(nan.compareTo(zero) > 0) for the Double.compare total order.
Also, LineSegment.segmentFraction already maps NaN to 1.0, then normalize() walks that to the next vertex — that is why testZeroLengthLineString still compares against (1, 0.0). The public constructors still accept NaN, so the contract fix stands; indexOf on a collapsed segment does not store NaN.
| public void testEqualsHashCodeConsistentWithNaN() { | ||
| EdgeIntersection zero = new EdgeIntersection(new Coordinate(1, 2), 0, 0.0); | ||
| EdgeIntersection nan = new EdgeIntersection(new Coordinate(1, 2), 0, Double.NaN); | ||
| EdgeIntersection half = new EdgeIntersection(new Coordinate(1, 2), 0, 0.5); | ||
| // NaN must only be equal to itself, not to arbitrary other values (transitivity) | ||
| assertFalse(zero.equals(nan)); | ||
| assertFalse(nan.equals(half)); | ||
| assertFalse(zero.equals(half)); | ||
| assertEquals(nan, new EdgeIntersection(new Coordinate(1, 2), 0, Double.NaN)); | ||
| // equals/hashCode contract must hold even when NaN is involved | ||
| assertEquals(nan.hashCode(), new EdgeIntersection(new Coordinate(1, 2), 0, Double.NaN).hashCode()); | ||
| } |
There was a problem hiding this comment.
Same as the LinearLocation twin: assertFalse(zero.equals(half)) is not a NaN case. Worth putting zero and nan in one HashSet / TreeSet, and asserting nan.compareTo(zero) > 0.
Fixes#1228
LinearLocation.compareTo()andEdgeIntersection.compare()compared the floating-point fields (segmentFraction/dist) with raw</>, which are alwaysfalseforNaN. This madecompareTofall through toreturn 0("equal") whenever aNaNwas compared against any other value on the same segment/index, not just against anotherNaN.Since
equals()for both classes (added in #1201) is defined ascompareTo(o) == 0, this madeequals()non-transitive and inconsistent withhashCode()wheneverNaNwas involved (e.g.segmentFractionisNaNfor zero-length line segments).Fix
Replace the manual
</>comparisons withDouble.compare(), which givesNaNa well-defined, transitive ordering consistent withDouble.hashCode()/Objects.hash()(already used by both classes'hashCode()). No behavior change for non-NaNvalues.Tests
Added regression tests reproducing the transitivity/hashCode issue from the report:
LinearLocationTest.testEqualsHashCodeConsistentWithNaNEdgeIntersectionTest.testEqualsHashCodeConsistentWithNaN