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

Implement Data.List.Linear #189

Merged
merged 5 commits into from
Sep 25, 2020
Merged

Implement Data.List.Linear #189

merged 5 commits into from
Sep 25, 2020

Conversation

utdemir
Copy link
Contributor

@utdemir utdemir commented Sep 17, 2020

Closes #170, discussion at #126 .

This PR adds linear versions of the majority of Data.List and Data.Foldable functions, specializing them to work on lists. Most of them are implemented by unsafe coerces, and the ones requiring Consumable or Dupable typeclasses reimplemented. I hope I got the casts right (meaning, they aren't omitting or duplicating any elements without using the required typeclasses).

I also omitted a few methods from Data.List, if they

  • work on infinite lists, eg: iterate, repeat.
  • discard most of the elements, eg: head, find, null (or they require significant changes on the type signatures)
  • are partial, eg: scan1, foldl1

The latter two points are arbitrary decisions on my part, so let me know if you disagree and I can add them back. In my opinion, for those cases the user might find manually implementing them tailored to they needs easier. And we can always add them in future if someone requests them, or when we decide to.

It introduces a minor interface change:

-- before
foldl :: (b #-> a -> b) -> b #-> [a] -> b
-- after
foldl :: (b #-> a #-> b) -> b #-> [a] #-> b

Which, I think is not a major issue since the former can be derived from latter by wrapping list elements with Ur. I had to do this for one of our examples.

Similar to #174, it only copies over minimal amount of documentation from Data.List, to reduce the amount of duplication. Let me know if you prefer otherwise.

It also:

  • Moves forget to Prelude.Linear.Internal to break an import cycle.
  • It also adds Consumable instances for NonEmpty and Either, since they were necessary to reduce some duplication on zipping functions.
  • Rewrite Control.Monad.Linear.foldM with explicit recursion instead of foldr to break an import cycle (because now foldr is imported from Data.List.Linear instead of Prelude.Linear.Internal).

@aspiwack
Copy link
Member

  • work on infinite lists, eg: iterate, repeat.

There is nothing wrong with linear functions for infinite lists. Presumably they will eventually be used in an unrestricted context. But this is not an issue

  • discard most of the elements, eg: head, find, null (or they require significant changes on the type signatures)

It's hard to justify linear head and null as they become identical to the corresponding pattern matching. find might, but it's not clear what the actual definition ought to be. You should re-export the unrestricted versions of these functions, though.

  • are partial, eg: scan1, foldl1

There is nothing wrong with partial function (as long as they have HasCallStack 🙂 ), so let's have these.

@utdemir
Copy link
Contributor Author

utdemir commented Sep 22, 2020

Thanks @aspiwack , I applied your suggestions, with below exception:

It's hard to justify linear head and null as they become identical to the corresponding pattern matching. find might, but it's not clear what the actual definition ought to be. You should re-export the unrestricted versions of these functions, though.

I still think from an user perspective it would be surprising to import a linear module and get an unrestricted function, especially when there is a (admittedly not ideal) linear implementation too. So I tried to find a middle-ground, and implemented the linear versions:

head :: (HasCallStack, Consumable a) => [a] #-> a
tail :: (HasCallStack, Consumable a) => [a] #-> [a]
last :: (HasCallStack, Consumable a) => [a] #-> a
init :: (HasCallStack, Consumable a) => [a] #-> [a]

find :: Dupable a => (a #-> Bool) -> [a] #-> Maybe a

Especially find's implementation is pretty ugly; since for each element it has to first duplicate it, check one copy against the predicate, and then consume the other copy when the predicate fails.

I think this way things will be less surprising. Let me know if you still prefer the unrestricted versions instead of the above ones.

This commit adds linear versions of the majority of Data.List functions.

In order to do that, it also adds `Consumable` instances for `NonEmpty`
and `Either`.

It causes a minor interface change, from

```
foldl :: (b #-> a -> b) -> b #-> [a] -> b
```

to

```
foldl :: (b #-> a #-> b) -> b #-> [a] #-> b
```

Which is not a major issue since the former can be derived from latter
by wrapping list elements with `Ur`.
@utdemir utdemir force-pushed the ud/data-list-linear-specialized branch from 9e8f2b7 to dd0bb81 Compare September 22, 2020 09:59
@utdemir utdemir force-pushed the ud/data-list-linear-specialized branch from dd0bb81 to 9868fdc Compare September 22, 2020 10:02
@aspiwack
Copy link
Member

So I tried to find a middle-ground, and implemented the linear versions:

These are mostly useless though. Plus, they are not compatible with the corresponding non-linear version. Same as record projections: they require unrestricted arguments. Even in a linearly typed module. I think that these functions ought to as well. We shouldn't require programmers to import two modules just for the sake of having mixed linear and non-linear code (which is the norm in linearly typed modules anyway).

@utdemir
Copy link
Contributor Author

utdemir commented Sep 22, 2020

These are mostly useless though.

I agree.

We shouldn't require programmers to import two modules just for the sake of having mixed linear and non-linear code (which is the norm in linearly typed modules anyway).

I am not a fan of having to import multiple modules either. My omittance was mostly about providing a syntactic clue about whether something is working on linear or unrestricted values. Qualified imports are one way to achieve it, putting a suffix to the names would also work (but not pretty), or just having it absent from .Linear module is a strong clue that there is no linear version.

As you mentioned, whether a linear version of something is useful or not is a blurry line. Especially, for examples like takeWhile or dropWhile, I can think of them either way. So, I can imagine someone importing Data.List.Linear and assuming the head function to be :: Consumable a => [a] #-> a, and getting an error. It wouldn't even be clear that head is the issue, since the error messages point to the binding rather than the unrestricted use. So, it would take some cross-referencing docs to figure out one of the functions there is not linear on the parameter. I imagine that to be frustrating.

But, it is not a huge issue, and definitely not a hill I want to die on before the release. So I removed the linear implementations and re-exported the unrestricted ones from Data.List. We can always change our decision in the future.

@utdemir utdemir force-pushed the ud/data-list-linear-specialized branch from d430352 to 8575e3e Compare September 24, 2020 07:47
@Divesh-Otwani Divesh-Otwani self-requested a review September 24, 2020 18:54
Copy link
Contributor

@Divesh-Otwani Divesh-Otwani left a comment

Choose a reason for hiding this comment

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

Nice and clean code. Please make an issue to eventually remove the gratuitous uses of Unsafe in this module. We should only use Unsafe when there's a non-linear optimized implementation already in base that we want to copy.

, unzip3
) where

import qualified Unsafe.Linear as Unsafe
Copy link
Contributor

Choose a reason for hiding this comment

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

No.......... I had hoped this wasn't going to be here. But, it might all work out 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that currently this module isn't look very safe since we can not ensure that none of those unsafe casts actually drop/duplicate values.

However, Data.List has a lot of tricks to make things fast; and implementing them here would be more work. I think this matches your sentence here:

We should only use Unsafe when there's a non-linear optimized implementation already in base that we want to copy.

(Also, it is likely that the unsafeCoerce's there probably get in the way of many optimisations so I do not think this will be as fast as the Data.List even in this shape).

Copy link
Contributor

Choose a reason for hiding this comment

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

I might have looked at the wrong files but when I looked at what's underneath Data.List for some of the coerced functions I didn't find optimized versions. In any case, not something we need to worry about right now. I'm not worried about safety, since most of these look like they could have linear implementations. Still, we should have an issue to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I did not make that choice per-function basis. I just looked Data.List overall, saw some gnarly bits and decided to implement this module by coerces. You are probably right that some of the functions would be easy to duplicate here. I'll create an issue.

src/Prelude/Linear/Internal.hs Show resolved Hide resolved
leftovers `lseq` ret

-- | Same as 'zipWith', but returns the leftovers instead of consuming them.
zipWith' :: (a#->b#->c) -> [a] #-> [b] #-> ([c], Maybe (Either (NonEmpty a) (NonEmpty b)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice type.

@utdemir utdemir merged commit e43ae69 into master Sep 25, 2020
@utdemir utdemir deleted the ud/data-list-linear-specialized branch September 25, 2020 00:26
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.

Add Data.List or Data.Foldable
3 participants