kowainik / eio

🎯 IO with Exceptions tracked on the type-level
Mozilla Public License 2.0
58 stars 2 forks source link

The list of exceptions can contain duplicates #12

Closed utdemir closed 3 years ago

utdemir commented 3 years ago

Thanks for the package, the interface looks great!

I have a quick question. When looking at the source code, I noticed that bind simply concatenates without checking for duplicates, as can be seen from the below code (based on your example):

foo :: EIO '[MyErr, MyErr] ()
foo = EIO.do
  EIO.throw MyErr
  EIO.throw MyErr
  EIO.return ()

This was unexpected for me initially, I would expect the type to be EIO '[MyErr] () in that case. I guess the likely argument is that catch deletes all occurences, so removing the duplicates are not necessary. Is this the case?

As an alternative, I tried a simple (but inefficient) change:

diff --git a/src/EIO.hs b/src/EIO.hs
index a747819..03dbfa8 100644
--- a/src/EIO.hs
+++ b/src/EIO.hs
@@ -90,7 +90,7 @@ combine thrown exceptions.
 type family (<>) (xs :: [Type]) (ys :: [Type]) :: [Type] where
     '[] <> ys = ys
     xs <> '[] = xs
-    (x ': xs) <> ys = x ': (xs <> ys)
+    (x ': xs) <> ys = x ': (xs <> Delete x ys)

 {- | Throw exception.

I think this increases readability a bit, since now we can write things like:

foo :: EIO '[MyErr1, MyErr2] ()
foo = EIO.do
  EIO.throw MyErr1
  EIO.throw MyErr2
  EIO.return ()

bar :: EIO '[MyErr2, MyErr1] ()
bar = EIO.do
  EIO.throw MyErr2
  EIO.throw MyErr1
  EIO.return ()

baz :: EIO '[MyErr1, MyErr2] ()
baz = EIO.do
  foo
  bar
  EIO.return ()

With the current type, baz's type would be EIO '[MyErr1, MyErr2, MyErr2, MyErr1] (), which is a mouthful.

What do you think?

chshersh commented 3 years ago

@utdemir The current behaviour that relies on simple lists is indeed suboptimal. A proper fix would be to use a type-level set, as proposed in #4.

Your solution with changing the definition of <> would also work, but I'm afraid it will be very slow at compile time.

utdemir commented 3 years ago

Thanks, I didn't see that issue. Looking forward to the solution!