byteOffset argument in buffer indexOf / lastIndexOf has some contradictions in code vs doc vs test

[vsemozhetbyt]

• Version: 7.2.0
• Platform: Windows 7
• Subsystem: buffer

Some contradictions in status quo:

1. buffer.lastIndexOf() code example in doc:

const utf16Buffer = Buffer.from('\u039a\u0391\u03a3\u03a3\u0395', 'ucs2');

// Prints: 6
console.log(utf16Buffer.lastIndexOf('\u03a3', null, 'ucs2'));

Actually, it prints -1 now.

2. buffer.js coerces byteOffset null to 0. However, in the next block it checks if byteOffset is null to make it default byteOffset if so.

3. test-buffer-indexof.js expects null byteOffset to return -1, i.e. it expects null byteOffset not to be converted into the default byteOffset.

Maybe the fix steps could be these:

1. buffer.js should not coerce null to Number.
2. test-buffer-indexof.js should expect null byteOffset to be converted into the default byteOffset.
3. Doc should clarify which argument types and values trigger default. Maybe something like position remarks in the fs doc for fs.read() and fs.write().

[vsemozhetbyt]

It seems this big PR concerns all points.

[silverwind]

I agree, this certainly looks like a bug. A offset of null and undefined should result in the same behaviour. Wanna file a PR to fix it in both code and tests?

[vsemozhetbyt]

@silverwind #4846 was a big complex PR. Maybe the author wants to do this? This way all the method system will be taken into account. ping @dcposch

[vsemozhetbyt]

Maybe cc #4846 reviewers? @jasnell, @trevnorris.

[dcposch]

@vsemozhetbyt @silverwind I have time to look at this today. PR coming soon!

[dcposch]

So it looks like I did that in order to match the behavior of String:

var s = "abcdef"
var b = new Buffer("abcdef")
s.indexOf('b', null) // prints 1
b.indexOf('b', null) // prints 1
s.lastIndexOf('b', null) // prints -1
b.lastIndexOf('b', null) // prints -1

...in other words, both String and Buffer coerce an offset of null to zero.

Introducing inconsistency between String and Buffer seems bad. I'll send a PR that fixes the documentation and makes the tests more explicit, while leaving the behavior as is. Thoughts?

[dcposch]

After a bit more investigating, it looks like String and Buffer both coerce the offset argument to a number, then use the default offset if the result is NaN. (Using the default offset means searching the whole String or Buffer.)

The bad news is that this leads to some really weird, unintuitive behavior. The good news is that at least they're consistent with each other and with Javascript's unary + operator:

var s = "abcdef"
var b = new Buffer("abcdef")

console.log('OFFSETS THAT COERCE TO NaN')
console.log(+{}) // prints NaN, so this will turn into the default offset
console.log(+undefined) // same here
// The following statements all print 1, searching the whole String or Buffer
s.lastIndexOf('b')
s.lastIndexOf('b', undefined)
s.lastIndexOf('b', {})
b.lastIndexOf('b')
b.lastIndexOf('b', undefined)
b.lastIndexOf('b', {})

console.log('OFFSETS THAT COERCE TO 0')
console.log(+null) // print 0
console.log(+[]) // same here
// The following statements all print -1, because they search from offset 0
s.lastIndexOf('b', null)
s.lastIndexOf('b', [])
b.lastIndexOf('b', null)
b.lastIndexOf('b', [])

[dcposch]

Looks like my mistake in the docs was fixed here: 6050bbe

Thanks @vsemozhetbyt

[vsemozhetbyt]

So, if this is the desired behavior, this comment needs to be fixed:

https://github.com/nodejs/node/blob/master/lib/buffer.js#L593

[trevnorris]

@vsemozhetbyt good catch. mind commenting directly on that line of the commit that the comment should be removed?

[vsemozhetbyt]

@trevnorris If I get it right, it seems the comment has been changed already.

[trevnorris]

@vsemozhetbyt thanks for pointing that out.