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.

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

Heterogeneous array creation #109

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

treeowl
Copy link
Collaborator

@treeowl treeowl commented Mar 25, 2018

Create arbitrarily many arrays of arbitrarily many types from one
ST action.

Closes #103

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 25, 2018

Question: I made the heterogeneous creation functions take traversals that are polymorphic over their Applicatives. Is that necessary to ensure the safety of the unsafeFreeze calls, or would it be okay to specialize to ST s? The rest of the polymorphism seems to be necessary.

Create arbitrarily many arrays of arbitrarily many types from one
`ST` action.

Closes haskell#103
@treeowl treeowl mentioned this pull request Mar 27, 2018
@treeowl
Copy link
Collaborator Author

treeowl commented Mar 27, 2018

FYI, the higher-rank traversals are basically the ones from Rank2.Traversable in rank2classes and from Data.Comp.Multi.HTraversable in compdata. They're type-flipped from Data.Vinyl.Core.rtraverse in vinyl (we definitely don't want them flipped like that). The only difference is that the traversals can be type-changing. I don't think that likely lets anyone break things (though I haven't proven that), and it seems to be somewhat useful.

runSmallArrays m = runST $ m >>= traverse unsafeFreezeSmallArray

-- | Create arbitrarily many arrays that may have different types. For
-- a simpler but less general version, see 'runArrays'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should say "see runSmallArrays".

@andrewthad
Copy link
Collaborator

I cannot verify the soundness of this, but I like it. Also, it would be good to know the answer of your question about Applicative possibly being specialized to ST instead.

@andrewthad
Copy link
Collaborator

Also, I dislike the names with Het in them, but at the moment, I don’t have any better suggestions.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 29, 2018 via email

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

