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

util.inherits does not establish prototype chain #4179

Closed
silkentrance opened this Issue Dec 7, 2015 · 15 comments

Comments

Projects
None yet
8 participants
@silkentrance

In inherits.js Object.setPrototypeOf is being called with the wrong parameters.

The reason for me assuming that the wrong parameters are being used is

(REPL)

> function MyError() {}
> util.inherits(MyError, Error);
> MyError
{ [Function: MyError]
  super_: 
   { [Function: Error]
     captureStackTrace: [Function: captureStackTrace],
     stackTraceLimit: 10 } }
> Error.isPrototypeOf(MyError);
false

While inheritance sort of works, the prototype chain is actually never established.

> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
true

Replacing the existing call to Object.setPrototypeOf() by

Object.setPrototypeOf(ctor, superCtor);

See https://github.com/nodejs/node/blob/master/lib/util.js#L805.

(REPL)

> function MyError() {}
> util.inherits(MyError, Error);
> MyError
[Function: OError]
> Error.isPrototypeOf(MyError);
true

Yet, instanceof will now fail

> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
false

When using the new class feature, everything works as expected, though

(REPL)

> class MyError extends Error {}
[Function: MyError]
> Error.isPrototypeOf(MyError)
true
> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
true

@silkentrance silkentrance changed the title from util.inherits calls Object.setPrototypeOf with the wrong parameters to util.inherits does not establish prototype chain Dec 7, 2015

@mscdex mscdex added the util label Dec 7, 2015

@silkentrance

This comment has been minimized.

Show comment
Hide comment
@silkentrance

This comment has been minimized.

Show comment
Hide comment

See also #3188

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Dec 8, 2015

Member

Object.setPrototypeOf(ctor, superCtor)

That's because inherits() calls Object.setPrototypeOf(ctor.prototype, superCtor.prototype). It goes all the way back to v0.3.2 so I don't think we can change that without breaking a lot of existing code.

Perhaps it's time for inherits() to go gently into that good night and for us to point people to ES6 extends.

Member

bnoordhuis commented Dec 8, 2015

Object.setPrototypeOf(ctor, superCtor)

That's because inherits() calls Object.setPrototypeOf(ctor.prototype, superCtor.prototype). It goes all the way back to v0.3.2 so I don't think we can change that without breaking a lot of existing code.

Perhaps it's time for inherits() to go gently into that good night and for us to point people to ES6 extends.

@silkentrance

This comment has been minimized.

Show comment
Hide comment
@silkentrance

silkentrance Dec 8, 2015

Sigh perhaps making it @deprecated would suffice then...

Sigh perhaps making it @deprecated would suffice then...

@silkentrance

This comment has been minimized.

Show comment
Hide comment
@silkentrance

silkentrance Jan 31, 2016

Reopening as I deem it fit to at least document that shortcoming in the official documentation and deprecate util.inherits in favor of new ES2015/harmony features and also in favor of external libraries that get the job done properly.

Reopening as I deem it fit to at least document that shortcoming in the official documentation and deprecate util.inherits in favor of new ES2015/harmony features and also in favor of external libraries that get the job done properly.

@silkentrance silkentrance reopened this Jan 31, 2016

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 1, 2016

Member

We're open to pull requests that improve the documentation.

As to deprecation, IMO it should start as a documentation-only deprecation that steers people away from using it in new code. util.inherits() is used in a lot of existing code and is, for the most part, not broken. Printing deprecation warnings is arguably too aggressive / obnoxious.

Member

bnoordhuis commented Feb 1, 2016

We're open to pull requests that improve the documentation.

As to deprecation, IMO it should start as a documentation-only deprecation that steers people away from using it in new code. util.inherits() is used in a lot of existing code and is, for the most part, not broken. Printing deprecation warnings is arguably too aggressive / obnoxious.

@silkentrance

This comment has been minimized.

Show comment
Hide comment
@silkentrance

silkentrance Feb 2, 2016

Well I think that in the past, people did not care too much about isPrototypeOf, so I think it would be safe to fix util.inherits. Considering that most users would test recursively for _super. See also the vibejs-subclassof project, which BTW did not gain that much traction 😁

As such, I think that with the next release of node the behavior should just be changed and let users just fix their code if they ever managed to screw it up so badly as to rely on that falsely set up prototype chain.

Well I think that in the past, people did not care too much about isPrototypeOf, so I think it would be safe to fix util.inherits. Considering that most users would test recursively for _super. See also the vibejs-subclassof project, which BTW did not gain that much traction 😁

As such, I think that with the next release of node the behavior should just be changed and let users just fix their code if they ever managed to screw it up so badly as to rely on that falsely set up prototype chain.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 12, 2016

Member

@nodejs/documentation ... not sure about the deprecation here but it would likely be good to include some documentation on the limitations of util.inherits with regards to the prototype chain.

Member

jasnell commented Apr 12, 2016

@nodejs/documentation ... not sure about the deprecation here but it would likely be good to include some documentation on the limitations of util.inherits with regards to the prototype chain.

@lordKnighton

This comment has been minimized.

Show comment
Hide comment
@lordKnighton

lordKnighton Apr 12, 2016

+1

some documentation on the limitations of util.inherits

+1

some documentation on the limitations of util.inherits

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 12, 2016

Contributor

Reading up un this I think the change would actually be possible, no? @bnoordhuis there was this PR in Oct'15 that actually change the from .create() to .setPrototypeOf(). Though I don't mind the missing chain, the change wouldn't breaking then, or is it?

Pointing the extends for real inheritance sounds good to me, though I wouldn't deprecate, since it actually works well.

Documentation should only(?): 1. mention that it is overriding the prototype instead appending the chain 2. point to extends in order to the chain.

