Surprising behavior of ByteString literals via IsString

[parsonsmatt]

At work, we discovered a somewhat surprising behavior of ByteString's IsString instance and interaction with OverloadedStrings.

The following REPL session demonstrates the issue:

λ> BS.unpack $ T.encodeUtf8 ("bla語" :: Text)
[98,108,97,232,170,158]
λ> BS.unpack $ ("bla語" :: BS.ByteString)
[98,108,97,158]
λ> T.decodeUtf8 $ ("bla語" :: BS.ByteString)
*** Exception: Cannot decode byte '\x9e': Data.Text.Internal.Encoding.decodeUtf8: Invalid UTF-8

The IsString instance calls packChars which calls c2w, which silently truncates the bytes.

I'd be happy to put together a PR to document the behavior of the IsString instance.

I think I expected it to encode the string using the source encoding. I don't know whether or not that's a feasible or desirable change.

[hvr]

The behavior you're experiencing is documented at http://hackage.haskell.org/package/bytestring-0.10.8.2/docs/Data-ByteString-Char8.html

However, it may make sense to attach a docstring to the IsString instance since the IsString instance transcends module barriers.

[chris-martin]

It seems unusual to me that packChars is the basis for fromString yet is not part of the API. I think packChars ought to be exported, and the IsString instance haddock can just link to the packChars doc, and the packChars doc should explain what it does.

[hvr]

I don't understand the rationale of having to export packChars if we already have access to it via other means (i.e. fromString & pack) which alias it.

[chris-martin]

Ah, sorry, I didn't notice the pack function in the Char8 module. Yeah, the important thing is for the IsString instance doc to have a link to Char8.pack.

[sjakobi]

This issue is still confusing users today: https://stackoverflow.com/questions/58777439/haskell-data-yaml-utf-8-decoding

[joeyh]

This is a landmine to anyone using RawFilePath. Consider:

{-# LANGUAGE OverloadedStrings #-}
import System.Posix.Directory.ByteString
main = removeDirectory "bla語"

I agree that this seems to assume the source encoding will be used.

[sjakobi]

Since the IsString instance is still confusing both beginners and experienced users, how about removing it?

[hvr]

@sjakobi Since this instance has been in place for as long as I can remember, it'd be interesting in order to inform a decision to know how many packages would be affected (and also whether those packages were in full compliance with the PVP by having the mandated upper bounds in place) on Hackage if this instance was removed in a future release of bytestring.

[sjakobi]

@hvr I'm sure that a removal would cause tons of upgrade hassle.

Maybe it would be better to change the instance to UTF8-encode the input – that should limit any "breakage" to code that intentionally or more likely unintentionally triggered the current truncating behaviour.

[hasufell]

I think the instance should be removed. It isn't well defined. Carrying such pitfalls around for backwards compatibility reasons isn't what the haskell ecosystem should be about in my opinion. It should be about correctness.

We could mark it deprecated and remove it in 1 year.

[sjakobi]

Yet another alternative to removal: Raise an error if any Char in the input would be truncated. IMO that's better than truncating silently, but runtime errors aren't that great either…

[parsonsmatt]

Yet another alternative to removal: Raise an error if any Char in the input would be truncated. IMO that's better than truncating silently, but runtime errors aren't that great either…

As much as I'd like for ""bla語" :: ByteString to be a compile-time error, I'd rather have a runtime error telling me why something is wrong rather than having this fact stick around in the back of my head, and I'd certainly rather have received the error instead of investigating and discovering this 😄

I'm in favor of removing the instance over a long-enough timeline. With qq-literals and the statically checked overloaded strings trick, the fix could be as simple as inserting the relevant quasiquoter ([bs| ... |]) or quotation $$(...) (or even $$"..." in 8.12!). This is an annoyance but would dramatically improve safety.

In stages:

1. Replace the fromString function with one that throws a runtime error in the next major version bump.
2. Add a warning to the IsString instance in the next major version bump with migration instructions
3. Add a TypeError to the IsString instance in the next major version bump with instructions on how to migrate away.

[sjakobi]

@parsonsmatt I really like the idea of offering quasiquoters as an alternative to literals as a first step. The $$(...) and $$"..." quotations seem slightly less readable to me.

bs and lbs seem like good names to me. Does everyone agree or have better suggestions?