Redefine (#), fixes #198

[exFalso]

This PR fixes a subtle bug in the definition of (#). To recap the previous definition of (#) specialised to Vectors was:

avec :: Storable a => (CInt -> Ptr a -> b) -> Vector a -> b
avec f v = inlinePerformIO (unsafeWith v (return . f (fromIntegral (Vector.length v))))

b will be unified with further function types, and eventually IO. However note the part unsafeWith v (return (..)): We are not actually executing the eventual nested IO action, but rather just returning it! This means that the IO that b will be unified with escapes the bracket of unsafeWith.

This resulted in rare cases of segfaults/corrupt memory, as the runtime deallocated the underlying memory earlier than FFI calls finished.

This fix redefines (#) to build up a continuation ((CInt -> Ptr a -> ... -> IO r) -> IO r) instead that nests the passed in function into the unsafeWith brackets safely. This also avoids the use of inlinePerformIO.

[albertoruiz]

Thanks for the fix!

The idea of (#) was taken from the contributed module https://github.com/albertoruiz/hmatrix/blob/master/packages/base/src/Internal/Foreign.hs, which is much more elegant than my previous wrapper helpers. All tests pass and I have never experienced any segfault, it seems that this is very subtle and dangerous bug. Perhaps this module must also be fixed.

Do you think it is possible to avoid swapping argument order in the usage of the new (#) and (#!) ?

[bitonic]

@albertoruiz indeed, those definitions are broken too. Those type signatures make it impossible for the bracketing to actually happen, we'll need to change them to something else -- possibly with the style introduced in this PR.

[bitonic]

BTW, swapping the arguments might be possible with some type-class machinery. The basic problem to solve is that the final id needs to be pushed to the right of the #s, which is not easily done without type classes.

[bitonic]

@mikeplus64 beyond what we've fixed and the other module cited by @albertoruiz , are there other parts that might also incur in the same problem?

[exFalso]

Indeed, those functions have the same issue.

Re argument ordering: We could add another primitive #@ like this:

f #@ a # b # c #! d

The reason we went with passing in the function at the end was that this way it's very clear that we're not actually partially applying the function, but rather building up a closure that expects the final f.

As @bitonic points out we can also use type class machinery to unify the syntax completely. The goal is to build up this application:

(avec a (avec b (avec c (avec d id)))) f

[bitonic]

As a side note, me and @exFalso think that there it's not so absurd to have the function on the right -- since it echoes the bracketing those #s are performing.

[albertoruiz]

Ok, in that case I am happy with the function on the right. Thanks for the explanations!

[mikeplus64]

@bitonic, it's been a very long while, but I don't think so. Seems much more elegant than the hacky unsafePerformIO/inlinePerformIO stuff so kudos!

[rcook]

This changes the public API by dropping functions, so it might be worth uploading the documentation for 0.18.0.0 to Hackage since the latests docs are for 0.17.0.2:

https://hackage.haskell.org/package/hmatrix-0.17.0.2

[albertoruiz]

Ok, I will do it soon.