search

serokell / universum

:milky_way: Prelude written in @Serokell
MIT License
176 stars 26 forks source link

Add `FromList` #238

Closed Martoon-00 closed 3 years ago

Martoon-00 commented 3 years ago

Description

Problem: we lack a generic function for constructing an object from list, while toList is present.

Solution: add fromList.

The sad point here is that we cannot define it opposite to toList because for e.g. Map our toList returns only values, unfortunatelly. (Maybe this is worth changing one day?)

So fromList is opposite to something intermediate between toList and toPairs, and so I put it to a separate typeclass.

Related issues(s)

✓ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this checkmark indicating that you are sure it is dealt with (be that by irrelevance).

Related changes (conditional)

Stylistic guide (mandatory)

Martoon-00 commented 3 years ago

We have too many functions for conversion to lists, I mean toPairs, keys, elems, so the situation with them already looks somewhat complicated to me and I think we need a really good reason to change toList and this one is not sufficiently good. So I would personally just accept that fromList is not always the opposite to toList.

Thanks for your opinion here, I agree.

Another (potentially weird) idea that comes to my mind is to have no fromList for maps and instead have fromPairs in ToPairs class shrug But fromList is a standard name while fromPairs is not, so it's probably not a good idea.

Probably this would make sense. I think fromList should be implemented for all list-like types including maps (just because it can be, and not to require extra mental efforts in picking the right method), and then we can also have fromPairs for better symmetry, and this fromPairs will be identical to fromList but defined only for maps.

However with this approach we get some redundancy, there will be two ways to do the same thing. But that is probably minor in this case.

UPD: I wrote some nonsense initially, fixed now.

Martoon-00 commented 3 years ago

Oops, I've pressed some wrong button

dcastro commented 2 years ago

We're now upgrading morley to LTS-19.3 and, as part of that, universum 1.7.3, which introduces FromList (see here for context: https://gitlab.com/morley-framework/morley/-/merge_requests/1114#note_910547695).

I have a couple of questions:

  1. How is this sufficiently different from base's GHC.Exts.IsList? Can't we just re-export that?

    • Most of our FromList implementations default to using IsList anyway.
    • And those that were manually written, seem like they could have used the default implementation as well.
      • ZipList has had an IsList instance since base-4.15.0.0 (aka GHC 9)
      • NonEmpty has always had an IsList instance
      • ByteString has had an IsList instance since bytestring-0.10.12.0 (that goes back to at least LTS 17.15, I didn't check earlier LTSs)

  2. If this is sufficiently different from IsList (and therefore worth having), then we should not have a FromList instance for NonEmpty as it is inherently unsafe.

Martoon-00 commented 2 years ago
  1. The new fromList really mostly duplicates GHC.Exts.IsList.fromList. The main problem with GHC.Exts.IsList is that it is not very inconsistent with the model provided by universum.

In univerum we have Container typeclass that has its own toList, and at the user's side it would be weird to use toList from Prelude and completely different fromList from another place.

Moreover, if I need to have fromList for my custom type, I will have to define the entire IsList mostly duplicating Container instance this time. And this is real UX issue.

  1. At that moment, omitting this instance altogether seemed like a too big step for me (migrating to a sufficiently different fromList would be harder). But this is worth discussing, and I rather agree on deleting the instance than not.

I was also thinking about the ways to leave the instance but make it more safe, but didn't manage to come up with something worthy.

Created #264 for this.

dcastro commented 2 years ago

In univerum we have Container typeclass that has its own toList, and at the user's side it would be weird to use toList from Prelude and completely different fromList from another place.

I don't see how it would be weird, if both were exported from Universum.

Moreover, if I need to have fromList for my custom type, I will have to define the entire IsList mostly duplicating Container instance this time. And this is real UX issue.

I see your point, but I personally don't think that's enough to outweigh a bigger downside: the ecosystem has great support for IsList. Lots of libraries provide an IsList instance for their datatypes (off the top of my head, Bimap is a good example), whereas the FromList we're introducing here has virtually no support. We can't possibly provide FromList instances for all those types, nor is it reasonable for us to expect other library authors to do it. Which means it'll fall on universum users to define a lot of FromList instances, potentially orphans.

A yet bigger problem is that the OverloadedLists extension specifically only works on types with an IsList instance. Which means, in practice, when defining custom collection types, users might actually need to sometimes define BOTH a FromList instance (for universum compatibility) AND an IsList instance (for OverloadedLists compat).

All in all, I don't think having to write one extra redundant line is worth these 2 downsides:

instance IsList (MyList a) where
  type Item (MyList a) = a
  toList = Universum.toList -- <-- unfortunately redundant :/
  fromList = myFromListImpl
Martoon-00 commented 2 years ago

I see your point, but I personally don't think that's enough to outweigh a bigger downside: the ecosystem has great support for IsList.

Maybe yes, but if we followed this reasoning, we wouldn't introduce Container with its custom toList (and many other methods) too. Presence of Container produces mostly all the same problems as the newly introduced FromList.

Probably the scope of this issue is larger, and we should choose between Container + FromList and plain old typeclasses like Foldable + IsList. The current model naturally required custom FromList, as to me.

A yet bigger problem is that the OverloadedLists extension specifically only works on types with an IsList instance.

Oh, this is an issue I didn't fully accounted for. Somehow I supposed that another fromList would be picked automatically. And even RebindableSyntax won't save the user now, as nowadays [1..3] desugars to fromListN, not fromList.

But nevertheless, the use of OverloadedLists is arguable then as it relies on a function that is inherently unsafe, taking instance FromList NonEmpty into account. So this also goes against the model we take in universum with all its dancing around making unsafety mandatorily explicit.

All in all, I don't think having to write one extra redundant line is worth these 2 downsides:

By the way, considering your example, if we add a deriving via helper that delegates IsList implementation to Container.toList and FromList.fromList, that would be not less concise than the code in your snippet. Does it make sense :thinking:

Also, thinking about it, maybe it's worth considering changing the name of fromList to indicate that it comes from a different typeclass? Though fromList and toList usually come paired, and renaming only one of them would be not good IMO.