Repository: Toblerity/Shapely (python)

Issue:
Inconsistent behavior when passing empty Points to MultiPoint
The [documentation on `MultiPoint`](https://shapely.readthedocs.io/en/latest/manual.html#MultiPoint) says that I can pass to it a sequence of `Point`s. But it doesn't warn us against passing empty `Point`s. Interestingly enough, if I pass an empty `Point` as the first element, it will throw an `AssertionError`:
```python
>>> from shapely.geometry import Point, MultiPoint
>>> MultiPoint([Point()])
```
```none
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-30-8942eaebd9a2> in <module>
----> 1 MultiPoint([Point()])

~\Miniconda3\lib\site-packages\shapely\geometry\multipoint.py in __init__(self, points)
     56             pass
     57         else:
---> 58             self._geom, self._ndim = geos_multipoint_from_py(points)
     59 
     60     def shape_factory(self, *args):

~\Miniconda3\lib\site-packages\shapely\geometry\multipoint.py in geos_multipoint_from_py(ob)
    162     except TypeError:
    163         n = ob[0]._ndim
--> 164     assert n == 2 or n == 3
    165 
    166     # Array of pointers to point geometries

AssertionError: 
```
```python
>>> MultiPoint([Point(), Point(1, 2)])
```
```none
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-31-08a0b83c889a> in <module>
----> 1 MultiPoint([Point(), Point(1, 2)])

~\Miniconda3\lib\site-packages\shapely\geometry\multipoint.py in __init__(self, points)
     56             pass
     57         else:
---> 58             self._geom, self._ndim = geos_multipoint_from_py(points)
     59 
     60     def shape_factory(self, *args):

~\Miniconda3\lib\site-packages\shapely\geometry\multipoint.py in geos_multipoint_from_py(ob)
    162     except TypeError:
    163         n = ob[0]._ndim
--> 164     assert n == 2 or n == 3
    165 
    166     # Array of pointers to point geometries

AssertionError: 
```
But if the empty `Point` goes after a nonempty `Point`, then it works fine:
```python
>>> MultiPoint([Point(1, 2), Point()])
>>> # no error here
```

But the resulting object will throw an exception after trying to get its wkt:
```python
>>> MultiPoint([Point(1, 2), Point()]).wkt
```
```none
---------------------------------------------------------------------------
OSError                                   Traceback (most recent call last)
<ipython-input-38-c645b8b312e6> in <module>
----> 1 MultiPoint([Point(1, 2), Point()]).wkt

~\Miniconda3\lib\site-packages\shapely\geometry\base.py in wkt(self, **kw)
    365     def wkt(self, **kw):
    366         """WKT representation of the geometry"""
--> 367         return WKTWriter(lgeos, **kw).write(self)
    368 
    369     @property

~\Miniconda3\lib\site-packages\shapely\geos.py in write(self, geom)
    361         if geom is None or geom._geom is None:
    362             raise ValueError("Null geometry supports no operations")
--> 363         result = self._lgeos.GEOSWKTWriter_write(self._writer, geom._geom)
    364         text = string_at(result)
    365         lgeos.GEOSFree(result)

OSError: exception: access violation reading 0x0000000000000000
```
I assume there needs to be a guard against a presence of empty points in the passed sequence, and the docs should be updated too.

---
Shapely version: 1.6.4.post1, installed from conda.

Pull request:
Raise an error on creation of MultiPoint or LineString with empty components
Closes #836.

As of Shapely 1.7, asking for the WKT of a MultiPoint or -LineString with an empty component results in a segfault. I figured it made more sense to error in that case and let the user take care of the issue - realistically I can't see much utility in having a MultiPoint or -LineString with an empty component anyway, and I don't think you can create such a geometry through WKT, WKB, or GeoJSON, so it doesn't make sense to allow it through the geometry object constructor, either.

Patch:
diff --git a/shapely/errors.py b/shapely/errors.py
index 8166f5a..1cb88cf 100644
--- a/shapely/errors.py
+++ b/shapely/errors.py
@@ -31,3 +31,7 @@ class TopologicalError(ShapelyError):
 
 class PredicateError(ShapelyError):
     """A geometric predicate has failed to return True/False."""
+
+
+class EmptyPartError(ShapelyError):
+    """An error signifying an empty part was encountered when creating a multi-part."""
diff --git a/shapely/geometry/multilinestring.py b/shapely/geometry/multilinestring.py
index ca432df..9b4708a 100644
--- a/shapely/geometry/multilinestring.py
+++ b/shapely/geometry/multilinestring.py
@@ -8,6 +8,7 @@ if sys.version_info[0] < 3:
 
 from ctypes import c_void_p, cast
 
+from shapely.errors import EmptyPartError
 from shapely.geos import lgeos
 from shapely.geometry.base import BaseMultipartGeometry, geos_geom_from_py
 from shapely.geometry import linestring
@@ -132,6 +133,10 @@ def geos_multilinestring_from_py(ob):
     # add to coordinate sequence
     for l in range(L):
         geom, ndims = linestring.geos_linestring_from_py(obs[l])
+
+        if lgeos.GEOSisEmpty(geom):
+            raise EmptyPartError("Can't create MultiLineString with empty component")
+
         subs[l] = cast(geom, c_void_p)
             
     return (lgeos.GEOSGeom_createCollection(5, subs, L), N)
diff --git a/shapely/geometry/multipoint.py b/shapely/geometry/multipoint.py
index 538b75b..c2869f9 100644
--- a/shapely/geometry/multipoint.py
+++ b/shapely/geometry/multipoint.py
@@ -8,6 +8,7 @@ if sys.version_info[0] < 3:
 
 from ctypes import byref, c_double, c_void_p, cast
 
+from shapely.errors import EmptyPartError
 from shapely.geos import lgeos
 from shapely.geometry.base import (
     BaseMultipartGeometry, exceptNull, geos_geom_from_py)
@@ -169,6 +170,10 @@ def geos_multipoint_from_py(ob):
     for i in range(m):
         coords = ob[i]
         geom, ndims = point.geos_point_from_py(coords)
+
+        if lgeos.GEOSisEmpty(geom):
+            raise EmptyPartError("Can't create MultiPoint with empty component")
+
         subs[i] = cast(geom, c_void_p)
 
     return lgeos.GEOSGeom_createCollection(4, subs, m), n
diff --git a/shapely/strtree.py b/shapely/strtree.py
index 00dea7c..eee22e0 100644
--- a/shapely/strtree.py
+++ b/shapely/strtree.py
@@ -46,7 +46,11 @@ class STRtree:
 
     def __del__(self):
         if self._tree_handle is not None:
-            lgeos.GEOSSTRtree_destroy(self._tree_handle)
+            try:
+                lgeos.GEOSSTRtree_destroy(self._tree_handle)
+            except AttributeError:
+                pass  # lgeos might be empty on shutdown.
+
             self._tree_handle = None
 
     def query(self, geom):


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 9cf4ac0..5a6094d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2,6 +2,12 @@ import sys
 
 import pytest
 
+from shapely.geos import geos_version
+
+
+requires_geos_342 = pytest.mark.skipif(geos_version < (3, 4, 2), reason="GEOS > 3.4.2 is required.")
+
+
 def pytest_addoption(parser):
     parser.addoption("--with-speedups", action="store_true", default=False,
                      help="Run tests with speedups.")
diff --git a/tests/test_multilinestring.py b/tests/test_multilinestring.py
index be3f4ab..74c201d 100644
--- a/tests/test_multilinestring.py
+++ b/tests/test_multilinestring.py
@@ -1,5 +1,6 @@
 from . import unittest, numpy, test_int_types
 from .test_multi import MultiGeometryTestCase
+from shapely.errors import EmptyPartError
 from shapely.geos import lgeos
 from shapely.geometry import LineString, MultiLineString, asMultiLineString
 from shapely.geometry.base import dump_coords
@@ -79,6 +80,15 @@ class MultiLineStringTestCase(MultiGeometryTestCase):
         line1 = LineString([(4.0, 5.0), (6.0, 7.0)])
         self.subgeom_access_test(MultiLineString, [line0, line1])
 
+    def test_create_multi_with_empty_component(self):
+        with self.assertRaises(EmptyPartError) as exc:
+            wkt = MultiLineString([
+                LineString([(0, 0), (1, 1), (2, 2)]),
+                LineString()
+            ]).wkt
+
+        self.assertEqual(str(exc.exception), "Can't create MultiLineString with empty component")
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromTestCase(MultiLineStringTestCase)
diff --git a/tests/test_multipoint.py b/tests/test_multipoint.py
index c84443c..b7bb657 100644
--- a/tests/test_multipoint.py
+++ b/tests/test_multipoint.py
@@ -1,5 +1,6 @@
 from . import unittest, numpy, test_int_types
 from .test_multi import MultiGeometryTestCase
+from shapely.errors import EmptyPartError
 from shapely.geometry import Point, MultiPoint, asMultiPoint
 from shapely.geometry.base import dump_coords
 
@@ -74,5 +75,12 @@ class MultiPointTestCase(MultiGeometryTestCase):
         p1 = Point(3.0, 4.0)
         self.subgeom_access_test(MultiPoint, [p0, p1])
 
+    def test_create_multi_with_empty_component(self):
+        with self.assertRaises(EmptyPartError) as exc:
+            wkt = MultiPoint([Point(0, 0), Point()]).wkt
+
+        self.assertEqual(str(exc.exception), "Can't create MultiPoint with empty component")
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromTestCase(MultiPointTestCase)
diff --git a/tests/test_strtree.py b/tests/test_strtree.py
index 8eb5dcf..e046cbe 100644
--- a/tests/test_strtree.py
+++ b/tests/test_strtree.py
@@ -1,69 +1,81 @@
 import gc
 
-from . import unittest
-
 from shapely.strtree import STRtree
 from shapely.geometry import Point, Polygon
-from shapely.geos import geos_version
-
-
-@unittest.skipIf(geos_version < (3, 4, 2), 'GEOS 3.4.2 required')
-class STRTestCase(unittest.TestCase):
-    def test_query(self):
-        points = [Point(i, i) for i in range(10)]
-        tree = STRtree(points)
-        results = tree.query(Point(2,2).buffer(0.99))
-        self.assertEqual(len(results), 1)
-        results = tree.query(Point(2,2).buffer(1.0))
-        self.assertEqual(len(results), 3)
-
-    def test_insert_empty_geometry(self):
-        """
-        Passing nothing but empty geometries results in an empty strtree.
-        The query segfaults if the empty geometry was actually inserted.
-        """
-        empty = Polygon()
-        geoms = [empty]
-        tree = STRtree(geoms)
-        assert(tree._n_geoms == 0)
-        query = Polygon([(0,0),(1,1),(2,0),(0,0)])
-        results = tree.query(query)
-
-    def test_query_empty_geometry(self):
-        """
-        Empty geometries should be filtered out.
-        The query segfaults if the empty geometry was actually inserted.
-        """
-        empty = Polygon()
-        point = Point(1, 0.5)
-        geoms = [empty, point]
-        tree = STRtree(geoms)
-        assert(tree._n_geoms == 1)
-        query = Polygon([(0,0),(1,1),(2,0),(0,0)])
-        results = tree.query(query)
-        self.assertEqual(len(results), 1)
-        self.assertEqual(results[0], point)
-
-    def test_references(self):
-        """Don't crash due to dangling references"""
-        empty = Polygon()
-        point = Point(1, 0.5)
-        geoms = [empty, point]
-        tree = STRtree(geoms)
-        assert(tree._n_geoms == 1)
-
-        empty = None
-        point = None
-        gc.collect()
-
-        query = Polygon([(0,0),(1,1),(2,0),(0,0)])
-        results = tree.query(query)
-        self.assertEqual(len(results), 1)
-        self.assertEqual(results[0], Point(1, 0.5))
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromTestCase(STRTestCase)
-
-if __name__ == '__main__':
-    unittest.main()
+
+from shapely import strtree
+
+from tests.conftest import requires_geos_342
+
+
+@requires_geos_342
+def test_query():
+    points = [Point(i, i) for i in range(10)]
+    tree = STRtree(points)
+    results = tree.query(Point(2, 2).buffer(0.99))
+    assert len(results) == 1
+    results = tree.query(Point(2, 2).buffer(1.0))
+    assert len(results) == 3
+
+
+@requires_geos_342
+def test_insert_empty_geometry():
+    """
+    Passing nothing but empty geometries results in an empty strtree.
+    The query segfaults if the empty geometry was actually inserted.
+    """
+    empty = Polygon()
+    geoms = [empty]
+    tree = STRtree(geoms)
+    assert tree._n_geoms == 0
+    query = Polygon([(0, 0), (1, 1), (2, 0), (0, 0)])
+    results = tree.query(query)
+    assert len(results) == 0
+
+
+@requires_geos_342
+def test_query_empty_geometry():
+    """
+    Empty geometries should be filtered out.
+    The query segfaults if the empty geometry was actually inserted.
+    """
+    empty = Polygon()
+    point = Point(1, 0.5)
+    geoms = [empty, point]
+    tree = STRtree(geoms)
+    assert tree._n_geoms == 1
+    query = Polygon([(0, 0), (1, 1), (2, 0), (0, 0)])
+    results = tree.query(query)
+    assert len(results) == 1
+    assert results[0] == point
+
+
+@requires_geos_342
+def test_references():
+    """Don't crash due to dangling references"""
+    empty = Polygon()
+    point = Point(1, 0.5)
+    geoms = [empty, point]
+    tree = STRtree(geoms)
+    assert tree._n_geoms == 1
+
+    empty = None
+    point = None
+    gc.collect()
+
+    query = Polygon([(0, 0), (1, 1), (2, 0), (0, 0)])
+    results = tree.query(query)
+    assert len(results) == 1
+    assert results[0] == Point(1, 0.5)
+
+
+@requires_geos_342
+def test_safe_delete():
+    tree = STRtree([])
+
+    _lgeos = strtree.lgeos
+    strtree.lgeos = None
+
+    del tree
+
+    strtree.lgeos = _lgeos