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

Add SmallBounded* type classes and related instances; Fixes #38 #39

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

Conversation

bbarker
Copy link

@bbarker bbarker commented Jul 10, 2019

For addressing #38; I've tested this on a personal project and it works as desired there with boundedEnumMaybe.

@garyb
Copy link
Member

garyb commented Jul 10, 2019

I think perhaps this would be better off as a separate library rather than including it here. I totally get the motivation for it, but I think it's a bit dodgy - even following the laws set out here, if there's a SmallEnum type that has a range of Int - 1 (let's call it LargeButStillSmall), and you have a Maybe (Maybe LargeButStillSmall) then there's still going to be trouble. Even if the law was Cardinality < 1000 I think this problem could still be arrived at when you start nesting types.

Even aside from deep nestings of SmallEnum, you could have a different type constructor where a exists in say 10 branches instead of 1 the way it does in Maybe, and with that the cardinality starts amplifying pretty quickly.

@bbarker
Copy link
Author

bbarker commented Jul 10, 2019

Thanks for the discussion @garyb, a few comments and questions:

  1. As a separate library, I think the issue is I can't generate instances of e.g. BoundedEnum, without using a Newtype wrapper which, IMO, really has an effect on all APIs that use it in a negative (though usually not unbearable) way. I actually tried this as a separate library and noticed that issue. It occurs to me that maybe allowing for explicit import/export of instances (much like Scala does with implicits) might solve the issue of not allowing orphan instances, but that is probably a discussion for elsewhere.
  1. I used the loosey goosey "much less than" notation: <<, so you really shouldn't be using this on sets of size Int - 1, or probably even sets of size floor(Int/2) (though for most cases that would probably be ok). Maybe that could be made more explicit in docstrings.

Even if the law was Cardinality < 1000 I think this problem could still be arrived at when you start nesting types.

Yes, exactly, though rate, it pops up, that's why I suggest <<; it implies some responsibility on the user's part, but I think the power it achieves is hopefully worth it.

Even aside from deep nestings of SmallEnum, you could have a different type constructor where a exists in say 10 branches instead of 1 the way it does in Maybe, and with that the cardinality starts amplifying pretty quickly.

This is a very good point; I had thought of this, and though it seems rare, I guess it could come up. Again with using << as a buffer.

I'm wondering if there's any way to do a compile-time check on Cardinality, but probably not (outside of unit tests, which could be advised). Still could be really cool to have that ability.

  1. I think you meant SmallBounded and not SmallEnum, but just to be sure ...

@bbarker
Copy link
Author

bbarker commented Jul 10, 2019

Regarding the compile-time check, this might be possible by using something like, https://github.com/danieljharvey/purescript-refined, though adding that dependency would likely be another point in the direction of doing this as a separate library.

For instance (half pseudo code):

- | A lawful subclass  of `Bounded` to denote
-- | Cardinality a << Cardinality Int
class Bounded a <= SmallBounded a

-- | A lawful subclass of `BoundedEnum` to denote
-- | Cardinality a << Cardinality Int
class BoundedEnum a <= SmallBoundedEnum a where
  sizeCheck :: LessThan (Size Int)

instance smallBoundedEnum :: (SmallBounded a, BoundedEnum a)
  => SmallBoundedEnum a where
    sizeCheck = refine $ unwrap cardinality

@ajnsit
Copy link

ajnsit commented Jul 11, 2019

I think perhaps this would be better off as a separate library rather than including it here. I totally get the motivation for it, but I think it's a bit dodgy - even following the laws set out here, if there's a SmallEnum type that has a range of Int - 1 (let's call it LargeButStillSmall), and you have a Maybe (Maybe LargeButStillSmall) then there's still going to be trouble. Even if the law was Cardinality < 1000 I think this problem could still be arrived at when you start nesting types.

I don't have an opinion on whether this should be a separate library, but I don't see how what you said is a problem. SmallBounded a does not imply SmallBounded (Maybe a). So you would have to manually define an instance SmallBounded (Maybe LargeButStillSmall), which is violating the law.

@garyb
Copy link
Member

garyb commented Jul 11, 2019

Ah yeah, you're right. My thing would only be true if there was an instance like SmallBounded a => SmallBounded (Maybe a).

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.

3 participants