I, too, cannot offer an opinion on whether the extra polymorphism is necessary or not, but these seem like they're in the spirit of vector's createT, so I generally support them. (I also can't offer better naming suggestions.)

Two minor comments inline.

@@ -1,6 +1,9 @@
{-# LANGUAGE CPP, MagicHash, UnboxedTuples, DeriveDataTypeable, BangPatterns #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE TypeFamilies #-}
#if __GLASGOW_HASKELL__ >= 708
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be 706? (That's the first GHC version that reliably supported PolyKinds.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mistakenly though it didn't stabilize till later. Fixed!

@@ -7,6 +7,9 @@
{-# LANGUAGE DeriveDataTypeable #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE BangPatterns #-}
#if __GLASGOW_HASKELL__ >= 708
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, 706 here.

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 13, 2018

Using Data.Functor.Sum, I've convinced myself that the type change from t to u shouldn't be a problem. I'm still not so sure about whether we can have trouble with unlawful traversals in general. I think the very most we might be able to get away with is this:

runArraysHetOfThen
  :: Applicative q
  => (forall s1 s2.
       ((forall x. MutableArray s2 x -> Compose (ST s1) q (r x)) -> t (MutableArray s2) -> Compose (ST s1) q (u r)))
  -> (forall x. Array x -> q (r x))
  -> (forall s. ST s (t (MutableArray s)))
  -> q (u r)

This reveals almost everything to the traversal function, but doesn't give it quite enough to actually monkey with the MutableArrays itself (since it doesn't know it's working in the same state thread they live in).

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 14, 2018

I've pretty much convinced myself that these are safe, even generalized considerably. See my answer to my question on SO.

@RyanGlScott
Copy link
Member

Do you mind including a summary of the rationale in that SO post somewhere in the comments? Once that's included, I'd say this is ready to be merged.

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 15, 2018

@RyanGlScott, I added some notes, changed some types, and removed PolyKinds (I don't think it actually does anything useful here). Could you take another glance?

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 15, 2018

Oh, and a note I meant to leave earlier:

I've generalized the types of runArraysHetOf and runSmallArraysHetOf. Thanks to that generalization, runArraysHetOfThen and runSmallArraysHetOfThen can be implemented (efficiently) using them, so I've removed the latter two functions to reduce clutter. I've added runArraysOf and runSmallArraysOf for consistency and because not everyone who might want to supply a custom traversal will also want to figure out rank-2 traversals.

* Add notes.

* Improve some of the types.

* Drop `PolyKinds`. I added it because of unclear thinking, and
  I don't think any of this benefits meaningfully from having it.
@treeowl
Copy link
Collaborator Author

treeowl commented Apr 15, 2018

One more question: would runHetArraysOf roll more easily off the tongue?

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 15, 2018

I think it would; if other's don't disagree, I'll make that change.

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion on the names, so pick whichever sounds best to you.

@cartazio
Copy link
Contributor

cartazio commented Apr 15, 2018 via email

:: (forall s1 s2.
((forall x. MutableArray s1 x -> ST s2 (Array x)) -> t (mut s1) -> ST s2 u))
-- ^ A rank-2 traversal
-> (forall s. ST s ((t :: (* -> *) -> *) (mut s)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With PolyKinds, this can be t :: (j -> k) -> *. Maybe we can do that once we drop support for 7.4; until then I think it's probably a bit too much CPP for what it offers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Remember that mut doesn't necessarily have to be used at all; it's just there to make the type match that of htraverse. So if someone wants to use a container of MutableArray#, say, they can still do that with a bit of type trickery.)

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 15, 2018

One thing that's still missing is a way to produce Arrays, SmallArrays, and ByteArrays in one ST action. There's actually a pretty clean way to do that (using an abstract record of freezing functions), but I don't yet know how to make it extensible in an Olegian sense. ByteArray throws a bit of a wrench into most nice approaches, since its s parameter is its last rather than second to last. I think we can leave that as an open question for future research.

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 16, 2018

I deleted my last comment because it was a bit wrong, thankfully.

I've been thinking some more, and there is another heterogeneous option:

class Freezable m i | m -> i where
  type GetS m :: *
  uf :: m -> ST (GetS m) i

instance Freezable (MutableByteArray s) ByteArray where
  type GetS (MutableByteArray s) = s
  uf = unsafeFreezeByteArray

instance a ~ b => Freezable (MutableArray s a) (Array b) where
  type GetS (MutableArray s a) = s
  uf = unsafeFreezeArray

instance a ~ b => Freezable (SmallMutableArray s a) (SmallArray b) where
  type GetS (SmallMutableArray s a) = s
  uf = unsafeFreezeSmallArray

runVariousHetArraysOf
  :: (forall s1 s2.
       ((forall m i. (Freezable m i, GetS m ~ s1) => m -> ST s2 i) -> t (mut s1) -> ST s2 u))
  -> (forall s. ST s ((t :: k -> *) (mut s)))
  -> u
runVariousHetArraysOf trav m = runST $ m >>= trav uf

The nice thing is that you can run one ST action and get all sorts of different results all at once: Arrays, SmallArrays, ByteArrays, etc. Since the Freezable class is open, it could easily be instantiated for types living in other packages.

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 19, 2018

Aha! I just realized something. If I modify runArraysOf very slightly, I can actually implement runHetArraysOf on top of it! Specifically, if I change the type signature of runArraysOf to

runArraysOf
  :: (forall s1 s2.
       (forall x. MutableArray s1 x -> ST s2 (Array x)) -> t (mut s1 a) -> ST s2 u)
  -> (forall s. ST s (t (mut s a)))
  -> u

then I can write

runHetArraysOf trav m = runArraysOf (\f -> trav f . unT) $ T <$> m

newtype T (t :: (* -> *) -> *) (msa :: *) = T {unT :: t (Fun msa)}
type family Fun x where
  Fun (f x) = f

@cartazio
Copy link
Contributor

cartazio commented Apr 19, 2018 via email

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 19, 2018 via email

@treeowl
Copy link
Collaborator Author

treeowl commented Apr 19, 2018 via email

@andrewthad
Copy link
Collaborator

In my mind, runArrays is clearly useful and should be included. I doubt that I personally would ever remember to use anything beyond runArrays. I would probably forget and just do it without the combinators. Here are my general documentation-related concerns:

  • What is a rank-2 traversal? Are there laws that such a function must satisfy? A google search for "haskell rank 2 traversal" turns up nothing relevant. The docs should probably link to resources or other haskell libraries.
  • An example of how to use runHetArraysOf is provided. This is good. But, is this function intended to be used with functions from some other library? Are the arguments taken in a particular order for a particular reason? Is this intended to be used with lens or something like lens? If so, it would be nice to include the lens-type-synonym translation of the type signature in the docs.

My other concern is that we have the following implementations:

runArraysOf trav m = runST $ m >>= trav unsafeFreezeArray
runHetArraysOf trav m = runST $ m >>= trav unsafeFreezeArray

These two functions have identical implementations and slightly different type signatures. It bothers me that the type signatures of these two cannot somehow be unified, but there seems to be no way around this.

Also, there's a annoying asymmetry going on. We don't have a runHetArrays function (with a TraversableRank2 typeclass constraint or whatever the appropriate typeclass is named). This makes me more inclined to say that runHetArraysOf belongs in some other package, but it does seem to be pretty useful in the example given, so I guess I like it here.

@cartazio
Copy link
Contributor

cartazio commented Apr 20, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants