[dart2wasm] Fix JSStringImpl trim methods, add tests

Fix trim methods returning a new string (instead of the argument) when
there's nothing to trim.

A second bug fixed in NEL handling in `trim`: the last line should be
`result.substring(...)` instead of `this.substring(...)`. The bug wasn't
caught by any of the tests: NEL needs to be handled specially on the
browser, but mixing NEL with other whitespace wasn't tested in the
existing tests `string_trim_test` and `string_trimlr_test`, so a new
test file added with this case.

Change-Id: Ibf84e185de1b26c9811e3ea58c2ad3f223da8515
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/363081
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Ömer Ağacan <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>

Author: osa1

sdk/lib/_internal/wasm/lib/js_string.dart

@@ -328,6 +328,7 @@ final class JSStringImpl implements String {
   String substring(int start, [int? end]) {
     end ??= length;
     RangeErrorUtils.checkValidRangePositiveLength(start, end, length);
+    if (start == end) return "";
     return JSStringImpl(_jsSubstring(toExternRef, start, end));
   }
 
@@ -441,73 +442,96 @@ final class JSStringImpl implements String {
     return index;
   }
 
-  // dart2wasm can't use JavaScript trim directly,
-  // because JavaScript does not trim
-  // the NEXT LINE (NEL) character (0x85).
+  // dart2wasm can't use JavaScript trim directly, because JavaScript does not
+  // trim the NEXT LINE (NEL) character (0x85).
   @override
   String trim() {
-    // Start by doing JS trim. Then check if it leaves a NEL at
-    // either end of the string.
+    final length = this.length;
+    if (length == 0) return this;
+
+    // Start by doing JS trim. Then check if it leaves a NEL at either end of
+    // the string.
     final result =
         JSStringImpl(js.JS<WasmExternRef?>('s => s.trim()', toExternRef));
     final resultLength = result.length;
     if (resultLength == 0) return result;
-    int firstCode = result._codeUnitAtUnchecked(0);
+
+    // Check NEL on the left.
+    final int firstCode = result._codeUnitAtUnchecked(0);
     int startIndex = 0;
     if (firstCode == nelCodeUnit) {
       startIndex = _skipLeadingWhitespace(result, 1);
       if (startIndex == resultLength) return "";
     }
 
+    // Check NEL on the right.
     int endIndex = resultLength;
     // We know that there is at least one character that is non-whitespace.
     // Therefore we don't need to verify that endIndex > startIndex.
-    int lastCode = result.codeUnitAt(endIndex - 1);
+    final int lastCode = result.codeUnitAt(endIndex - 1);
     if (lastCode == nelCodeUnit) {
       endIndex = _skipTrailingWhitespace(result, endIndex - 1);
     }
-    if (startIndex == 0 && endIndex == resultLength) return result;
-    return substring(startIndex, endIndex);
+
+    if (startIndex == 0 && endIndex == resultLength) {
+      return length == resultLength ? this : result;
+    }
+
+    return result.substring(startIndex, endIndex);
   }
 
   // dart2wasm can't use JavaScript trimLeft directly because it does not trim
-  // the NEXT LINE character (0x85).
+  // the NEXT LINE (NEL) character (0x85).
   @override
   String trimLeft() {
-    // Start by doing JS trim. Then check if it leaves a NEL at
-    // the beginning of the string.
+    final length = this.length;
+    if (length == 0) return this;
+
+    // Start by doing JS trim. Then check if it leaves a NEL at the beginning
+    // of the string.
     int startIndex = 0;
     final result =
         JSStringImpl(js.JS<WasmExternRef?>('s => s.trimLeft()', toExternRef));
     final resultLength = result.length;
     if (resultLength == 0) return result;
+
+    // Check NEL.
     int firstCode = result._codeUnitAtUnchecked(0);
     if (firstCode == nelCodeUnit) {
       startIndex = _skipLeadingWhitespace(result, 1);
     }
-    if (startIndex == 0) return result;
-    if (startIndex == resultLength) return "";
+
+    if (startIndex == 0) {
+      return resultLength == length ? this : result;
+    }
+
     return result.substring(startIndex);
   }
 
   // dart2wasm can't use JavaScript trimRight directly because it does not trim
-  // the NEXT LINE character (0x85).
+  // the NEXT LINE (NEL) character (0x85).
   @override
   String trimRight() {
+    final length = this.length;
+    if (length == 0) return this;
+
     // Start by doing JS trim. Then check if it leaves a NEL at the end of the
     // string.
     final result =
         JSStringImpl(js.JS<WasmExternRef?>('s => s.trimRight()', toExternRef));
     final resultLength = result.length;
+    if (resultLength == 0) return result;
+
     int endIndex = resultLength;
-    if (endIndex == 0) return result;
     int lastCode = result.codeUnitAt(endIndex - 1);
     if (lastCode == nelCodeUnit) {
       endIndex = _skipTrailingWhitespace(result, endIndex - 1);
     }
 
-    if (endIndex == resultLength) return result;
-    if (endIndex == 0) return "";
+    if (endIndex == resultLength) {
+      return resultLength == length ? this : result;
+    }
+
     return result.substring(0, endIndex);
   }
 

tests/corelib/string_trim_nel_test.dart

@@ -0,0 +1,34 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// NEXT LINE (NEL, 0x85) character is special in that some Unicode versions do
+// not include it as a whitespace character.
+//
+// However all Dart backends handle it as a whitespace character. dart2js and
+// dart2wasm do this by a calling JS (`String.prototype.trim` and its
+// left/right variants) and doing post-processing. To check these
+// implementations tests in this file mix the SPACE character with NEL in
+// various positions.
+//
+// string_trim_lr_test.dart tests various NEL characters on the left and right,
+// but they do not mix NEL with other whitespace characters.
+// string_trim_test.dart tests do not test NEL.
+
+import "package:expect/expect.dart";
+
+const int space = 0x20;
+const int nel = 0x85;
+const int h = 0x68;
+const int i = 0x69;
+
+void main() {
+  Expect.equals(
+      "hi", String.fromCharCodes([space, nel, space, h, i]).trimLeft());
+  Expect.equals(
+      "hi", String.fromCharCodes([h, i, space, nel, space]).trimRight());
+  Expect.equals(
+      "hi",
+      String.fromCharCodes([space, nel, space, h, i, space, nel, space])
+          .trim());
+}