x/text/encoding: decoder aborts on some invalid encodings instead of using U+FFFD

[AsoSunag]

Hello,

I noticed that for some encodings (at least all the ones for Japanese, Korean, traditional and simplified Chinese) the Transform method of their respective decoder handle invalid character by failing the decoding. But for example the UTF8 decoder replace everything it does not know by the invalid one (\ufffd).

Wouldn't it be better to normalize all the decoders to replace unknown characters by the invalid one?

[AsoSunag]

I had not seen it before but it seems that this issue was already address in a code comment (https://github.com/golang/text/blob/c27e06ce2911df9af962d3325d66ce05b7502c32/encoding/encoding.go#L23) :

// TODO:
// - There seems to be some inconsistency in when decoders return errors
//   and when not. Also documentation seems to suggest they shouldn't return
//   errors at all (except for UTF-16).

So here is a list of all the encodings returning errors :

• EUC-JP
• ISO-2022-JP
• Shift_JS
• EUC-KR
• gb18030
• GBK
• HZ-GB-2312
• Big5
• UTF-16
• UTF-32

And from what I see here https://encoding.spec.whatwg.org/#error the default error management for all decoders should be doing replacement instead of fatal error by default (even for UTF-16).

[ALTree]

You have marked the issue as "proposal", but IMHO that's a bit of a stretch if there's a TODO in the code, you're actually asking to fix a known problem.

anyway cc @mpvl who owns x/text.

[AsoSunag]

Sorry my bad I saw the comment when I was having a second look around.

[mpvl]

@nigeltao Could you clarify why these decoders returned errors instead of substituting the replacement character?

[nigeltao]

In https://groups.google.com/d/msg/golang-nuts/pENT3i4zJYk/o35l8WmyBwAJ on 2015 December, I said:

I remember, a year or three ago, discussing whether or not Encoding transformers should return an error early, use a substitute character, or be configurable between the two. I can't remember the details, though.

I still can't remember why we return errors instead of substituting U+FFFD.

For the record, there's also an "Error Modes" CL proposal (since abandoned) at https://go-review.googlesource.com/#/c/8303/

But as I cannot seem to contact 2013-era-nigeltao, I'm OK with just always substituting U+FFFD and changing the implementations to match the documentation, as per the TODO.

[mpvl]

Then substituting U+FFFD without the option to do otherwise seems the best approach to me.

[gopherbot]

CL https://golang.org/cl/37316 mentions this issue.

[gopherbot]

CL https://golang.org/cl/37317 mentions this issue.

[gopherbot]

CL https://golang.org/cl/37319 mentions this issue.

[gopherbot]

CL https://golang.org/cl/37322 mentions this issue.

[gopherbot]

CL https://golang.org/cl/37324 mentions this issue.

[gopherbot]

CL https://golang.org/cl/37325 mentions this issue.

[mpvl]

As for UTF-16, it will return only an error if ExpectBOM is given. This mode is not used by either the ianaindex or htmlindex, and can easily be avoided by users if they do not wish an error in such circumstances. As this is just an opt-in error, and an error for which it is unclear what replacement would mean, I'm not altering UTF-16. The same holds for UTF-32.

[gopherbot]

CL https://golang.org/cl/37595 mentions this issue.

[mpvl]

All decoders should no longer return errors except for the opt-in error for UTF-16/32.