Fix UTF-16 autocompletion

[alexanderadam]

fixes #52

PS: I'm looking for a new adventure in case anybody is looking to hire or work with a Ruby/Rails/Crystal dev

[alexanderadam]

Thank you so much for your valuable advice @tompng 🙏

[alexanderadam]

It seems that I can't re-run the build?
Or at least I can't find how.
TruffleRuby failed on the CI while bundling and I created an issue for that on the TruffleRuby repo.

[eregon]

I schedule a rerun of that job, it's a known issue and I'm working on a fix for it.

[st0012]

Looking at the code, looks like the change is addressing the issue with utf16? Is that's the case, can you update both commit message and the PR title?

[alexanderadam]

Looking at the code, looks like the change is addressing the issue with utf16? Is that's the case, can you update both commit message and the PR title?

It is indeed 😆
I changed both now to be correct.

[tompng]

I think we don't need this. The original issue is about utf-16, but another realistic case is other ascii-compatible encoding method defined by code with magic comments.

#encoding: sjis
def a【codepoint=825C letter here】; end

So we don't need to consider UTF-16 specific case.
And It looks like BOM is not preserved.

'😄'.encode(Encoding::UTF_16)
# => "\xFE\xFF\xD8\x3D\xDE\x04" (has BOM)
'😄'.encode(Encoding::UTF_16).encode(Encoding::UTF_8).chars
# => ["😄"] (BOM is removed)

Other part of this pull request looks good 👍

[alexanderadam]

This is true for all tested Ruby implementations except TruffleRuby.

However, TruffleRuby adds the BOM in the beginning of the string after a conversion.

Benoit would rather see that Ruby would generally prevent such method definitions.

[tompng]

I see, but I don't think this should be in lib code.
Can you remove this and omit truffleruby in the added test, just like test_regexp_completor_handles_encoding_errors_gracefully does?

[alexanderadam]

I see. That makes sense. 🙂

Thank you, I changed it.

[eregon]

It would be better to use Encoding::UTF_16LE/BE for testing, Encoding::UTF_16 is basically deprecated and a dummy encoding.
Even better would be to use something more realistic than UTF-16, since that's not even valid source encoding: #52 (comment)
So I think testing with SJIS as in @tompng's example is much more realistic and useful, using UTF-16 is basically a user mistake in the first place.

[tompng]

Adding a realistic test case looks good. I'd like to check for some existing test that use UTF16LE, encoding-invalid method and add/change in a followup pull request.

[tompng]

Looks good 👍
Thank you