Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer.prototype.slice is incompatible with Uint8Array.prototype.slice; it's documented but tends to cause bugs #28087

Closed
gfx opened this issue Jun 6, 2019 · 4 comments
Labels
buffer Issues and PRs related to the buffer subsystem. question Issues that look for answers.

Comments

@gfx
Copy link
Contributor

gfx commented Jun 6, 2019

  • Version: v12.4.0 (official binary)
  • Platform: macOS 10.14.5 (Darwin Kernel Version 18.6.0)

The documentation says Buffer has an incompatibility with Uint8rray and I believe it is considered to a bug, or at least tends to cause bugs.

https://nodejs.org/api/buffer.html

Buffer instances are also Uint8Array instances. However, there are subtle incompatibilities with TypedArray. For example, while ArrayBuffer#slice() creates a copy of the slice, the implementation of Buffer#slice() creates a view over the existing Buffer without copying, making Buffer#slice() far more efficient.

This incompatibility is problematic when a code expects Uint8Array as a parameter and it distinguishes the semantics of Uint8Array.prototype.slice() and Uint8Array.prototype.subarray().

Unfortunately, TypeScript can't help this problem because function f(b: Uint8Array) can take Buffer without notice:

// typtescript
const a: Uint8Array = Buffer.from([]); // OK because Buffer extends Uint8Array

const b: Int8Array = Buffer.from([]); // NG, of course

I think this incompatibility should be fixed in the long-term perspective, so firstly I propose to mark slice() as deprecated.

What do you think of it?

@devsnek devsnek added buffer Issues and PRs related to the buffer subsystem. question Issues that look for answers. labels Jun 6, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Jun 6, 2019

The Node.js buffers existed before Uint8Array became part of ECMAScript. Changing this behavior now would be a significant breaking change and would likely cause a big disruption in the ecosystem. We could in theory introduce a new function that works like our current .slice() and add the deprecation notice but changing all occurrences is very hard (the buffer constructor is still used frequently even though it's deprecated since years and we made a huge effort to change it. This would probably be similar).

There is a simple work around that would work for both, Uint8Array and Buffer: Uint8Array.prototype.slice.call(buffer, begin, end).

Due to that possibility I do not think that we should change this.

@bnoordhuis
Copy link
Member

firstly I propose to mark slice() as deprecated

Depends on what you mean by "deprecated".

As @BridgeAR mentions, it's used very extensively in the ecosystem so deprecating it as in "printing warnings" is probably out of the question.

Some kind of documentation-only "soft deprecation" in the sense of "don't use this, use that" might be acceptable.

You could start by adding a note to doc/api/buffer.md that explains how and why Buffer#slice() is different from TypedArray#slice(). That seems like a useful addition regardless of the outcome of this discussion.

@gfx
Copy link
Contributor Author

gfx commented Jun 6, 2019

As you said, deprecating methods is difficult but compatibility with Uint8Array is worth considering in the long-term perspective.

Fortunately, I think, it is easier than deprecating new Buffer because Buffer#subarray() is the same as the current slice() and compatible with Uint8Array, although subarray() is not documented.

Trott pushed a commit to Trott/io.js that referenced this issue Jun 13, 2019
PR-URL: nodejs#28101
Refs: nodejs#28087
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this issue Jun 17, 2019
PR-URL: #28101
Refs: #28087
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bnoordhuis
Copy link
Member

Closing now that #28101 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

4 participants