Repository: artichoke/artichoke (rust)

Issue:
`String#==` does not work the same way as MRI expects
Generated from https://github.com/artichoke/artichoke/pull/2182#issuecomment-1257154057

Ruby docs ref: https://ruby-doc.org/core-3.1.2/String.html#method-i-3D-3D. Relevant part:

> Returns true if object has the **same length** and content; as self; false otherwise.

And it looks like we're currently just comparing the internal `bytes` at present:
- [spinoso-string](https://github.com/artichoke/artichoke/blob/2db53033309c3e4f923fdea960d0884c4b47d1c8/spinoso-string/src/eq.rs)
- [artichoke-backend](https://github.com/artichoke/artichoke/blob/trunk/artichoke-backend/src/extn/core/string/trampoline.rs#L315-L333)

From MRI:

```console
[3.1.2]> "A" == "A"
=> true
[3.1.2]> "A".b == "A"
=> true
[3.1.2]> "\u{1F600}" == "\u{1F600}"
=> true
[3.1.2]> "\u{1F600}".b == "\u{1F600}"
=> false
```

Artichoke:
```console
=> true
>>> "A" == "A"
=> true
>>> "A".b == "A"
=> true
>>> "\u{1F600}" == "\u{1F600}"
=> true
>>> "\u{1F600}".b == "\u{1F600}"
=> true
```

## Solutions ideas

Rust psuedo-ish code change:

```rust
self.as_bytes() == other.as_bytes() && self.len() == other.len()
```

:point_up: I imagine `len()` would be fine since it is encoding aware, and should return the number of codepoints vs. bytes in the above.

The question I have is, should it be in `spinoso-string`, or in `string/trampoline.rs`. `spinoso-string` does mention `The Ruby String class`, which means we could assume that RStrings should do equality in this way - however it feels a little bit unexpected/hidden.

Pull request description:
Change PartialEq for EncodedString
Fixes #2193 

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 069c00916..a8b924255 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -818,7 +818,7 @@ dependencies = [
 
 [[package]]
 name = "spinoso-string"
-version = "0.20.0"
+version = "0.21.0"
 dependencies = [
  "bstr",
  "bytecount",
diff --git a/artichoke-backend/Cargo.toml b/artichoke-backend/Cargo.toml
index 5dff34012..3a127bcb8 100644
--- a/artichoke-backend/Cargo.toml
+++ b/artichoke-backend/Cargo.toml
@@ -34,7 +34,7 @@ spinoso-math = { version = "0.3.0", path = "../spinoso-math", optional = true, d
 spinoso-random = { version = "0.3.0", path = "../spinoso-random", optional = true }
 spinoso-regexp = { version = "0.4.0", path = "../spinoso-regexp", optional = true, default-features = false }
 spinoso-securerandom = { version = "0.2.0", path = "../spinoso-securerandom", optional = true }
-spinoso-string = { version = "0.20.0", path = "../spinoso-string", features = ["always-nul-terminated-c-string-compat"] }
+spinoso-string = { version = "0.21.0", path = "../spinoso-string", features = ["always-nul-terminated-c-string-compat"] }
 spinoso-symbol = { version = "0.3.0", path = "../spinoso-symbol" }
 spinoso-time = { version = "0.7.1", path = "../spinoso-time", features = ["tzrs"], default-features = false, optional = true }
 
diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock
index b77ebaac4..fca40f491 100644
--- a/fuzz/Cargo.lock
+++ b/fuzz/Cargo.lock
@@ -543,7 +543,7 @@ dependencies = [
 
 [[package]]
 name = "spinoso-string"
-version = "0.20.0"
+version = "0.21.0"
 dependencies = [
  "bstr",
  "bytecount",
diff --git a/spec-runner/Cargo.lock b/spec-runner/Cargo.lock
index 02a820d1b..b986d33ba 100644
--- a/spec-runner/Cargo.lock
+++ b/spec-runner/Cargo.lock
@@ -982,7 +982,7 @@ dependencies = [
 
 [[package]]
 name = "spinoso-string"
-version = "0.20.0"
+version = "0.21.0"
 dependencies = [
  "bstr",
  "bytecount",
diff --git a/spinoso-string/Cargo.toml b/spinoso-string/Cargo.toml
index aa3d00775..045841da3 100644
--- a/spinoso-string/Cargo.toml
+++ b/spinoso-string/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "spinoso-string"
-version = "0.20.0"
+version = "0.21.0"
 authors = ["Ryan Lopopolo <rjl@hyperbo.la>"]
 description = """
 Encoding-aware string implementation for Ruby String core type in Artichoke Ruby
diff --git a/spinoso-string/README.md b/spinoso-string/README.md
index bddacd9d4..a4e960155 100644
--- a/spinoso-string/README.md
+++ b/spinoso-string/README.md
@@ -27,7 +27,7 @@ Add this to your `Cargo.toml`:
 
 ```toml
 [dependencies]
-spinoso-string = "0.20.0"
+spinoso-string = "0.21.0"
 ```
 
 ## `no_std`
diff --git a/spinoso-string/src/enc/mod.rs b/spinoso-string/src/enc/mod.rs
index f152a7856..965c7c6bf 100644
--- a/spinoso-string/src/enc/mod.rs
+++ b/spinoso-string/src/enc/mod.rs
@@ -69,7 +69,13 @@ impl PartialEq for EncodedString {
         // [3.0.2] > s == t
         // => true
         // ```
-        *self.as_slice() == *other.as_slice()
+        //
+        // See the functional tests in `string_test.rb` for more cases.
+        match (self.encoding(), other.encoding()) {
+            (self_enc, other_enc) if self_enc == other_enc => self.as_slice() == other.as_slice(),
+            _ if self.is_ascii_only() && other.is_ascii_only() => self.as_slice() == other.as_slice(),
+            _ => false,
+        }
     }
 }
 
diff --git a/spinoso-string/src/lib.rs b/spinoso-string/src/lib.rs
index d590e4806..ca4202114 100644
--- a/spinoso-string/src/lib.rs
+++ b/spinoso-string/src/lib.rs
@@ -2183,6 +2183,8 @@ fn chomp(string: &mut String, separator: Option<&[u8]>) -> bool {
 
 #[cfg(test)]
 mod tests {
+    use alloc::string::ToString;
+
     use crate::center::CenterError;
     use crate::String;
 
@@ -2203,23 +2205,104 @@ mod tests {
     }
 
     #[test]
-    fn strings_compare_equal_only_based_on_byte_content() {
+    fn strings_equality_is_reflexive() {
+        // ASCII only, all encodings valid
+        let utf8 = String::utf8(b"abc".to_vec());
+        let ascii = String::ascii(b"abc".to_vec());
+        let binary = String::binary(b"abc".to_vec());
+        assert_eq!(&utf8, &utf8);
+        assert_eq!(&ascii, &ascii);
+        assert_eq!(&binary, &binary);
+
+        // Invalid UTF-8
+        let utf8 = String::utf8(b"abc\xFE\xFF".to_vec());
+        let ascii = String::ascii(b"abc\xFE\xFF".to_vec());
+        let binary = String::binary(b"abc\xFE\xFF".to_vec());
+        assert_eq!(&utf8, &utf8);
+        assert_eq!(&ascii, &ascii);
+        assert_eq!(&binary, &binary);
+
+        // Multibyte UTF-8
+        let utf8 = String::utf8("很困".to_string().into_bytes());
+        let ascii = String::ascii("很困".to_string().into_bytes());
+        let binary = String::binary("很困".to_string().into_bytes());
+        assert_eq!(&utf8, &utf8);
+        assert_eq!(&ascii, &ascii);
+        assert_eq!(&binary, &binary);
+    }
+
+    #[test]
+    fn strings_compare_equal_only_based_on_byte_content_with_valid_encoding() {
+        // ASCII only
         let utf8 = String::utf8(b"abc".to_vec());
         let ascii = String::ascii(b"abc".to_vec());
         let binary = String::binary(b"abc".to_vec());
         assert_eq!(utf8, ascii);
+        assert_eq!(ascii, utf8);
         assert_eq!(utf8, binary);
+        assert_eq!(binary, utf8);
         assert_eq!(binary, ascii);
+        assert_eq!(ascii, binary);
     }
 
     #[test]
-    fn strings_compare_equal_only_based_on_byte_content_without_valid_encoding() {
+    fn strings_with_multibyte_utf8_content_require_compatible_encoding_to_compare_equal() {
+        let utf8 = String::utf8("很困".to_string().into_bytes());
+        let ascii = String::ascii("很困".to_string().into_bytes());
+        let binary = String::binary("很困".to_string().into_bytes());
+        assert_ne!(utf8, ascii);
+        assert_ne!(ascii, utf8);
+        assert_ne!(utf8, binary);
+        assert_ne!(binary, utf8);
+        assert_ne!(binary, ascii);
+        assert_ne!(ascii, binary);
+    }
+
+    #[test]
+    fn strings_compare_unequal_with_equal_byte_content_without_valid_encoding() {
+        // ```
+        // [3.2.2] > utf8 = "abc\xFE\xFF"
+        // => "abc\xFE\xFF"
+        // [3.2.2] > utf8.encoding
+        // => #<Encoding:UTF-8>
+        // [3.2.2] > ascii = "abc\xFE\xFF"
+        // => "abc\xFE\xFF"
+        // [3.2.2] > ascii.force_encoding(Encoding::ASCII)
+        // => "abc\xFE\xFF"
+        // [3.2.2] > ascii.encoding
+        // => #<Encoding:US-ASCII>
+        // [3.2.2] > binary = "abc\xFE\xFF".b
+        // => "abc\xFE\xFF"
+        // [3.2.2] > binary.encoding
+        // => #<Encoding:ASCII-8BIT>
+        // [3.2.2] > utf8.ascii_only?
+        // => false
+        // [3.2.2] > ascii.ascii_only?
+        // => false
+        // [3.2.2] > binary.ascii_only?
+        // => false
+        // [3.2.2] > utf8 == ascii
+        // => false
+        // [3.2.2] > ascii == utf8
+        // => false
+        // [3.2.2] > utf8 == binary
+        // => false
+        // [3.2.2] > binary == utf8
+        // => false
+        // [3.2.2] > binary == ascii
+        // => false
+        // [3.2.2] > ascii == binary
+        // => false
+        // ```
         let utf8 = String::utf8(b"abc\xFE\xFF".to_vec());
         let ascii = String::ascii(b"abc\xFE\xFF".to_vec());
         let binary = String::binary(b"abc\xFE\xFF".to_vec());
-        assert_eq!(utf8, ascii);
-        assert_eq!(utf8, binary);
-        assert_eq!(binary, ascii);
+        assert_ne!(utf8, ascii);
+        assert_ne!(ascii, utf8);
+        assert_ne!(utf8, binary);
+        assert_ne!(binary, utf8);
+        assert_ne!(binary, ascii);
+        assert_ne!(ascii, binary);
     }
 
     #[test]


Test patch:
diff --git a/artichoke-backend/src/extn/core/string/string_test.rb b/artichoke-backend/src/extn/core/string/string_test.rb
index c9e46a87b..244b8071a 100644
--- a/artichoke-backend/src/extn/core/string/string_test.rb
+++ b/artichoke-backend/src/extn/core/string/string_test.rb
@@ -12,6 +12,7 @@ def spec
   string_tr
   string_end_with
   string_to_i
+  string_eq
 
   true
 end
@@ -207,4 +208,19 @@ def string_to_i
   raise unless '1__23'.to_i == 1
 end
 
+def string_eq
+  raise unless '太贵了!!' != '太贵了!!'.b # rubocop:disable Style/YodaCondition
+  raise unless '太贵了!!'.b != '太贵了!!'
+
+  a = "\x7f"
+  b = a.b
+  raise unless a == b
+  raise unless b == a
+
+  c = "\x80"
+  d = c.b
+  raise if c == d
+  raise if d == c
+end
+
 spec if $PROGRAM_NAME == __FILE__