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

Amend sized literals to support deriving Show #596

Merged
merged 1 commit into from Jul 15, 2023

Conversation

monoidal
Copy link
Contributor

@monoidal monoidal commented Jun 15, 2023

GHC supports deriving Show for constructors containing fields of types {Int,Word}{8,16,32}#.

For example, given

data T = MkT Int8# deriving Show
x = show $ MkT (intToInt8# 42#)

we have x == "MkT (intToInt8# 42#)". This has been supported since the introduction of Int8# etc. in GHC 8.8.

This is rather clumsy. At that time, there was no better way of referring to a number of type Int8#. But since #451 we can form literals of those types directly: MkT 42#Int8.

I'm proposing to change deriving Show to use that syntax.

  • Show is supposed to use the normal form (literals and data constructors, no functions).
  • The current method doesn't support Int64#. A naive adaptation is wrong on 32-bit: think about intToInt64# BIG#. Using sized literals removes the Int# indirection and everything just works. Therefore, I'm also proposing to add support for Int64# and Word64#, which will work in the same way as {Int,Word}{8,16,32}#.

Deriving Show for other unboxed fields (Char#, Int# etc.) is unaffected.

This change is technically changing the behavior of GHC, so I'm submitting it as an amendement to sized literals. It is still minor and I don't expect much breakage: {Int,Word}{8,16,32}# are relatively new.

I have already implemented this change in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10673/diffs.

@nomeata
Copy link
Contributor

nomeata commented Jun 15, 2023

Convincing. I just hope that “I don't expect much breakage” is true not only technically but also socially…

@RyanGlScott
Copy link
Contributor

If I'm understanding your PR description correctly, you are proposing two distinct (but related) changes:

  1. Making derived Show instances use sized literal syntax.
  2. Extending deriving Show to support data types with Int64# or Word64# fields. (Currently, deriving Show only supports Int{8,16,32}# or Word{8,16,32}# fields.)

This PR amends the sized literals proposal to state change (1) explicitly, but is rather silent about change (2). Can you mention change (2) somewhere as well?

Relatedly, Int{8,16,32}# and Word{8,16,32}# also support derived Eq and Ord instances. Should deriving Eq and deriving Ord be extended to support Int64# and Word64# as well? (I apologize if this constitutes scope creep—I am just curious if these classes were considered.)

@monoidal
Copy link
Contributor Author

monoidal commented Jun 16, 2023

Can you mention change (2) somewhere as well?

I've added a mention to this PR.

Should deriving Eq and deriving Ord be extended to support Int64# and Word64# as well?

They already do, as of GHC 9.4 and above.

@RyanGlScott
Copy link
Contributor

They already do, as of GHC 9.4 and above.

Oops, I had incorrectly assumed that they didn't, since deriving Show wasn't supported. My mistake!

@monoidal
Copy link
Contributor Author

@nomeata I'm submitting, please review.

@nomeata
Copy link
Contributor

nomeata commented Jun 28, 2023

Thanks! I have more questions:
What about the Read instance? Will it be able to read back the changed show’ed format? Only the new one, or also the old one?

@nomeata
Copy link
Contributor

nomeata commented Jun 28, 2023

Also, since this has been around since 8.8, given recent discussions, I wouldn’t feel good about accepting this without checking back with the CLC folks and see if they want to be consulted.

@monoidal
Copy link
Contributor Author

monoidal commented Jun 28, 2023

There is no deriving Read for types containing unboxed fields. While we could add support, I consider that out of scope for this proposal.

Looking at the entire libraries directory in a GHC checkout (git grep --recurse-submodules -w Int8#), the only types having sized types as fields are:

  • {Int,Word}{8,16,32,64}, with a custom instance that prints the number without the hash (e.g. show (5 :: Int8) == "5")
  • StaticPtr, which does not have a Show instance
  • TyCon, which has a custom Show instance that does not display the integer fields

In particular, none of them derive Show.

@Bodigrim should CLC be involved?

@nomeata
Copy link
Contributor

nomeata commented Jun 29, 2023

I'm surprised you don't get a Read instance, but it certainly helps a lot here!

I'll shepherd this myself.

@nomeata nomeata added the Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion label Jun 29, 2023
@nomeata nomeata self-assigned this Jun 29, 2023
@nomeata nomeata added Pending committee review The committee needs to evaluate the proposal and make a decision and removed Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion labels Jun 29, 2023
@simonpj
Copy link
Contributor

simonpj commented Jun 29, 2023

@monoidal Our new protocol suggests that we should explicitly invite the CLC to comment.

In this case it seems so straightforward that the easiest thing is to open a CLC issue and ask for their approval. Would you like to do that, in parallel with getting GHC SC to agree?

@angerman
Copy link
Contributor

This change is technically changing the behavior of GHC, so I'm submitting it as an amendement to sized literals. It is still minor and I don't expect much breakage: {Int,Word}{8,16,32}# are relatively new.

So you do expect some breakage? What breakage do you expect?

After discussing this with @hsyl20 it appears there will be only breakage for golden tests, that relied on the old format?
What other breakage do we potentially look at?

@adamgundry
Copy link
Contributor

Can you mention change (2) somewhere as well?

I've added a mention to this PR.

@monoidal There still doesn't seem to be anything in the PR content about Int64#, should it be added?

Our new protocol suggests that we should explicitly invite the CLC to comment.

@simonpj Is there actually any change to libraries being proposed here? As far as I can see this change impacts deriving clauses only, and #596 (comment) indicates that no such deriving clauses appear in boot libraries. So I don't see why CLC approval is needed.

@nomeata
Copy link
Contributor

nomeata commented Jun 29, 2023

So I don't see why CLC approval is needed.

I tend to agree, but in light of recent discussions let's be polite and ask them if they see it that way too, or if they want to weigh in.

@simonpj
Copy link
Contributor

simonpj commented Jun 29, 2023

As far as I can see this change impacts deriving clauses only, and #596 (comment) indicates that no such deriving clauses appear in boot libraries.

Huh. I thought it changed the behaviour of a Show instance, e.g. Show Int8. But it doesn't -- I think that is special-cased not done via deriving. So yes perhaps you are right.

But as Joachim says, it's probably polite to give them a heads-up. Not a CLC proposal then; perhaps just an email to the committee pointing them to this thread?

@monoidal
Copy link
Contributor Author

I have submitted a CLC issue (link above).

So you do expect some breakage?

No. My expectation is: definitely no major breakage, likely no breakage at all. Of course I might be wrong - I have started a head.hackage build to get some confirmation.

What other breakage do we potentially look at?

If someone went through the effort of writing a Read instance by hand that parsed "MkT (intToInt8# 42#)", then their code would break. It is a remote possibility. I still doubt it. Unboxed types are usually used when speed is important, while Read is kind of the opposite.

There still doesn't seem to be anything in the PR content about Int64#, should it be added?

It's a bit implicit, but since the edited file is about sized literals of all types, this includes Int64#.

Really, imagine that we already had Int8#/Int16#/Int32# using extended literals. It'd be considered an oversight not to have the same support for Int64#, and I think GHC devs would just accept such a change as a trivial consistency fix.

@monoidal
Copy link
Contributor Author

I have started a head.hackage build to get some confirmation.

The build passed.

@nomeata nomeata added Accepted The committee has decided to accept the proposal and removed Pending committee review The committee needs to evaluate the proposal and make a decision labels Jul 15, 2023
@nomeata nomeata merged commit 440c373 into ghc-proposals:master Jul 15, 2023
@monoidal monoidal deleted the wip/sized-deriving branch July 17, 2023 10:46
@monoidal
Copy link
Contributor Author

This has been Implemented The proposal has been implemented and has hit GHC master in 9.8. @nomeata could you add the label?

@nomeata nomeata added the Implemented The proposal has been implemented and has hit GHC master label Jul 21, 2023
sthagen pushed a commit to sthagen/ghc-ghc that referenced this pull request Jul 23, 2023
This implements GHC proposal
ghc-proposals/ghc-proposals#596

Also add support for Int64# and Word64#; see testcase ShowPrim.
tbagrel1 pushed a commit to tweag/ghc that referenced this pull request Aug 1, 2023
This implements GHC proposal
ghc-proposals/ghc-proposals#596

Also add support for Int64# and Word64#; see testcase ShowPrim.

(cherry picked from commit 787bae9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted The committee has decided to accept the proposal Implemented The proposal has been implemented and has hit GHC master
Development

Successfully merging this pull request may close these issues.

None yet

6 participants