Fix String.reverse/1 grapheme ordering around invalid utf8 bytes

[AlexGx]

Before

iex> "ab" <> <<255>> <> "cd"
<<97, 98, 255, 99, 100>>

iex> String.reverse("ab" <> <<255>> <> "cd")
<<98, 97, 255, 100, 99>> # "ba" <> <<255>> <> "dc"

After

iex> String.reverse("ab" <> <<255>> <> "cd")
<<100, 99, 255, 98, 97>> # "dc" <> <<255>> <> "ba"

[AlexGx]

Maybe regression after #11024

[josevalim]

Should we move them to the accumulator instead?

Suggested change

	do: do_reverse(:unicode_util.gc(rest), []) <> <<byte>> <> :unicode.characters_to_binary(acc)
	do: do_reverse(:unicode_util.gc(rest), [<<byte>>, acc])

[AlexGx]

[<<byte>>, acc]variant may produce a nested lists. And also this affects hot path (strings without invalid utf8), because needs rewrite to something like:

defp do_reverse([grapheme | rest], acc),
    # extra calls grapheme_to_binary, was: do_reverse(:unicode_util.gc(rest), [grapheme | acc])
    do: do_reverse(:unicode_util.gc(rest), [grapheme_to_binary(grapheme) | acc])

  defp do_reverse([], acc),
    # do: :unicode.characters_to_binary(acc)
    do: IO.iodata_to_binary(acc)

If this is what you mean, I will do benchmark...

[josevalim]

Nested lists are supported in Unicode characters to binary:

iex>  :unicode.characters_to_binary([[?j], ?o, <<?s>>, [[?é]]])
"josé"

But we should probably do this instead anyway:

Suggested change

	do: do_reverse(:unicode_util.gc(rest), []) <> <<byte>> <> :unicode.characters_to_binary(acc)
	do: do_reverse(:unicode_util.gc(rest), [<<byte>> | acc])

[AlexGx]

Im stuck, please help!

on OTP 29
iex> :unicode.characters_to_binary(["dc", [<<255>>, "b", "a"]])
{:error, "dc", [[<<255>>, "b", "a"]]}

[josevalim]

Ah, of course, the operation will fail due to the invalid byte.

[lukaszsamson]

Shouldn't this function raise ArgumentError on invalid byte instead of silently processing?

[AlexGx]

@lukaszsamson Then all functions in String module would have to raise, which would be a BC

[lukaszsamson]

String API assumes the inputs have been validated at boundary via String.valid?/1. I see no explicite handling of NULL bytes anywhere

[josevalim]

A null byte is actually valid UTF-8 encoding. About invalid encodings, see this section on the module docs: https://elixir.hexdocs.pm/String.html#module-self-synchronization

[josevalim]

💚 💙 💜 💛 ❤️