Better error message from Data.Char.chr

[Bodigrim]

(Previously discussed at https://gitlab.haskell.org/ghc/ghc/-/issues/21468)

At the moment Data.Char.chr is implemented as follows:

chr :: Int -> Char
chr i@(I# i#)
 | isTrue# (int2Word# i# `leWord#` 0x10FFFF##) = C# (chr# i#)
 | otherwise
    = errorWithoutStackTrace ("Prelude.chr: bad argument: " ++ showSignedInt (I# 9#) i "")

The error message is terrible: chr actually is not exported from Prelude and it would be nice to explain why exactly the argument is bad (that's because it is outside of Unicode range). I propose to change it to:

 | otherwise
    = error ("Argument of Data.Char.chr is outside of the Unicode range 0..0x10FFFF: " ++ showSignedInt (I# 9#) i "")

This also switches errorWithoutStackTrace to error: even while there is no HasCallStack, let's at least not suppress a call stack.

[hasufell]

I guess this can break test suites. Not sure how much we care, but...

[noughtmare]

Two minor points:

1. If we print the range as hex then the value itself should also be shown as hex (or both as int).
2. The <qualified function>: <message> format is used in many more error messages in base. Should we really break consistency here?

So I would suggest:

    = error ("Data.Char.chr: argument is outside of the Unicode range 0..1114111: " ++ showSignedInt (I# 9#) i "")

(If a hex showing function is readily available then I would prefer that, but I couldn't find it quickly)

[noughtmare]

While looking for a hex showing function, I saw that intToDigit also refers to Char.intToDigit instead of Data.Char.intToDigit:

    | otherwise =  errorWithoutStackTrace ("Char.intToDigit: not a digit " ++ show (I# i))

I found more bad error messages related to succ maxBound:

ghci> succ maxBound :: Char
*** Exception: Prelude.Enum.Char.succ: bad argument
ghci> succ maxBound :: Int
*** Exception: Prelude.Enum.succ{Int}: tried to take `succ' of maxBound
ghci> succ maxBound :: Word
*** Exception: Enum.succ{Word}: tried to take `succ' of maxBound

All inconsistent, and I don't like any of them. I would suggest Prelude.succ: tried to take `succ' of `maxBound :: Int'

[tomjaguarpaw]

A bit of bikeshedding: can we slightly reduce the verbiage?

"Data.Char.chr: argument outside Unicode range: 0..1114111: "

[ChickenProp]

"Data.Char.chr: argument outside Unicode range: 0..1114111: "

This is my favorite so far.

More bikeshedding:

This also switches errorWithoutStackTrace to error: even while there is no HasCallStack, let's at least not suppress a call stack.

Does this mean the call stack will always have just one item pointing at the place chr is defined? If so I'd be inclined to leave it out, but if users can make it more useful then sure.

And, if the argument is negative do we want parens around it? I'd be inclined not to, so simply show or similar instead of showSignedInt (I# 9#). (Anyone know why the current version uses I# 9# instead of just 9?)

[Bodigrim]

I knew that bikeshedding error messages will jump start everyone ;)

And, if the argument is negative do we want parens around it? I'd be inclined not to, so simply show or similar instead of showSignedInt (I# 9#). (Anyone know why the current version uses I# 9# instead of just 9?)

GHC.Internal.GHC.Char (which is where chr is actually defined) is pretty deep in the module graph, I suppose we don't have class Show defined yet.

[Bodigrim]

I'm happy to adopt the wording of "Data.Char.chr: argument outside Unicode range: 0..1114111".

@TeofilC would you like me to prepare an MR for a vote?

[TeofilC]

Sounds good. Hopefully we've settled on error messages now

[Bodigrim]

Done: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/15815

[Bodigrim]

Unless there are more comments / suggestions, this is ready for a vote from my perspective.

[TeofilC]

Let's vote @velveteer @Daniel-Diaz @ChickenProp @noughtmare @mpscholten @doyougnu @parsonsmatt @jeltsch @rickowens

+1 from me

[velveteer]

+1