gleam-lang / stdlib

🎁 Gleam's standard library
https://hexdocs.pm/gleam_stdlib/
Apache License 2.0
502 stars 178 forks source link

On JavaScript, string.replace \r splits \r\n graphemes #584

Open richard-viney opened 7 months ago

richard-viney commented 7 months ago

I was expecting string.replace to treat control characters like any other code points, but then ran into this behaviour which looks wrong.

The issue is only on the Erlang target, works fine under JS.

I can dig a bit deeper if it would be helpful, possible that other control characters are affected too.

import gleam/bit_array
import gleam/string
import gleeunit
import gleeunit/should

pub fn string_replace_test() {
  // Check \r\n binary representation is as expected
  "\r\n"
  |> bit_array.from_string
  |> bit_array.inspect
  |> should.equal("<<13, 10>>")

  // Test replacing \n with a space character (U+0020)
  "\r\n"
  |> string.replace("\n", " ")
  |> bit_array.from_string
  |> bit_array.inspect
  |> should.equal("<<13, 32>>")

  // Test replacing \r with a space character (U+0020) fails with:
  //
  //   Values were not equal
  //   expected: "<<32, 10>>"
  //   got: "<<13, 10>>"
  //
  "\r\n"
  |> string.replace("\r", " ")
  |> bit_array.from_string
  |> bit_array.inspect
  |> should.equal("<<32, 10>>")
}
lpil commented 7 months ago

Thank you

mooreryan commented 7 months ago

This is a bit subtle, but as far as I can tell from skimming through the Erlang/OTP string module is that the functions are treating the \r\n as a single grapheme cluster.

First, check out string:length which returns the number of grapheme clusters the given string.

12> string:length("\r").
1
13> string:length("\r\n").
1
14> string:length("\n").
1
15> string:length("\n\n").
2
16> string:length("\r\r").
2

For trying to use the "\r" as the needle to replace in the haystack "\r\n", here is my understanding from following along with the code to see why \r will not work like the \n or \r\n.

1> [Head|Tail] = "\r\n".
"\r\n"
2> Head.
13
3> Tail.
"\n"

So, (as far as I can tell from going through the Erlang/OTP code right now), that's how the \r\n can match, and the \n can match, but not the \r by itself.

richard-viney commented 6 months ago

Yep agreed it's due to the \r\n being treated as a grapheme cluster, this is mentioned in several places including here: https://www.erlang.org/doc/man/string#trim-3:

"Notice that [$\r,$\n] is one grapheme cluster according to the Unicode Standard."

So the question is which of JS or Erlang is the desired/intended behaviour? Should it replace by codepoint or by grapheme cluster 🤔.

lpil commented 6 months ago

Sounds like the JS implementation is incorrect.

Pi-Cla commented 6 months ago

Erlang's splitting behaviour seems inconsistent though Currently when you call the split function for either the Erlang or Javascript target we end up with this:

import gleam/should
import gleam/string

pub fn split_newline_test() {
  string.split("\r\n","\n")
  |> should.equal(["\r",""])
}

Meaning Erlang just like Javascript is perfectly ok with removing the \n from a \r\n grapheme cluster.

However Erlang is not ok with

import gleam/should
import gleam/string

pub fn split_newline_test() {
  string.split("\r\n","\r")
  |> should.equal(["","\n"])
}

whereas Javascript is.

Pi-Cla commented 6 months ago

I would have expected Erlang to reject attempting to split a \n on a \r\n cluster if it was always treating \r\n as a separate thing from either \r or \n

Pi-Cla commented 6 months ago

So if we want to fully copy what Erlang does we need to ensure Javascript still DOES split on \n which is a bit odd...

Edit: The most "correct" behaviour to me would be either:

1) Make \r\n split on neither \r nor \n or 2) Treat \r\n as \n so that when you split on \n it removes the entire cluster \r\n

import gleam/should
import gleam/string

pub fn split_newline_test() {
  string.split("thing\r\nthing","\n")
  |> should.equal(["thing","thing"])
}

Both sequences are semantically the same in most cases and leaving a dangling \r would be very odd to me.