indexOf empty string in buffer should return 0

[simonkcleung]

"abc".indexOf("") return 0.
But Buffer.from("abc").indexOf("") return -1.
Expected result : 0

[lance]

Hmm - the Buffer#indexOf documentation explicitly states that it will return -1 if the value is not found. https://nodejs.org/api/buffer.html#buffer_buf_indexof_value_byteoffset_encoding

Returns: The index of the first occurrence of value in buf or -1 if buf does not contain value

I believe this is expected behavior.

[TimothyGu]

See what @addaleax said below!

The following information is wrong, see #13023 (comment)!

Buffer functions more akin to an array than a string. (In fact it extends the Uint8Array class!) ["a", "b", "c"].indexOf("") returns -1 also.

[addaleax]

@lance I think this is a bug – it makes sense for an empty input value to say that it is found immediately, without any searching. Also, String.prototype.indexOf behaves the same way, and I think consistency is important (especially when there’s no reason to deviate) because developers expect them to behave the same way.

@TimothyGu Unlike Array#indexOf, Buffer#indexOf doesn’t compare elements – it compares subsequences, and there’s no equivalent of an empty subsequence for arrays. :)

This should be enough to fix:

diff

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 367af6592ff3..32a942a42c4e 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -974,7 +974,13 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {
   const size_t needle_length =
       StringBytes::Size(args.GetIsolate(), needle, enc);
 
-  if (needle_length == 0 || haystack_length == 0) {
+  if (needle_length == 0) {
+    const double first_index = is_forward ? 0 : haystack_length;
+    args.GetReturnValue().Set(first_index);
+    return;
+  }
+
+  if (haystack_length == 0) {
     return args.GetReturnValue().Set(-1);
   }
 
@@ -1077,7 +1083,13 @@ void IndexOfBuffer(const FunctionCallbackInfo<Value>& args) {
   const char* needle = buf_data;
   const size_t needle_length = buf_length;
 
-  if (needle_length == 0 || haystack_length == 0) {
+  if (needle_length == 0) {
+    const double first_index = is_forward ? 0 : haystack_length;
+    args.GetReturnValue().Set(first_index);
+    return;
+  }
+
+  if (haystack_length == 0) {
     return args.GetReturnValue().Set(-1);
   }

[TimothyGu]

Oops, read what @addaleax said :)

[lance]

@addaleax I see your point - especially given that String.prototype.indexOf works this way. Comment retracted. And your diff looks reasonable to me.

[simonkcleung]

Welcome for the changes. But I would like to explain consistency more.
In doc:

Buffer class implements the Uint8Array API in a manner that is more optimized and suitable for Node.js' use cases

There is no consistency in the first place. Buffer.from("abc").indexOf("") = -1, is more consistent with the Uint8Array API.

However, existing Buffer#indexOf implements the String#indexOf more likely. Especially when Buffer.from("abc").indexOf("bc") = 1. Then, Buffer.from("abc").indexOf("") should be 0.

[joyeecheung]

Hmm, since the spec has already defined TypedArray.prototype.indexOf() as:

22.2.3.14%TypedArray%.prototype.indexOf ( searchElement [ , fromIndex ] )
%TypedArray%.prototype.indexOf is a distinct function that implements the same algorithm as Array.prototype.indexOf as defined in 22.1.3.12 except that the this object's [[ArrayLength]] internal slot is accessed in place of performing a [[Get]] of "length"

I would say the current behavior is more consistent..especially when we are on our way making APIs that accept Buffers also accept Uint8Arrays

[addaleax]

@joyeecheung That’s what Uint8Array.prototype.indexOf does, but as mentioned above we already deviate very strongly from that – we accept sequences instead of individual values as input, and there’s no way of mapping that to Uint8Array.prototype.indexOf’s type signature.