Skip to content

add unwrapF and unF #15

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

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

add unwrapF and unF #15

wants to merge 7 commits into from

Conversation

safareli
Copy link

fixes #10

@LiamGoodacre
Copy link
Member

This came up in slack just now. I mentioned that Invariant (imap unwrap wrap) might make more sense as the constraint because this is still applicable to contravariant functors. But having said that; we rarely seem to use Invariant, so perhaps having multiple declarations would be better - one for Functor and one for Contravariant?

@safareli
Copy link
Author

I see this unwrapF useful for structures which contain multiple values: Array, List, some other Tree structures, as otherwise you would need to iterate on all values and apply id to each of them.

All this are Functors. As I understand most Contravariant functors have a function underneath so f a "consumes" values of type a and usually they consume just one a so it's not that costly to cmap unwrap when you are going to run the unwrap on one value. I might be missing something tho, but if not, it means the unwrap specialized for Contravariant will be not that useful.

If you think it's worth adding tho what you think about the names? are this fine: unwrapCF and unCF ?

@garyb
Copy link
Member

garyb commented Apr 12, 2018

Since we're implementing this as a straight coerce, we could drop the constraint entirely too?

@safareli
Copy link
Author

safareli commented Apr 12, 2018

If we drop the constraint than it might be unsafe

@LiamGoodacre
Copy link
Member

How would it be unsafe?

@@ -33,6 +34,15 @@ class Newtype t a | t -> a where
un :: forall t a. Newtype t a => (a -> t) -> t -> a
un _ = unwrap

-- | Cheap version of `map unwrap`. Uses `unsafeCoerce` internally.
Copy link
Author

Choose a reason for hiding this comment

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

should we leave the map unwrap?

@safareli
Copy link
Author

Yes, it actually seems to be safe even for contravention functions

@eric-corumdigital
Copy link

eric-corumdigital commented Feb 20, 2019

There is no telling what a parameter is used for in the general case. Many libraries use unsafeCoerce themselves under the guard of carefully constructed types. The existence of wrapF and unwrapF without the Functor constraint means anyone can reach inside any type and wreak havoc.

@safareli
Copy link
Author

@eric-corumdigital there is wrap defined as part of class Newtype, but there is nowrapF. this pr is proposing addition of unwrapF an unF (note this functions have Newtype constraint). It should work fine for bot functors and contravariant functors. Can you point out some example where current definition of unWrapF can cause some issues?

@eric-corumdigital
Copy link

Yes, it works fine for Functors, but wrapF and unwrapF are not constrained to Functor, which is why there is a problem.

@safareli
Copy link
Author

.... which is why there is a problem.

But what exactly is a problem? Can you point out some concrete example where the problems of unWrapF (as it's defined now) are ilustrated?

@eric-corumdigital
Copy link

@safareli Data.Set.Set.

@kl0tl
Copy link
Member

kl0tl commented Nov 26, 2020

Now that we have safe coercions we could, in the spirit of #22, constrain unwrapF and unF with Coercible (f t) (f a) instead of Functor f and support both covariant and contravariant functors!

This would make the Newtype t a constraint redundant but we probably want to keep it to aid inference. Ideally we would have a Representational f constraint (given type Representational f = forall a b. Coercible (f a) (f b)) instead of Coercible (f t) (f a) but this requires constraint kinds and quantified constraints, we’re not there yet.

@JordanMartinez
Copy link
Contributor

#22 was merged, so this can move forward.

@hdgarrood
Copy link

Is this needed now that we have Coercible? I’d suggest letting people use coerce directly and being done with it.

@kl0tl
Copy link
Member

kl0tl commented Dec 27, 2020

Those functions could benefit from the functional dependency brought by the Newtype constraint, I’d be inclined to revisit this after 0.14 though: perhaps bare coerce will be enough for most cases and perhaps we’ll discover more helpful combinators for the rest.

@JordanMartinez
Copy link
Contributor

I've addressed the merge conflict. Are we still interested in adding this?

@JordanMartinez
Copy link
Contributor

CI is now failing with:

[1/1 NoInstanceFound] src/Data/Newtype.purs:62:11

  62  unwrapF = coerce
                ^^^^^^
  
  No type class instance was found for
  
    Prim.Coerce.Coercible (f0 t1)
                          (f0 a2)
  
  while checking that type forall (a :: Type) (b :: Type). Coercible @Type a b => a -> b
    is at least as general as type f0 t1 -> f0 a2
  while checking that expression coerce
    has type f0 t1 -> f0 a2
  in value declaration unwrapF
  
  where f0 is a rigid type variable
          bound at (line 62, column 11 - line 62, column 17)
        t1 is a rigid type variable
          bound at (line 62, column 11 - line 62, column 17)
        a2 is a rigid type variable
          bound at (line 62, column 11 - line 62, column 17)

           Src   Lib   All
Warnings   0     0     0  
Errors     1     0     1  

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.

Provide cheap coercions over functors
9 participants