This repository has been archived by the owner. It is now read-only.

Change formatProperty in util.js to use Object.getOwnPropertyDescriptor ... #2109

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
1 participant
@ghost

ghost commented Nov 14, 2011

...instead of lookup[GS]etter.

They are deprecated and often break when using Harmony Proxies.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Nov 14, 2011

There is lot of other places they use {define,lookup}{G,S}etter.
I myself am interested if move to proper ES5 is in the plan.

ghost commented Nov 14, 2011

There is lot of other places they use {define,lookup}{G,S}etter.
I myself am interested if move to proper ES5 is in the plan.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Nov 15, 2011

It's a particular issue with the four deprecated accessor functions because they make an end-run around all the new Object.* stuff introduced in ES5. If there's anything else that's not at least ES5-normal then it should be updated because most of it will be disallowed in ES6-strict or at least its interaction with new things in ES6 will be 'undefined'. The spec is a couple years from final completion but much of it is well defined and implemented in both V8 and SpiderMonkey already.

I'll start going through the libs with this task in mind. The specific issue in this pull request is by far the biggest as it causes anything inspect for formatting to be unusable with Proxies (as in immediate die with 'illegal access').

I'm currently working around it by monkey-patching the four functions prior to using Proxies which works.

ghost commented Nov 15, 2011

It's a particular issue with the four deprecated accessor functions because they make an end-run around all the new Object.* stuff introduced in ES5. If there's anything else that's not at least ES5-normal then it should be updated because most of it will be disallowed in ES6-strict or at least its interaction with new things in ES6 will be 'undefined'. The spec is a couple years from final completion but much of it is well defined and implemented in both V8 and SpiderMonkey already.

I'll start going through the libs with this task in mind. The specific issue in this pull request is by far the biggest as it causes anything inspect for formatting to be unusable with Proxies (as in immediate die with 'illegal access').

I'm currently working around it by monkey-patching the four functions prior to using Proxies which works.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Nov 15, 2011

I updated the function a bit to make use of the fact that the descriptor will have the value in it. In playing around with stack tracing proxies it's glaring that formatProperty is doing 3 redundant lookups for every property.

ghost commented Nov 15, 2011

I updated the function a bit to make use of the fact that the descriptor will have the value in it. In playing around with stack tracing proxies it's glaring that formatProperty is doing 3 redundant lookups for every property.

Brandon Benvie
Fallback to standard lookup if the descriptor is empty. This doesn't …
…ever happen with normal JS objects (this function is called only when the key exists) but apparently does with Node's custom ENV interface.
@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Dec 1, 2011

Poke, someone review me. It's only 13 lines of code and they do the exact same thing except less broken and slightly faster. It's good for your health!

ghost commented Dec 1, 2011

Poke, someone review me. It's only 13 lines of code and they do the exact same thing except less broken and slightly faster. It's good for your health!

@koichik

This comment has been minimized.

Show comment Hide comment
@koichik

koichik Jan 22, 2012

@Benvie - Sorry for the delay. LGTM, merging.
Actually, this patch is not only refactoring. For example, this is displayed as expected:

> var a = Object.create({}, {a: {get: function() {return 'aaa'}}});
undefined
> require('util').inspect(a, true);
'{ [a]: [Getter] }'

However, if the object has no prototype, current util.inspect() displays the value of the property:

> var a = Object.create(null, {a: {get: function() {return 'aaa'}}});
undefined
> require('util').inspect(a, true);
'{ [a]: \'aaa\' }'

After this patch is applied:

> var a = Object.create(null, {a: {get: function() {return 'aaa'}}});
undefined
> require('util').inspect(a, true);
'{ [a]: [Getter] }'

corrected.

koichik commented Jan 22, 2012

@Benvie - Sorry for the delay. LGTM, merging.
Actually, this patch is not only refactoring. For example, this is displayed as expected:

> var a = Object.create({}, {a: {get: function() {return 'aaa'}}});
undefined
> require('util').inspect(a, true);
'{ [a]: [Getter] }'

However, if the object has no prototype, current util.inspect() displays the value of the property:

> var a = Object.create(null, {a: {get: function() {return 'aaa'}}});
undefined
> require('util').inspect(a, true);
'{ [a]: \'aaa\' }'

After this patch is applied:

> var a = Object.create(null, {a: {get: function() {return 'aaa'}}});
undefined
> require('util').inspect(a, true);
'{ [a]: [Getter] }'

corrected.

koichik added a commit that referenced this pull request Jan 22, 2012

util: use getOwnPropertyDescripter
Change formatProperty in util.js to use Object.getOwnPropertyDescriptor
instead of __lookup[GS]etter__.

Use the cached value from the descriptor to reduce number of property
lookups from 3 to 1.

Fallback to standard lookup if the descriptor is empty. This doesn't
ever happen with normal JS objects (this function is called only when
the key exists) but apparently does with Node's custom ENV interface.

Fixes: #2109.

koichik added a commit that referenced this pull request Jan 22, 2012

@koichik

This comment has been minimized.

Show comment Hide comment
@koichik

koichik Jan 22, 2012

@Benvie - Merged in f901443 (v0.6). I squashed into a single commit, and added the test. Thanks for your contribution!

koichik commented Jan 22, 2012

@Benvie - Merged in f901443 (v0.6). I squashed into a single commit, and added the test. Thanks for your contribution!

@koichik koichik closed this Jan 22, 2012

wiedi pushed a commit to wiedi/node that referenced this pull request Aug 15, 2015

test: fix messages and use return to skip tests
This is a followup of nodejs/node#2109.
The tests which didn't make it in #2109, are included in this patch.
The skip messages are supposed to follow the format

    1..0 # Skipped: [Actual reason why the test is skipped]

and the tests should be skipped with the return statement.

PR-URL: nodejs/node#2290
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.