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

Export getSolo from Data.Tuple #113

Closed
treeowl opened this issue Dec 20, 2022 · 26 comments
Closed

Export getSolo from Data.Tuple #113

treeowl opened this issue Dec 20, 2022 · 26 comments
Labels
approved Approved by CLC vote base-4.19 Implemented in base-4.19 (GHC 9.8)

Comments

@treeowl
Copy link

treeowl commented Dec 20, 2022

GHC.Tuple provides a function

getSolo :: Solo a -> a
getSolo (Solo a) = a

Unfortunately, that's not available in Data.Tuple, so anyone who wants to use it must depend on ghc-prim. I propose to export it from Data.Tuple.

It may not be immediately obvious why this function is useful. After all, the point of Solo is generally to control evaluation in a fine-grained way through pattern matching. However, traversals provide a significant application. For example, one might want to write a function to map over a container strictly:

map' :: Traversable t => (a -> b) -> t a -> t b
map' f = getSolo . traverse (\a -> Solo $! f a)

All the "interesting" pattern matching on Solo is being done by its Applicative instance here, and we just want to get out at the end.

@Bodigrim
Copy link
Collaborator

Yep, fully in favor of it.

@tomjaguarpaw
Copy link
Member

+1

@Bodigrim
Copy link
Collaborator

Bodigrim commented Jan 6, 2023

@treeowl could you please raise an MR, so that we can proceed with a vote?

@Bodigrim Bodigrim added the awaits-MR No GHC MR (https://gitlab.haskell.org/ghc/ghc/-/merge_requests) has been raised yet label Feb 18, 2023
@treeowl
Copy link
Author

treeowl commented Mar 1, 2023

MR opened.

@treeowl
Copy link
Author

treeowl commented Mar 1, 2023

@Bodigrim Please trigger a vote.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Mar 1, 2023

Dear CLC members, let's vote on the proposal to export GHC.Tuple.getSolo :: Solo a -> a (which is in ghc-prim) from Data.Tuple (which is in base) as detailed in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10060/diffs.
@chshersh @hasufell @mixphix @tomjaguarpaw @parsonsmatt @angerman


+1 from me. As explained in the proposal, this is a generally useful function, and users should not reach out to ghc-prim (which is meant to be used by GHC only) to access it. It seems that getSolo was not re-exported from the get-go only by inadverent omission.

@parsonsmatt
Copy link

Since it appears to be a somewhat subtle distinction on matching on Solo a vs calling getSolo, I think that attaching some documentation to the function would be good if we're exposing it in base. The function currently doesn't have any Haddocks at all.

I'm also wondering if it's possible to give it a more suggestive name for the base export - one of the main ways people find things is Hoogle, and if they search Solo a -> a, it'd be Good if the name itself suggested somehow why you'd want to use it.

For example, the Solo docs say one potential use is this:

case arr `indexA` 12 of
    Solo a -> insert "hello" a m

One way to rewrite this would be:

insert "hello" (getSolo $ arr `indexA` 12) m

If I just have my "referential transparency" hat on, then I'd imagine these to be equivalent expressions - but I suspect that they're not. So perhaps some docs explaining when you'd want to use getSolo vs an explicit pattern match?


I am generally +1 on exposing it, since it seems like an oversight, but I'd also like to take the chance to potentially find a more illustrative name and/or provide some docs.

@konsumlamm
Copy link
Contributor

konsumlamm commented Mar 1, 2023

For example, the Solo docs say one potential use is this:

case arr `indexA` 12 of
    Solo a -> insert "hello" a m

One way to rewrite this would be:

insert "hello" (getSolo $ arr `indexA` 12) m

If I just have my "referential transparency" hat on, then I'd imagine these to be equivalent expressions - but I suspect that they're not. So perhaps some docs explaining when you'd want to use getSolo vs an explicit pattern match?

Yes, these two aren't equivalent w.r.t. laziness (the latter doesn't force the array access), but that's a general thing, nothing specific to getSolo, so I'd find it weird to document that for getSolo specifically.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Mar 1, 2023

@parsonsmatt just to be clear, are you happy to +1 the MR as is? Additional documentation if any can be added without CLC approval.

In general, CLC members are encouraged to discuss proposals before a vote is triggered.

@treeowl
Copy link
Author

treeowl commented Mar 1, 2023

I will add Haddocks. I think pointing out the laziness distinction is a good idea, given the purpose of Solo. I've generally wanted getSolo to get the final result out of a traversal.

@parsonsmatt
Copy link