Contributor

eljefedelrodeodeljefe commented Apr 12, 2016

Reading up un this I think the change would actually be possible, no? @bnoordhuis there was this PR in Oct'15 that actually change the from .create() to .setPrototypeOf(). Though I don't mind the missing chain, the change wouldn't breaking then, or is it?

Pointing the extends for real inheritance sounds good to me, though I wouldn't deprecate, since it actually works well.

Documentation should only(?): 1. mention that it is overriding the prototype instead appending the chain 2. point to extends in order to the chain.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 12, 2016

Member

For future reference, @eljefedelrodeodeljefe is referring to #3455.

Member

bnoordhuis commented Apr 12, 2016

For future reference, @eljefedelrodeodeljefe is referring to #3455.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 12, 2016

Contributor

Sorry meant to post the link.

Contributor

eljefedelrodeodeljefe commented Apr 12, 2016

Sorry meant to post the link.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe May 2, 2016

Contributor

I am leaning towards class extends as solution for this one. Started a discussion at #6512 for docs.

Contributor

eljefedelrodeodeljefe commented May 2, 2016

I am leaning towards class extends as solution for this one. Started a discussion at #6512 for docs.

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe May 9, 2016

Contributor

FYI this got closed due to a change in docs, see 07c572d. Thx for reporting! It triggered a good conversation and change.

Contributor

eljefedelrodeodeljefe commented May 9, 2016

FYI this got closed due to a change in docs, see 07c572d. Thx for reporting! It triggered a good conversation and change.

eljefedelrodeodeljefe added a commit that referenced this issue May 9, 2016

doc: discourage use of util.inherits
util.inherits breaks the prototype chain. A fix does not seem
useful, since ES6 extends provides language level support for the
same functionality.

This commit starts fasing out mentions of the method.

Fixes: #6512
Fixes: #4179

PR-URL: #6514
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

evanlucas added a commit that referenced this issue May 17, 2016

doc: discourage use of util.inherits
util.inherits breaks the prototype chain. A fix does not seem
useful, since ES6 extends provides language level support for the
same functionality.

This commit starts fasing out mentions of the method.

Fixes: #6512
Fixes: #4179

PR-URL: #6514
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@senocular

This comment has been minimized.

Show comment
Hide comment
@senocular

senocular Aug 15, 2016

I know this is old, but I stumbled onto this looking for something else, and I think there's a little confusion in the claims of the original issue.

First, as far as I'm concerned, this implementation of inherits is correct, or at least "good", with what we can call "room for improvement". The point of inherits is to allow instances of one constructor to inherit members that are available to instances of another (those members stored in the prototype). This is what the current implementation achieves, as seen with the original example with:

> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
true

This is your classic behavior with prototype chain inheritance. Instances of one constructor (MyError) are able to access members of another (Error) and pass the instanceof check (which compares prototypes).

For this example:

> function MyError() {}
> util.inherits(MyError, Error);
> MyError
{ [Function: MyError]
  super_: 
   { [Function: Error]
     captureStackTrace: [Function: captureStackTrace],
     stackTraceLimit: 10 } }
> Error.isPrototypeOf(MyError);
false

It implies that the constructor itself would inherit from the super constructor. Generally this isn't done when setting up inheritance manually, but ES6 happens to do this with extends as it allows static members to be inherited. Prior to ES6, I don't think I've ever seen anyone ever do this (plus prior to ES6 and Object.setPrototypeOf, it was only possible going through __proto__ which you should feel dirty for even considering using ;P).

Now, this behavior is something that can be added on top of the existing behavior if desired. It's not meant to replace it. So "Object.setPrototypeOf is being called with the wrong parameters" is not really the correct way to explain what's going on. If you're expecting the constructor inheritance for static members, then it would be something done along side the existing instance inheritance.

Object.setPrototypeOf(ctor.prototype, superCtor.prototype); // existing instance member inheritance
Object.setPrototypeOf(ctor, superCtor); // additional static inheritance

senocular commented Aug 15, 2016

I know this is old, but I stumbled onto this looking for something else, and I think there's a little confusion in the claims of the original issue.

First, as far as I'm concerned, this implementation of inherits is correct, or at least "good", with what we can call "room for improvement". The point of inherits is to allow instances of one constructor to inherit members that are available to instances of another (those members stored in the prototype). This is what the current implementation achieves, as seen with the original example with:

> myerr = new MyError()
[Error]
> myerr instanceof MyError
true
> myerr instanceof Error
true

This is your classic behavior with prototype chain inheritance. Instances of one constructor (MyError) are able to access members of another (Error) and pass the instanceof check (which compares prototypes).

For this example:

> function MyError() {}
> util.inherits(MyError, Error);
> MyError
{ [Function: MyError]
  super_: 
   { [Function: Error]
     captureStackTrace: [Function: captureStackTrace],
     stackTraceLimit: 10 } }
> Error.isPrototypeOf(MyError);
false

It implies that the constructor itself would inherit from the super constructor. Generally this isn't done when setting up inheritance manually, but ES6 happens to do this with extends as it allows static members to be inherited. Prior to ES6, I don't think I've ever seen anyone ever do this (plus prior to ES6 and Object.setPrototypeOf, it was only possible going through __proto__ which you should feel dirty for even considering using ;P).

Now, this behavior is something that can be added on top of the existing behavior if desired. It's not meant to replace it. So "Object.setPrototypeOf is being called with the wrong parameters" is not really the correct way to explain what's going on. If you're expecting the constructor inheritance for static members, then it would be something done along side the existing instance inheritance.

Object.setPrototypeOf(ctor.prototype, superCtor.prototype); // existing instance member inheritance
Object.setPrototypeOf(ctor, superCtor); // additional static inheritance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment