Explore using ST for effects (issue #24)

[Dretch]

I have been exploring how this library could look if it allowed using ST for effects in addition to Effect.

The changes I suggest here break backwards compatibility somewhat, in order to provide an API that I think makes more sense. Function names have been kept the same but the types and module arrangements have changed.

In short, the modules I propose to have in this library are:

• Node.Buffer: Immutable buffers and functions that operate on them. This is something that the current library does not have, but I think it makes sense in the same way that STArray has Array.
• Node.Buffer.Mutable: Contains an EffectBuffer type, a STBuffer h type and a MutableBuffer b e typeclass with all the read and write operations, and instances of MutableBuffer EffectBuffer Effect and MutableBuffer (STBuffer h) (ST h). The typeclass allows you to write code that can operate in the Effect or the ST effect, rather like how MArray works in Haskell.
• Node.Buffer.Mutable.Unsafe: Unsafe operations on mutable buffers - using a MutableBufferUnsafe b e typeclass.
• Node.Buffer.Encoding - Unchanged

I realise this is quite a big unsolicited change, so I don't expect it to be merged... but maybe I can provoke some discussion: what do you think?

[hdgarrood]

I haven’t looked properly yet but having just read the PR description this seems like it’s worth pursuing. 👍

[hdgarrood]

Fixes #24

[hdgarrood]

I like the approach, but I'd quite like to make this non-breaking, and in particular I'd like the Buffer type to retain its current meaning - a mutable Buffer which can be mutated in the Effect monad. I wonder if we can make it non-breaking as follows:

• Have Buffer retain its current meaning (i.e. the type which in this PR is currently called EffectBuffer)
• Introduce a new type ImmutableBuffer for immutable buffers (I think immutable buffers are unusual, generally the point of them is for use in imperative-style code which uses mutation, so it makes sense to have the names reflect that, right?)
• Re-export the methods of the MutableBuffer class from Node.Buffer, keeping the API as close as possible to the current API.

Perhaps it would also make sense to put the class on its own in a module Node.Buffer.Class, put the unsafe method implementations in a Node.Buffer.Internal module, and put the concrete mutable buffer types and instances in Node.Buffer and Node.Buffer.ST modules respectively.

[Dretch]

@hdgarrood

I like the approach, but I'd quite like to make this non-breaking, and in particular I'd like the Buffer type to retain its current meaning - a mutable Buffer which can be mutated in the Effect monad. I wonder if we can make it non-breaking as follows...

I have now shuffled things around to try and keep the API is similar as possible to before - I hope this satisfies your requests.

Perhaps it would also make sense to put the class on its own in a module Node.Buffer.Class, put the unsafe method implementations in a Node.Buffer.Internal module, and put the concrete mutable buffer types and instances in Node.Buffer and Node.Buffer.ST modules respectively.

I have experimented with this in a separate branch: https://github.com/Dretch/purescript-node-buffer/tree/stbuffer-typeclass-modularised

There is one thing I am not sure is okay with this approach: because the instances of MutableBuffer are now in separate modules to the class itself, we can no long have:

class Monad m <= MutableBuffer buf m | m -> buf, buf -> m where ...

Instead we must have:

class Monad m <= MutableBuffer buf m | buf -> m where ...

This means that users of this module will have to add type assertions near methods of MutableBuffer when the compiler knows the type of the monad but not the type of the buffer. I'm not sure if this is actually going to be a problem in practice, though.

[hdgarrood]

Thanks! I think I actually prefer it without the m -> buf functional dependency - it's probably a good thing that it's now feasible to add other implementations in separate modules. In particular, I think it's conceivable that there might be some other type HypotheticalBuffer which permits, say, a MutableBuffer Effect HypotheticalBuffer instance. I even considered asking you to drop that fundep for this reason in the first place! Having to add type annotations occasionally is not too onerous either, I think.

[Dretch]

Yup, I understand the MutableBuffer Effect HypotheticalBuffer scenario.

I have now merged the stbuffer-typeclass-modularised branch into this one. I am done on this pull request, unless you have other comments. :)

EDIT: Actually there is one more thing to mention. Right now the docs for MutableBuffer are missing the method docs because of purescript/purescript#3507 - I'm not sure if its worth bothering to workaround this though (by moving the method docs into the class docs).

[hdgarrood]

I'm tempted to say leave it, because otherwise moving the docs back again will be a faff once that issue is resolved.

[hdgarrood]

Does anyone else have comments on this? This is looking like it's basically ready for merging to me now. Note that this is technically a breaking change - as @Dretch pointed out - because we've generalised most of the functions exported by Node.Buffer which means that some instances where they are used may no longer compile. However, most downstream users won't need to change any of their code, and also we need to do a breaking release anyway to fix #25, so I'm not too bothered.

[hdgarrood]

Thanks very much; I think this looks great.