Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explore using ST for effects (issue #24) #33

Merged
merged 11 commits into from
Mar 10, 2019
Merged

Explore using ST for effects (issue #24) #33

merged 11 commits into from
Mar 10, 2019

Conversation

Dretch
Copy link
Contributor

@Dretch Dretch commented Dec 28, 2018

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?

@Dretch
Split into immutable buffers and Effect/ST buffer types.
a4a7797
@Dretch
Move mutable buffer functions into a typeclass.
efcd6a4 
There are two instances of the typeclass supplied: one for Effect +
EffectBuffer and one for ST + STBuffer.

This makes it possible to write code that mutates buffers that can
run in either Effect or ST.

The functional dependencies on MutableBuffer go in both directions so
that the type-checker only needs to know _either_ the type of the buffer
or the type of the effect: this should make it easier to write generic
code without type assertions, but it does mean that both typeclass
instances must go in the same module (Node.Buffer.Mutable) in order to
avoid orphaned instances.
@hdgarrood
Copy link
Member

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

@hdgarrood
Copy link
Member

Fixes #24

hdgarrood
hdgarrood reviewed Dec 30, 2018
View reviewed changes
Copy link
Member

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/Node/Buffer/Mutable.purs Outdated Show resolved Hide resolved
src/Node/Buffer/Mutable/Unsafe.purs Outdated Show resolved Hide resolved
src/Node/Buffer/Mutable.purs Outdated Show resolved Hide resolved
Dretch added 8 commits January 5, 2019 13:17
@Dretch
Copy link
Contributor Author

Dretch commented Jan 13, 2019

@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
Copy link
Member

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
Copy link
Contributor Author

Dretch commented Jan 13, 2019 •
edited
Loading

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
Copy link
Member

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

@hdgarrood
Copy link
Member

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
hdgarrood approved these changes Jan 13, 2019
View reviewed changes
Copy link
Member

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much; I think this looks great.

@hdgarrood hdgarrood merged commit 7098df6 into purescript-node:master Mar 10, 2019
@hdgarrood hdgarrood mentioned this pull request Mar 10, 2019
Closed
@Dretch Dretch deleted the stbuffer-typeclass branch July 20, 2019 20:21
srghma added a commit to srghma/hyper that referenced this pull request Apr 17, 2020
@srghma
feat: update dependencies and fix "No type class instance was found"
bdc5672 
error after addition of MutableBuffer class

purescript-node/purescript-node-buffer#33

```
No type class instance was found for

Node.Buffer.Class.MutableBuffer t2
                                Effect

The instance head contains unknown type variables. Consider adding a type annotation.

while applying a function fromString
of type MutableBuffer t0 t1 => String -> Encoding -> t1 t0
to argument "Not Found"
while inferring the type of fromString "Not Found"
in value declaration serveFilesAndGet
```
srghma added a commit to srghma/hyper that referenced this pull request Oct 21, 2020
@srghma
feat: update dependencies and fix "No type class instance was found"
35f7830 
error after addition of MutableBuffer class

purescript-node/purescript-node-buffer#33

```
No type class instance was found for

Node.Buffer.Class.MutableBuffer t2
                                Effect

The instance head contains unknown type variables. Consider adding a type annotation.

while applying a function fromString
of type MutableBuffer t0 t1 => String -> Encoding -> t1 t0
to argument "Not Found"
while inferring the type of fromString "Not Found"
in value declaration serveFilesAndGet
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdgarrood hdgarrood hdgarrood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Dretch @hdgarrood