Skip to content

Use nil for empty slices so reflect.DeepEqual agrees with ExactEquals - #718

Draft
peterstace wants to merge 1 commit into
masterfrom
reflect_deep_equals_fixes_xcA
Draft

peterstace wants to merge 1 commit into
masterfrom
reflect_deep_equals_fixes_xcA

Conversation

@peterstace

Copy link
Copy Markdown
Owner

Description

The doc comment on Geometry says that reflect.DeepEqual gives the same result as ExactEquals called without options. That depends on an empty geometry, or an empty member of one, holding a nil slice. Several operations built an empty slice that isn't nil, so reflect.DeepEqual reported their results as different from geometries that ExactEquals says are equal.

Before #717, NewGeometryCollection called ForceCoordinatesType on every member, which replaced some of these slices with nil whenever a result went into a collection. #717 skips that call when the type already matches, which exposed more of these cases.

This PR makes 13 functions return a nil slice where they built an empty one:

  • Sequence.Reverse (reached through LineString.Reverse), Polygon.Reverse, MultiLineString.Reverse and MultiPolygon.Reverse.
  • Polygon.Densify, MultiLineString.Densify, MultiPolygon.Densify and GeometryCollection.Densify.
  • Polygon.forceOrientation, which MultiPolygon.ForceCW and ForceCCW call on every member, including empty ones.
  • GeometryCollection.TransformXY (also used by SnapToGrid and FlipCoordinates) and GeometryCollection.Transform.
  • GeometryCollection.ForceCoordinatesType, for an empty collection whose type changes. The WKT and WKB parsers build GEOMETRYCOLLECTION Z EMPTY (and the M and ZM forms) this way, so this also fixes parsing.
  • Sequence.Slice with equal indices. It still slices before checking the length, so out-of-range indices still panic.

The tests check the internal form of each result rather than comparing it with a parsed copy. The WKT and WKB parsers had the same problem, so a round trip through them would hide it. canonicalFormViolations walks a value through its unexported fields and reports any empty slice that isn't nil, any empty Point with non-zero coordinates, and any Geometry holding an empty XY GeometryCollection. TestMethodsReturnCanonicalForm uses reflection to find every method that returns a geometry on the geometry types, Sequence and Envelope. It calls each one on 164 inputs and checks every result. It fails if a method takes a parameter type it doesn't have arguments for, so a new method is covered as soon as it's added. TestFunctionsReturnCanonicalForm does the same for a list of package-level functions.

Check List

Have you:

  • Added unit tests? Yes. Before the fix, 38 method subtests and the WKT and WKB subtests of TestFunctionsReturnCanonicalForm fail. Undoing any one of the 13 changes makes the tests fail again.

  • Add cmprefimpl tests? (if appropriate?) Not appropriate. The change is to the internal representation, which the reference implementations can't see.

  • Updated release notes? (if appropriate?) Yes. The Unreleased section of CHANGELOG.md has a bug-fix entry. The geom: build nested GeometryCollections in linear time #717 entry doesn't mention the reflect.DeepEqual changes, since this PR fixes them before they're released.

  • Updated the README.md (if new functionality is added?) Not appropriate. There's no new functionality.

Related Issue

Benchmark Results

Not run. The change adds a length check, and only empty results take the new return.

The doc comment on Geometry says that reflect.DeepEqual gives the same
result as ExactEquals called without options. That depends on an empty
geometry, or an empty member of one, holding a nil slice. Some
operations built an empty slice that isn't nil instead:

- Reverse on Sequence (and so LineString), Polygon, MultiLineString and
  MultiPolygon.
- Densify on Polygon, MultiLineString, MultiPolygon and
  GeometryCollection.
- ForceCW and ForceCCW on a MultiPolygon with an empty member.
- TransformXY and Transform on an empty GeometryCollection, and
  ForceCoordinatesType when it changes the type of an empty
  GeometryCollection. The WKT and WKB parsers call ForceCoordinatesType
  for an empty Z, M or ZM collection, so parsing had the problem too.
- Sequence.Slice with equal indices.

Each of these now returns a nil slice where it built an empty one.

Add TestMethodsReturnCanonicalForm, which finds every method that
returns a geometry by reflection and checks the results through their
unexported fields, and TestFunctionsReturnCanonicalForm, which does the
same for a list of package-level functions.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain for the empty-slice normalization changes.

No open issues 🎉

What changed in this PR

This PR normalizes empty slices to nil across geometry operations, restoring the documented agreement between reflect.DeepEqual and ExactEquals. It addresses cases exposed by #717’s collection-copying optimization.

Changes:

  • Normalize empty results in 13 functions while preserving coordinate types and Sequence.Slice bounds checks.
  • Add reflection-based canonical-form checks for geometry-returning methods and selected package functions.
  • Document the observable equality fixes in the changelog.
File Description
geom/​type_sequence.go Normalize empty results from Reverse and Slice.
geom/​type_polygon.go Normalize empty results from reversal, orientation, and densification.
geom/​type_multi_polygon.go Normalize empty reversal and densification results.
geom/​type_multi_line_string.go Normalize empty reversal and densification results.
geom/​type_geometry_deep_equal_test.go Add canonical-form checks across geometry methods and functions.
geom/​type_geometry_collection.go Normalize empty transformation, coordinate-conversion, and densification results.
CHANGELOG.md Record equality fixes, including empty collection parsing.

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants