XmlSource instances for bytestring incorrect for non-ascii

[hesselink]

The XmlSource instances for lazy and strict bytestrings are incorrect when applied to bytestrings representing something other than ASCII. For example:

>  uncons (UTF8.fromString "€")
Just ('\226',"\130\172")

This would probably be a lot better by using something like utf8-string or going through Text. That will be better than what it is now, even though it doesn't work for other encodings.

[yav]

The bytestring type is not intentded for representing non-ascii data. You can use the Text instance to do that: uncons ("€" :: Text) should work fine.

[hesselink]

It's not intended for ASCII data either, but for bytes. Perhaps the instance should be removed altogether?

[yav]

Well, they are intended to support the functionality in Data.ByteString.Char8, which does not introduce a separate type and simply reuses ordinary ByteStrings for its representation.

I agree that only supporting ASCII in an application is probably not a great idea, but ASCII is a valid encoding, and ByteString is a fine encoding for ASCII encoded data.

[hesselink]

But so is UTF-8, and it supports strictly more use cases than ASCII. Most libraries that support bytestring as a "text" type nowadays use UTF-8, so I think it's the path of least surprise, and it won't break any current users of the library.

[yav]

Currently the library supports processing text encoded in both ASCII, via ByteString, and UTF-8 via Text. If we were to change the ByteString instances, then we'd loose some functionality---namely the support for ASCII.

Working with text values represented as ByteString and trying to maintain the invariant that they are all in proper UTF-8 encoding seems like a bug waiting to happen.

Is there a reason to not use decodeUtf8 before parsing? I think it is supposed to even stream properly. Although, XML currently uses String in its representation, so it is very inefficient for large XML files anyway. This is something we should probably fix one day though.

I am not really convinced that we should change the behavior for ByteString, but I'll leave this open, so we can check for overhead of using Text, if one day XML becomes more efficient.

[hesselink]

How would it lose the support for ASCII? As far as I know, any valid ASCII byte sequence is also a valid UTF-8 byte sequence.

I think both paths I proposed (decoding via utf8-string, or via lazy Text) are streaming, so that shouldn't regress either.

[yav]

The point is that the XML library does not make choices about encoding, leaving the choice to the underlying type. This is because the library really has no way to know what is the correct encoding to use---while UTF-8 is popular indeed, not all data is represented in UTF8.

As I said, the ByteString instance is there to support Data.ByteString.Char8, not arbitrary sequences of bytes. This implements "extended ASCII" not just ASCII (i.e., all 255 characters, not just the first 127). Perhaps we should document this better.

[hesselink]

Hmm, ok. I think it's an odd choice to support "extended ASCII" (I assume you mean latin-1 or windows-1252? There are many ways to extend ASCII...) instead of UTF-8. But I guess if you document it, it's slightly less error-prone, though I still wish the Char8 bytestrings were a newtype. Right now these instances are just bugs waiting to happen (and escaping notice during development since you'll likely only test (low) ASCII). I still think assuming UTF-8 is the path of least surprise, but I can see that if some kind of high ASCII was explicitly supported in the past, then changing this would be a breaking change (without type errors), so perhaps documenting the behavior instead would be a better choice.

[NH002]

Hmm, ok. I think it's an odd choice to support "extended ASCII" (I assume you mean latin-1 or windows-1252? There are many ways to extend ASCII...) instead of UTF-8. But I guess if you document it, it's slightly less error-prone, though I still wish the Char8 bytestrings were a newtype. Right now these instances are just bugs waiting to happen (and escaping notice during development since you'll likely only test (low) ASCII). I still think assuming UTF-8 is the path of least surprise, but I can see that if some kind of high ASCII was explicitly supported in the past, then changing this would be a breaking change (without type errors), so perhaps documenting the behavior instead would be a better choice.

I second this. At least document that only ASCII is supported through ByteString.