Broken referential transparency?

[andreabedini]

Hey there,

thanks for the package.

The function Data.Geometry.Geos.STRTree.insert has the following type signature

insert :: Storable a => Geometry b -> a -> STRTreeBuilder a -> () 

which looks very suspicious. Either this function has no effect at all or it has some hidden IO in which it modifies the passed builder.

This seems to be indeed the case and it's easy to make the library crash

λ> import Data.Geometry.Geos.STRTree
λ> import Data.Geometry.Geos.Geometry
λ> p = PointGeometry (point $ coordinate2 0.1 0.2) Nothing
λ> empty
STRTreeBuilder 0x0000000000eb5ff0
λ> insert p 1 empty
()
λ> toList (build empty)
[()]
λ> insert p 1 empty
ghc: AbstractSTRtree.cpp:115: virtual void geos::index::strtree::AbstractSTRtree::insert(const void*, void*): Assertion `!built' failed.
cabal: repl failed for fake-package-0. The build process terminated with exit
code -6

[ewestern]

Hi there, Thanks for pointing this out. This haskell library is a wrapper around the geos library, and as such has to work with the memory management characteristics of the underlying library. The reason the insert function looks as it does is because the underlying geos data structure is mutable and -- unlike other data structures in geos -- the semantics of the STRTree kind of require it to be mutable. I have an idea for how to fix your issue -- give me a few days and I'll push something.

[ewestern]

On reflection, I think the best move is to remove STRTreeBuilder from the interface (along with its associated functions). Then, the only function for creating STRTree will be fromList and fromFoldable, which will completely avoid the mutability problem. I'm open to creating other convenient functions for constructing STRTrees. I was considering an unfoldr function, which would have the type signature: unfoldr :: (b -> Maybe ((Geometry, a), b)) -> b -> STRTree a Thoughts, @andreabedini ??

[andreabedini]

Hi @ewestern,

I think the simplest approach is to own up to the mutability. How about the following types:

empty :: IO STRTreeBuilder
build :: STRTreeBuilder a -> IO STRTree a 
insert :: Storable a => Geometry b -> a -> STRTreeBuilder a -> IO () 

They might be a way to use ST rather than IO but I haven't done it with FFI yet. I'll investigate.

Edit: this is what I was thinking https://stackoverflow.com/questions/20938819/how-do-i-perform-ffi-calls-inside-st-monad

Edit 2: thinking more about it, perhaps it's a good idea to leave anything in IO and let the user decide whether to use unsafeIOToST. There might be some considerations to make around thread-safety. Admittedly I don't know geos very well.

[ewestern]

I went ahead with the decision to abandon STRTreeBuilder. https://hackage.haskell.org/package/geos-0.4.1

I think this approach is best in line with what users will expect when using this library. Regarding your edit 2, note that I've exposed the "Raw" module, which exposes functions that are much closer to the underlying GEOS api -- these functions run in a Geos monad and can be used if you're looking for a more "honest" abstraction. That said, the main reason the low-level FFI functions run in IO is because of the effectful logging that geos does. Assuming that this logging and the memory management is handled correctly (which it may not be!), I don't feel too bad about calling unsafePerformIO to exit the Geos monad -- which is what the functions outside of the Raw module end up doing.

Regarding your suggestion to use ST. I'm definitely open to something like that for the Raw module. To be honest, I haven't used ST directly before, so I don't have a good sense for the advantages there.

With this recent patch, I'm going to close this issue. Feel free to open another if you have ideas for functions in the Raw module -- which appears to be closer to what you're looking for.

[andreabedini]

Thanks @ewestern, it sounds like a good step forward. Exposing the Raw module will allow us to experiment with different approaches. Cheers!