-
Notifications
You must be signed in to change notification settings - Fork 58
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.
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
Split off PrimMonadInternal haskell/vector#65 #19
Conversation
looks like the .travis formula needs to be updated to the modern @hvr style https://github.com/hvr/multi-ghc-travis/blob/master/.travis.yml could you swap it in? |
Swapped in new .travis.yml file, and rebased against newest commits as well. |
c92cb6d
to
b69456a
Compare
OK, after some tweaking, I applied a much smaller patch and got the build to pass on Travis. I was a bit surprised to notice that the test suites are not being run by Travis, is this intentional? |
no its not, add em! test all the things! :) |
Ahh, I see why the tests aren't included: I'm the only person who's written any tests, and I neglected to add testing to Travis. OK, let's see if this latest changes works... |
OK, tests are running and passing. Now that all of that is out of the way, I think we can discuss the actual change itself here. I'm up for any bikeshedding on the name of the new typeclass, but other than that I think our hands are tied on what can be changed, since we should leave all method names and signatures the same to avoid unnecessary breakage. |
hrm, how about we add something like I dont have a strong objection to the *Internal name, though it is kinda ugly for an exported name. I guess I'm mildly bothered by the fact that its still called PrimMonad, even though now its just for historical/compatibility reasons, but we've already established that this style design is needed if we want to minimize breakage. As written, i guess it looks fine, though it'd be good to have some feedback from a few more folks: |
oh, i'm noticing that this doesnt have the full multi-ghc travis formula btw, edit: oh did you push that already? |
I think
which may achieve much the same goal. (FWIW, I'm not a fan of the |
i dont think we need to do
we would be better served (at least while pre 7.10 support matters, because the above requires contraint kinds in the client module in pre 7.10 right?)
or something like that, I think? I personally favor World or the like over PrimState, because it is a world token right? |
Yeah, that seems fine to me. |
(though i do agree the UX of the type synonym approach IS better, so maybe that piece should be a bit more forward looking anyways?) |
I don't think I'm (edit) not very opposed to including it for (fairly) seamless replacement of monad-st if Ed thinks that's sensible, though. |
hrmm, you may have a good point though. |
I guess i'm just thinking PrimMonad is no longer a good/accurate name and I'm fishing for better ideas :) |
I'd rather not just start randomly recoloring the bikeshed and breaking every user for no real non-cosmetic reason. |
fair enough, i was just trying to think outloud. paint coloring ideas withdrawn. |
I think the only thing really open to bikeshedding then is the name of the new typeclass. Are there any objections to the current name (PrimMonadInternal) or suggestions for better ones? I don't feel strongly about the name at all, and am happy to have it changed. Otherwise, is there anything blocking this getting merged and released? |
I have some concerns about the interaction between nondeterminism and modules like http://hackage.haskell.org/package/vector-0.10.12.2/docs/Data-Vector-Generic-Mutable.html that I'd very much like to work through first. |
For example a MStream m a could never violate the linearity of the result vector before, but with this change it can, so far more of the API may wind up behind PrimMonadInternal than you might think. |
Re constraint kinded aliases, they currently require users to turn on extensions, which makes them less useful than the old school UndecidableInstance class + instance pair, but I don't think we'd even want to do that. |
We also need to add instances for the various types in transformers. Due to transformers having a hard Haskell 98 requirement they simply can't flow the other way. |
Also, even PriimMonadInternal is a bit stronger than most ops require, most things only need linear or affine computation, not isomorphism to ST s. |
Can you give an example of a problematic case? I think that might help solidify the discussion.
Can you clarify what you mean by this? Which instances need to be added? |
That was what I wanted to take the time to either reassure myself didn't exist, or find. ;) That said, let me try to construct one off the cuff: Consider a world where we have instance PrimMonad m => PrimMonad (ListT m) Now, lets look at transform :: (PrimMonad m, MVector v a) => (MStream m a -> MStream m a) -> v (PrimState m) a -> m (v (PrimState m) a)
transform f v = fill v (f (mstream v)) This will refill the vector after modifying it with the MStream calculation. Previously, Before, the fact that the 'future' of the calculation was linear, we could only ever 'fill' once, and each step we took filling it in was only going to be visited (at most) once, we could throw an exception in the modified stream, but that is it. But now in particular with Things like that are I'm not in a hurry to rush in and say "oh this more generalized form of PrimMonad is definitely okay" because the entire API of It seems obvious to me that monads that only use the future of the calculation in a linear (or affine) fashion are okay. It is the ones where we introduce a notion of continuation or non-determinism where it seems to me that we'll run into problems if we aren't careful.
It strikes me that the very point of allowing a more general type of Say what you will about |
So along these lines: @ekmett how do you feel about adding the PrimMonad instance for ExceptT in transformers-compat, instead of adding a dependency from primitive on transformers-compat? |
OK, it's pushed and Travis is passing. (I rebased slightly vs the previous set of commits to put the change under discussion as the last commit, and the other housekeeping stuff around Travis lower in the stack.) I think Edward still wanted to think about linearity a bit, but so far I'm not hearing anyone complain about the name PrimMonadInternal. |
I'm okay with tweaking transformers-compat to do the right dance to On Wed, Jan 7, 2015 at 1:56 PM, Michael Snoyman notifications@github.com
|
{-# INLINE primitive #-} | ||
instance PrimMonadInternal IO where | ||
internal (IO p) = p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this maybe be written as
internal = \(IO p)-> p
so that it has the same saturation/inlining behavior as other instances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops, nevermind, the ST instance is in the same style
I want to bikeshed a little. Another issue I want to bring up there's no way to constrain monad at the base of stack. Most importantly to specialize it to IO. It becomes problem if most of the library could use any PrimMonad but some need to do IO. Concrete example is mwc-random. PRNG initialization requires IO and rest of library doesn't. Only solution I can think of is to add |
For the latter, you can use I suppose we could include an alias for |
i'm inclined to agree with @dolio , (that those are easy to define in userland) |
It also has the disadvantage of not being able to be partially applied, On Tue, Jan 20, 2015 at 5:38 PM, dolio notifications@github.com wrote:
|
Er.. by partially applied I mean, 'left unapplied' as a type of kind (* -> On Tue, Jan 20, 2015 at 11:06 PM, Edward Kmett ekmett@gmail.com wrote:
|
@ekmett clarified what he meant on irc
which i agree with |
Yes. I agree that PrimState m ~ RealWorld is not best idea. It's also quite Any opinion on naming of PrimMonadInternal? |
i dont really care what we name it, but |
I don't particulary care either. I just think that PrimMonadInternal is |
we should probably also expose a pretty version of I think @dolio will do that a few other bits of cleanup and ship this soon (we talked through this and a few related action items at compose conf) |
It looks to me like this PR has officially stalled. Anything we should do to un-stall it? |
I'll talk to Ed tomorrow about any remaining objection he has. Other than that, there seems to be some dislike of the name. But I don't have a better one, and neither does anyone else, I think. Does anyone like the other two names Carter suggested better than the current one? Or have something better? Otherwise I'll probably just merge it. |
I like Carter's |
How does Don't bother revising the pull request. I'll just merge it and then we can rename on top of it. |
It's nice too
|
that works for me. |
No objection from me. On Wed, Feb 11, 2015, 7:50 PM Carter Tazio Schonwald <
|
Split off PrimMonadInternal haskell/vector#65
@cartazio By the way, I just realized that It might be difficult to notice that. Perhaps we should mention that in the comment/documentation? |
@dolio, YES, good point, we should probably document it! Would it be worth having some sort of |
but yeah, one way or another, we should probably make |
see haskell/vector#65