From 194c0a8906b0cc70191c8d5c9f7944b5a01557d0 Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Wed, 11 Nov 2026 16:07:34 +1100 Subject: [PATCH] geom: use nil for empty slices in operation results 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. --- CHANGELOG.md | 8 + geom/type_geometry_collection.go | 16 ++ geom/type_geometry_deep_equal_test.go | 378 ++++++++++++++++++++++++++ geom/type_multi_line_string.go | 8 + geom/type_multi_polygon.go | 8 + geom/type_polygon.go | 12 + geom/type_sequence.go | 11 +- 7 files changed, 440 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60dcbff4..f4ccce40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +- Fix `reflect.DeepEqual` returning false for some pairs of geometries that + `ExactEquals` reports as equal. Some operations gave an empty geometry (or + an empty member of a geometry) an empty slice where it should have a nil + one. These included `Reverse`, `Densify`, `ForceCW`, `ForceCCW`, + `TransformXY`, `Transform` and `ForceCoordinatesType`, and parsing an empty + Z, M or ZM `GeometryCollection` from WKT or WKB. `Sequence.Slice` had the + same problem when given equal indices. + - Fix quadratic time and memory (in the nesting depth) when building nested `GeometryCollection`s, including when parsing them from WKB, WKT, GeoJSON and TWKB. diff --git a/geom/type_geometry_collection.go b/geom/type_geometry_collection.go index ea06e737..6ad455b1 100644 --- a/geom/type_geometry_collection.go +++ b/geom/type_geometry_collection.go @@ -242,6 +242,10 @@ func (c *GeometryCollection) UnmarshalJSON(buf []byte) error { // [GeometryCollection] according to fn. See [Geometry.TransformXY] for more // details. func (c GeometryCollection) TransformXY(fn func(XY) XY) GeometryCollection { + if len(c.geoms) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return GeometryCollection{nil, c.ctype} + } transformed := make([]Geometry, len(c.geoms)) for i := range c.geoms { transformed[i] = c.geoms[i].TransformXY(fn) @@ -252,6 +256,10 @@ func (c GeometryCollection) TransformXY(fn func(XY) XY) GeometryCollection { // Transform transforms this [GeometryCollection] into another [GeometryCollection] // according to fn. See [Geometry.Transform] for more details. func (c GeometryCollection) Transform(fn func(CoordinatesType, []float64) error) (GeometryCollection, error) { + if len(c.geoms) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return GeometryCollection{nil, c.ctype}, nil + } transformed := make([]Geometry, len(c.geoms)) for i := range c.geoms { var err error @@ -427,6 +435,10 @@ func (c GeometryCollection) ForceCoordinatesType(newCType CoordinatesType) Geome if c.ctype == newCType { return c } + if len(c.geoms) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return GeometryCollection{nil, newCType} + } gs := make([]Geometry, len(c.geoms)) for i := range c.geoms { gs[i] = c.geoms[i].ForceCoordinatesType(newCType) @@ -584,6 +596,10 @@ func (c GeometryCollection) Simplify(threshold float64, nv ...NoValidate) (Geome // // Panics if maxDistance is zero or negative. func (c GeometryCollection) Densify(maxDistance float64) GeometryCollection { + if len(c.geoms) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return GeometryCollection{nil, c.ctype} + } gs := make([]Geometry, len(c.geoms)) for i, g := range c.geoms { gs[i] = g.Densify(maxDistance) diff --git a/geom/type_geometry_deep_equal_test.go b/geom/type_geometry_deep_equal_test.go index 6b7af029..401102ee 100644 --- a/geom/type_geometry_deep_equal_test.go +++ b/geom/type_geometry_deep_equal_test.go @@ -1,6 +1,9 @@ package geom_test import ( + "fmt" + "reflect" + "strings" "testing" "github.com/peterstace/simplefeatures/geom" @@ -73,3 +76,378 @@ func TestReflectDeepEqualWorksForZeroValue(t *testing.T) { test.DeepEqual(t, g2, g4) test.DeepEqual(t, g3, g4) } + +// TestMethodsReturnCanonicalForm checks that every method returning a +// geometry gives a result in canonical form (see [canonicalFormViolations]), +// which is what makes reflect.DeepEqual agree with ExactEquals. The methods +// are found by reflection, so new methods are checked without changes to +// this test. +func TestMethodsReturnCanonicalForm(t *testing.T) { + receivers := canonicalFormReceivers(t) + args := canonicalFormArgs() + for _, zero := range []any{ + geom.Geometry{}, + geom.Point{}, + geom.LineString{}, + geom.Polygon{}, + geom.MultiPoint{}, + geom.MultiLineString{}, + geom.MultiPolygon{}, + geom.GeometryCollection{}, + geom.Sequence{}, + geom.Envelope{}, + } { + typ := reflect.TypeOf(zero) + for i := 0; i < typ.NumMethod(); i++ { + method := typ.Method(i) + if !returnsGeometry(method.Type) { + continue + } + t.Run(typ.Name()+"."+method.Name, func(t *testing.T) { + argSets := argumentSets(t, method.Type, args) + var calls int + for _, recv := range receivers[typ] { + for _, argSet := range argSets { + in := append([]reflect.Value{recv.value}, argSet...) + results, ok := callRecovering(method.Func, in) + if !ok { + continue + } + calls++ + for _, r := range results { + if isGeometryType(r.Type()) { + checkCanonicalForm(t, recv.label+formatArgs(argSet), r) + } + } + } + } + if calls == 0 { + t.Error("every call panicked") + } + }) + } + } +} + +// TestFunctionsReturnCanonicalForm checks that package level functions +// returning geometries give results in canonical form. New functions must be +// added to the list. +func TestFunctionsReturnCanonicalForm(t *testing.T) { + poly := test.FromWKT(t, "POLYGON((0 0,0 1,1 0,0 0))") + for _, fn := range []struct { + name string + fn func(geom.Geometry) (any, error) + }{ + {"UnmarshalWKT", func(g geom.Geometry) (any, error) { + return geom.UnmarshalWKT(g.AsText()) + }}, + {"UnmarshalWKB", func(g geom.Geometry) (any, error) { + return geom.UnmarshalWKB(g.AsBinary()) + }}, + {"UnmarshalGeoJSON", func(g geom.Geometry) (any, error) { + buf, err := g.MarshalJSON() + if err != nil { + return nil, err + } + return geom.UnmarshalGeoJSON(buf) + }}, + {"UnmarshalTWKB", func(g geom.Geometry) (any, error) { + buf, err := geom.MarshalTWKB(g, 3) + if err != nil { + return nil, err + } + return geom.UnmarshalTWKB(buf) + }}, + {"NewGeometryCollection", func(g geom.Geometry) (any, error) { + return geom.NewGeometryCollection([]geom.Geometry{g, poly}), nil + }}, + {"Union", func(g geom.Geometry) (any, error) { return geom.Union(g, poly) }}, + {"Intersection", func(g geom.Geometry) (any, error) { return geom.Intersection(g, poly) }}, + {"Difference", func(g geom.Geometry) (any, error) { return geom.Difference(g, poly) }}, + {"SymmetricDifference", func(g geom.Geometry) (any, error) { return geom.SymmetricDifference(g, poly) }}, + {"UnaryUnion", func(g geom.Geometry) (any, error) { return geom.UnaryUnion(g) }}, + {"UnionMany", func(g geom.Geometry) (any, error) { return geom.UnionMany([]geom.Geometry{g, poly}) }}, + {"Buffer", func(g geom.Geometry) (any, error) { return geom.Buffer(g, 0.5) }}, + {"RotatedMinimumAreaBoundingRectangle", func(g geom.Geometry) (any, error) { + return geom.RotatedMinimumAreaBoundingRectangle(g), nil + }}, + {"RotatedMinimumWidthBoundingRectangle", func(g geom.Geometry) (any, error) { + return geom.RotatedMinimumWidthBoundingRectangle(g), nil + }}, + } { + t.Run(fn.name, func(t *testing.T) { + var calls int + for _, wkt := range canonicalFormInputs() { + result, err := fn.fn(test.FromWKT(t, wkt)) + if err != nil { + continue + } + calls++ + checkCanonicalForm(t, wkt, reflect.ValueOf(result)) + } + if calls == 0 { + t.Error("every call returned an error") + } + }) + } +} + +// canonicalFormInputs returns WKTs covering each geometry type in each +// coordinates type: empty, with an empty member, non-empty, and nested in +// collections. +func canonicalFormInputs() []string { + var wkts []string + for _, dim := range []struct{ suffix, extra string }{ + {"", ""}, {" Z", " 7"}, {" M", " 8"}, {" ZM", " 7 8"}, + } { + coords := func(xys ...float64) string { + var parts []string + for i := 0; i+1 < len(xys); i += 2 { + parts = append(parts, fmt.Sprintf("%g %g%s", xys[i], xys[i+1], dim.extra)) + } + return strings.Join(parts, ",") + } + pt := coords(1, 2) + ls := coords(0, 0, 2, 3) + cwShell := coords(0, 0, 0, 3, 3, 0, 0, 0) + ccwShell := coords(0, 0, 3, 0, 0, 3, 0, 0) + ccwHole := coords(0.5, 0.5, 1, 0.5, 0.5, 1, 0.5, 0.5) + + s := dim.suffix + base := []string{ + "POINT" + s + " EMPTY", + "POINT" + s + "(" + pt + ")", + "LINESTRING" + s + " EMPTY", + "LINESTRING" + s + "(" + ls + ")", + "POLYGON" + s + " EMPTY", + "POLYGON" + s + "((" + cwShell + "),(" + ccwHole + "))", + "POLYGON" + s + "((" + ccwShell + "))", + "MULTIPOINT" + s + " EMPTY", + "MULTIPOINT" + s + "(EMPTY)", + "MULTIPOINT" + s + "((" + pt + "),EMPTY)", + "MULTILINESTRING" + s + " EMPTY", + "MULTILINESTRING" + s + "(EMPTY)", + "MULTILINESTRING" + s + "((" + ls + "),EMPTY)", + "MULTIPOLYGON" + s + " EMPTY", + "MULTIPOLYGON" + s + "(EMPTY)", + "MULTIPOLYGON" + s + "(((" + cwShell + ")),EMPTY)", + "MULTIPOLYGON" + s + "(((" + ccwShell + ")),EMPTY)", + } + wkts = append(wkts, base...) + for _, b := range base { + wkts = append(wkts, "GEOMETRYCOLLECTION"+s+"("+b+")") + } + wkts = append(wkts, + "GEOMETRYCOLLECTION"+s+" EMPTY", + "GEOMETRYCOLLECTION"+s+"(GEOMETRYCOLLECTION"+s+" EMPTY)", + "GEOMETRYCOLLECTION"+s+"(GEOMETRYCOLLECTION"+s+"(POLYGON"+s+" EMPTY))", + ) + } + return wkts +} + +type canonicalFormReceiver struct { + label string + value reflect.Value +} + +// canonicalFormReceivers derives receivers for each type's methods from +// [canonicalFormInputs]. It reports any receiver that isn't in canonical +// form, since a method that returns its receiver unchanged would otherwise +// take the blame. +func canonicalFormReceivers(t *testing.T) map[reflect.Type][]canonicalFormReceiver { + t.Helper() + receivers := make(map[reflect.Type][]canonicalFormReceiver) + for _, wkt := range canonicalFormInputs() { + g := test.FromWKT(t, wkt) + var concrete any + switch g.Type() { + case geom.TypePoint: + concrete = g.MustAsPoint() + case geom.TypeLineString: + concrete = g.MustAsLineString() + case geom.TypePolygon: + concrete = g.MustAsPolygon() + case geom.TypeMultiPoint: + concrete = g.MustAsMultiPoint() + case geom.TypeMultiLineString: + concrete = g.MustAsMultiLineString() + case geom.TypeMultiPolygon: + concrete = g.MustAsMultiPolygon() + case geom.TypeGeometryCollection: + concrete = g.MustAsGeometryCollection() + } + for _, recv := range []any{g, concrete, g.DumpCoordinates(), g.Envelope()} { + v := reflect.ValueOf(recv) + if isGeometryType(v.Type()) { + checkCanonicalForm(t, wkt, v) + } + receivers[v.Type()] = append(receivers[v.Type()], canonicalFormReceiver{wkt, v}) + } + } + return receivers +} + +func canonicalFormArgs() map[reflect.Type][]reflect.Value { + transformXY := func(xy geom.XY) geom.XY { return xy } + transform := func(geom.CoordinatesType, []float64) error { return nil } + return map[reflect.Type][]reflect.Value{ + reflect.TypeOf(0.0): {reflect.ValueOf(0.5)}, + reflect.TypeOf(0): {reflect.ValueOf(0), reflect.ValueOf(1)}, + reflect.TypeOf(geom.DimXY): { + reflect.ValueOf(geom.DimXY), + reflect.ValueOf(geom.DimXYZ), + reflect.ValueOf(geom.DimXYM), + reflect.ValueOf(geom.DimXYZM), + }, + reflect.TypeOf(transformXY): {reflect.ValueOf(transformXY)}, + reflect.TypeOf(transform): {reflect.ValueOf(transform)}, + } +} + +func returnsGeometry(fnType reflect.Type) bool { + for i := 0; i < fnType.NumOut(); i++ { + if isGeometryType(fnType.Out(i)) { + return true + } + } + return false +} + +// isGeometryType reports whether typ is a geometry type, or a slice of a +// geometry type. +func isGeometryType(typ reflect.Type) bool { + if typ.Kind() == reflect.Slice { + typ = typ.Elem() + } + switch typ { + case reflect.TypeOf(geom.Geometry{}), + reflect.TypeOf(geom.Point{}), + reflect.TypeOf(geom.LineString{}), + reflect.TypeOf(geom.Polygon{}), + reflect.TypeOf(geom.MultiPoint{}), + reflect.TypeOf(geom.MultiLineString{}), + reflect.TypeOf(geom.MultiPolygon{}), + reflect.TypeOf(geom.GeometryCollection{}), + reflect.TypeOf(geom.Sequence{}): + return true + } + return false +} + +// argumentSets returns every combination of arguments from args for the +// parameters of the method type fnType, leaving out the receiver and any +// variadic parameter. +func argumentSets(t *testing.T, fnType reflect.Type, args map[reflect.Type][]reflect.Value) [][]reflect.Value { + t.Helper() + n := fnType.NumIn() + if fnType.IsVariadic() { + n-- + } + sets := [][]reflect.Value{nil} + for i := 1; i < n; i++ { + options, ok := args[fnType.In(i)] + if !ok { + t.Fatalf("no arguments for parameter type %v, add some to canonicalFormArgs", fnType.In(i)) + } + var next [][]reflect.Value + for _, set := range sets { + for _, opt := range options { + next = append(next, append(set[:len(set):len(set)], opt)) + } + } + sets = next + } + return sets +} + +// callRecovering calls fn, reporting false if it panics. Some methods panic +// for some inputs, such as PointN with an index that is out of range. +func callRecovering(fn reflect.Value, in []reflect.Value) (results []reflect.Value, ok bool) { + defer func() { + if recover() != nil { + ok = false + } + }() + return fn.Call(in), true +} + +func formatArgs(args []reflect.Value) string { + if len(args) == 0 { + return "" + } + parts := make([]string, len(args)) + for i, a := range args { + if a.Kind() == reflect.Func { + parts[i] = "func" + } else { + parts[i] = fmt.Sprint(a.Interface()) + } + } + return " with args (" + strings.Join(parts, ", ") + ")" +} + +// checkCanonicalForm fails the test if v isn't in canonical form. A slice +// is checked element by element, since the slice itself isn't a geometry. +func checkCanonicalForm(t *testing.T, label string, v reflect.Value) { + t.Helper() + var violations []string + if v.Kind() == reflect.Slice { + for i := 0; i < v.Len(); i++ { + violations = append(violations, canonicalFormViolations(v.Index(i), fmt.Sprintf("[%d]", i))...) + } + } else { + violations = canonicalFormViolations(v, v.Type().Name()) + } + for _, msg := range violations { + t.Errorf("%s: %s", label, msg) + } +} + +// canonicalFormViolations reads v through its unexported fields and +// describes each way it differs from canonical form. reflect.DeepEqual agrees +// with ExactEquals on values in canonical form, where: +// +// - Every slice is nil or non-empty. +// - An empty Point has zero X, Y, Z and M values. +// - A Geometry holding an empty XY GeometryCollection is the zero Geometry. +func canonicalFormViolations(v reflect.Value, path string) []string { + var violations []string + switch v.Kind() { + case reflect.Slice: + if !v.IsNil() && v.Len() == 0 { + violations = append(violations, path+": empty slice that isn't nil") + } + for i := 0; i < v.Len(); i++ { + violations = append(violations, canonicalFormViolations(v.Index(i), fmt.Sprintf("%s[%d]", path, i))...) + } + case reflect.Interface: + if !v.IsNil() { + violations = append(violations, canonicalFormViolations(v.Elem(), path+"("+v.Elem().Type().Name()+")")...) + } + case reflect.Struct: + switch v.Type() { + case reflect.TypeOf(geom.Point{}): + coords := v.FieldByName("coords") + if !v.FieldByName("full").Bool() { + for _, name := range []string{"X", "Y", "Z", "M"} { + if coords.FieldByName(name).Float() != 0 { + violations = append(violations, path+": empty Point with non-zero "+name) + } + } + } + case reflect.TypeOf(geom.Geometry{}): + impl := v.FieldByName("impl") + if !impl.IsNil() && impl.Elem().Type() == reflect.TypeOf(geom.GeometryCollection{}) { + gc := impl.Elem() + if gc.FieldByName("geoms").Len() == 0 && geom.CoordinatesType(gc.FieldByName("ctype").Uint()) == geom.DimXY { + violations = append(violations, path+": empty XY GeometryCollection that isn't the zero Geometry") + } + } + } + for i := 0; i < v.NumField(); i++ { + violations = append(violations, canonicalFormViolations(v.Field(i), path+"."+v.Type().Field(i).Name)...) + } + } + return violations +} diff --git a/geom/type_multi_line_string.go b/geom/type_multi_line_string.go index afbba577..df65a701 100644 --- a/geom/type_multi_line_string.go +++ b/geom/type_multi_line_string.go @@ -387,6 +387,10 @@ func (m MultiLineString) Centroid() Point { // Reverse in the case of MultiLineString outputs each component line string in their // original order, each individually reversed. func (m MultiLineString) Reverse() MultiLineString { + if len(m.lines) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return MultiLineString{nil, m.ctype} + } linestrings := make([]LineString, len(m.lines)) // Form the reversed slice. for i := 0; i < len(m.lines); i++ { @@ -538,6 +542,10 @@ func (m MultiLineString) Simplify(threshold float64) MultiLineString { // // Panics if maxDistance is zero or negative. func (m MultiLineString) Densify(maxDistance float64) MultiLineString { + if len(m.lines) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return MultiLineString{nil, m.ctype} + } lss := make([]LineString, len(m.lines)) for i := range m.lines { lss[i] = m.lines[i].Densify(maxDistance) diff --git a/geom/type_multi_polygon.go b/geom/type_multi_polygon.go index c5cd93e6..13a2d33d 100644 --- a/geom/type_multi_polygon.go +++ b/geom/type_multi_polygon.go @@ -394,6 +394,10 @@ func (m MultiPolygon) Centroid() Point { // Reverse in the case of MultiPolygon outputs the component polygons in their original order, // each individually reversed. func (m MultiPolygon) Reverse() MultiPolygon { + if len(m.polys) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return MultiPolygon{nil, m.ctype} + } polys := make([]Polygon, len(m.polys)) // Form the reversed slice. for i := 0; i < len(m.polys); i++ { @@ -595,6 +599,10 @@ func (m MultiPolygon) Simplify(threshold float64, nv ...NoValidate) (MultiPolygo // // Panics if maxDistance is zero or negative. func (m MultiPolygon) Densify(maxDistance float64) MultiPolygon { + if len(m.polys) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return MultiPolygon{nil, m.ctype} + } ps := make([]Polygon, len(m.polys)) for i, p := range m.polys { ps[i] = p.Densify(maxDistance) diff --git a/geom/type_polygon.go b/geom/type_polygon.go index 3805e199..c4ef0b0b 100644 --- a/geom/type_polygon.go +++ b/geom/type_polygon.go @@ -513,6 +513,10 @@ func (p Polygon) AsMultiPolygon() MultiPolygon { // Reverse in the case of [Polygon] outputs the coordinates of each ring in reverse order, // but note the order of the inner rings is unchanged. func (p Polygon) Reverse() Polygon { + if len(p.rings) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return Polygon{nil, p.ctype} + } reversed := make([]LineString, len(p.rings)) for i := range reversed { reversed[i] = p.rings[i].Reverse() @@ -572,6 +576,10 @@ func (p Polygon) ForceCCW() Polygon { } func (p Polygon) forceOrientation(forceCW bool) Polygon { + if len(p.rings) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return Polygon{nil, p.ctype} + } orientedRings := make([]LineString, len(p.rings)) for i, ring := range p.rings { alreadyCW := signedAreaOfLinearRing(ring, nil) < 0 @@ -715,6 +723,10 @@ func (p Polygon) Simplify(threshold float64, nv ...NoValidate) (Polygon, error) // // Panics if maxDistance is zero or negative. func (p Polygon) Densify(maxDistance float64) Polygon { + if len(p.rings) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return Polygon{nil, p.ctype} + } rings := make([]LineString, len(p.rings)) for i, r := range p.rings { rings[i] = r.Densify(maxDistance) diff --git a/geom/type_sequence.go b/geom/type_sequence.go index c2b320f8..3ba42650 100644 --- a/geom/type_sequence.go +++ b/geom/type_sequence.go @@ -99,6 +99,10 @@ func (s Sequence) GetXY(i int) XY { // Reverse returns a new [Sequence] containing the same point locations, but in // reversed order. func (s Sequence) Reverse() Sequence { + if len(s.floats) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + return Sequence{s.ctype, nil} + } stride := s.ctype.Dimension() n := s.Length() reversed := make([]float64, len(s.floats)) @@ -116,7 +120,12 @@ func (s Sequence) Reverse() Sequence { // rules work in the same way as Go Slices. func (s Sequence) Slice(i, j int) Sequence { stride := s.ctype.Dimension() - return Sequence{s.ctype, s.floats[i*stride : j*stride]} + floats := s.floats[i*stride : j*stride] + if len(floats) == 0 { + // Use nil rather than an empty slice so that reflect.DeepEqual works correctly. + floats = nil + } + return Sequence{s.ctype, floats} } // ForceCoordinatesType returns a new [Sequence] with a different [CoordinatesType]. If a