I would like to hold base to a slightly higher standard of documentation than ghc-prim and other more "internal" packages - I think the commentary I'd want to see on the Haddocks has pretty much already been written in this thread, so it shouldn't be too much extra work.

In general, I think I'd be fine to +1 the MR as-is provided that there's a follow-up issue somewhere to add the documentation, if not actually adding the documentation then.

@treeowl
Copy link
Author

treeowl commented Mar 1, 2023

I'm sorry if I jumped the gun asking for a vote. I will push a documentation commit tonight.

@parsonsmatt
Copy link

Nothing to apologize for 😄 I'm just getting caught up on everything and would have made the same comment whether you'd requested a vote or not.

@hasufell
Copy link
Member

hasufell commented Mar 2, 2023

+1

1 similar comment
@mixphix
Copy link
Collaborator

mixphix commented Mar 2, 2023

+1

@angerman
Copy link

angerman commented Mar 2, 2023

While I am a bit concerned about increasing the surface area of base. I don't think that concern should outweigh the prospect of not having to reach for ghc-prim. So +1 from me.

@tomjaguarpaw
Copy link
Member

+1

@chshersh
Copy link
Member

chshersh commented Mar 2, 2023

+1


It's a non-breaking change, and since Solo is already exported, it makes sense to export getSolo as well

@treeowl
Copy link
Author

treeowl commented Mar 2, 2023

@parsonsmatt I've added a commit to document getSolo. Please comment on the MR if you have any suggestions. Would it be possible to move the definition of getSolo from ghc-prim to base in order to make hyperlinks work?

@phadej
Copy link

phadej commented Mar 2, 2023

Would it be possible to move the definition of getSolo from ghc-prim to base in order to make hyperlinks work?

https://hackage-search.serokell.io/?q=GHC.Tuple.*Solo

Surprisingly getSolo isn't used much. People only import Solo (..).

but technically that will be a breaking change, though not in base...

@phadej
Copy link

phadej commented Mar 2, 2023

Addition: And in long term it would be nicer if getSolo were an actual selector (it supposed to be, but that didn't work), but it can't be right now without changes in GHC, as Solo is wired in type. (https://gitlab.haskell.org/ghc/ghc/-/issues/20562 - apparently not easy to do).

@treeowl
Copy link
Author

treeowl commented Mar 2, 2023

Surprisingly getSolo isn't used much. People only import Solo (..).

That's not at all surprising when getSolo is only exported by GHC.Tuple. Packages generally avoid depending on ghc-prim when they can, as they should. Solo itself was only exposed through Data.Tuple rather recently, and the ecosystem hasn't really settled into it.

@phadej
Copy link

phadej commented Mar 2, 2023

@treeowl the search I made was GHC.Tuple.*Solo, i.e. to see what people import specifically from GHC.Tuple.

OTOH, Data.Tuple.*Solo https://hackage-search.serokell.io/?q=Data.Tuple.*Solo returns only false positives of import of Data.Tuple.Solo (the compat module from OneTuple package) which does indeed export getSolo.

@parsonsmatt
Copy link

+1 - thanks for the comment, that's very helpful!

@Bodigrim
Copy link
Collaborator

Bodigrim commented Mar 2, 2023

Thanks all, the proposal to re-export getSolo has been approved unanimously.

@treeowl if you want to do something else than re-export (e. g., redefine, or remove from ghc-prim) please raise a separate proposal (but I would advise against).

@Bodigrim Bodigrim closed this as completed Mar 2, 2023
@Bodigrim Bodigrim added approved Approved by CLC vote and removed awaits-MR No GHC MR (https://gitlab.haskell.org/ghc/ghc/-/merge_requests) has been raised yet labels Mar 2, 2023
ghc-mirror-bot pushed a commit to ghc/ghc that referenced this issue Mar 3, 2023
ghc-mirror-bot pushed a commit to ghc/ghc that referenced this issue Mar 3, 2023
ghc-mirror-bot pushed a commit to ghc/ghc that referenced this issue Mar 3, 2023
@chshersh
Copy link
Member

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field Value
Authors @treeowl
Status merged
base version 4.19.0.0
Merge Request (MR) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10060
Blocked by nothing
CHANGELOG entry present
Migration guide not needed

Please, let me know if you find any mistakes 🙂

@chshersh chshersh added the base-4.19 Implemented in base-4.19 (GHC 9.8) label Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.19 Implemented in base-4.19 (GHC 9.8)
Projects
None yet
Development

No branches or pull requests

10 participants