All 158 comments
@xeioex I'm on this.
hongzhidao
on 26 Apr 2019
@xeioex @hongzhidao
Some thoughts.
I think that is better to add run-time support first.
It does not require any changes in parser.
The main problem is to change our property descriptor to support two callbacks (get/set) somehow.
> var o = {};
undefined
> Object.defineProperty(o, 'r', { get: function() { return this.r_value; } });
{}
> o.r
undefined
> o.r_value = 1;
1
> o.r
1
> o.r = 2
TypeError: Cannot set property r of #<Object> which has only a getter
> Object.defineProperty(o, 'rw', { get: function() { return this.rw_value; }, set: function(x) { this.rw_value = x; } });
{ r_value: 1 }
> o.rw = 2
2
> o.rw
2
> o
{ r_value: 1, rw_value: 2 }
> Object.defineProperty(o, 'w', { set: function(x) { this.w_value = x; } });
{ r_value: 1, rw_value: 2 }
> o.w = 3
3
> o.w
undefined
> o
{ r_value: 1, rw_value: 2, w_value: 3 }
> Object.getOwnPropertyDescriptors(o)
{ r:
{ get: [Function: get],
set: undefined,
enumerable: false,
configurable: false },
r_value:
{ value: 1, writable: true, enumerable: true, configurable: true },
rw:
{ get: [Function: get],
set: [Function: set],
enumerable: false,
configurable: false },
rw_value:
{ value: 2, writable: true, enumerable: true, configurable: true },
w:
{ get: undefined,
set: [Function: set],
enumerable: false,
configurable: false },
w_value:
{ value: 3, writable: true, enumerable: true, configurable: true } }
drsm
on 27 Apr 2019
@xeioex
I have a few questions:
- It seems both of
vm->retvalandcode->retvalare OK to deal with return value.
See the two ways.
// way 1
// In generator:
njs_generate_code(generator, ..., 1);
// In vm:
ret = njs_value_property(vm, object, property, &vm->retval);
// way 2
// In generator:
njs_generate_code(generator, ..., 0);
code->retval = index;
// In vm:
code = (njs_vmcode_prop_get_t *) vm->current;
retval = njs_vmcode_operand(vm, code->retval);
ret = njs_value_property(vm, object, property, retval);
md5-0b13e6ee703ac447c5965bc7bee62776
/*
* ES5.1, 8.12.1: [[GetOwnProperty]], [[GetProperty]].
* The njs_property_query() returns values
* NXT_OK property has been found in object,
* retval of type njs_object_prop_t * is in pq->lhq.value.
* in NJS_PROPERTY_QUERY_GET
* prop->type is NJS_PROPERTY, NJS_METHOD or NJS_PROPERTY_HANDLER.
* in NJS_PROPERTY_QUERY_SET, NJS_PROPERTY_QUERY_DELETE
* prop->type is NJS_PROPERTY, NJS_PROPERTY_REF, NJS_METHOD or
* NJS_PROPERTY_HANDLER.
* NXT_DECLINED property was not found in object,
* if pq->lhq.value != NULL it contains retval of type
* njs_object_prop_t * where prop->type is NJS_WHITEOUT
* NJS_TRAP the property trap must be called,
* NXT_ERROR exception has been thrown.
*
* TODO: // Does it not pass now?
* Object.defineProperty([1,2], '1', {configurable:false})
*/
njs_ret_t
njs_property_query(njs_vm_t *vm, njs_property_query_t *pq, njs_value_t *object,
const njs_value_t *property)
md5-da54eb338a83406504724198a75e0602
Object.defineProperty(Number, 'isFinite', {});
useless.
@drsm welcome to test. Thanks for your suggestion, it's totally right to add run-time support first.
hongzhidao
on 2 Jun 2019
@hongzhidao
The below works in Chrome, but not njs. @drsm
Object.defineProperty(Number, 'isFinite', {});
Yes it doesn't, as for now, the internal methods are special in njs, they are different from plain data properties with function value assigned.
Found an issue with inherited setter, is should cause an infinite loop in this case:
$ node --use-strict
> var x = {};
undefined
> Object.defineProperty(x, 'prop', { get: function() { return this.prop; }, set: function(x) { this.prop = x; } });
{}
> var y = Object.create(x)
undefined
> y.prop
Thrown:
RangeError: Maximum call stack size exceeded
at Object.get (repl:1:49)
...
> y.prop = 1
Thrown:
RangeError: Maximum call stack size exceeded
at Object.set (repl:1:88)
...
>
$ build/njs
...
>> var x = {};
undefined
>> Object.defineProperty(x, 'prop', { get: function() { return this.prop; }, set: function(x) { this.prop = x; } });
{
}
>> var y = Object.create(x)
undefined
>> y.prop
RangeError: Maximum call stack size exceeded
at anonymous (shell:1)
repeats 1 times
at main (native)
>> y.prop = 1
1
@hongzhidao
TODO: // Does it not pass now?
- Object.defineProperty([1,2], '1', {configurable:false})
It passes, but currently {configurable:false} in fact is ignored for array values (currently all values have properties configurable == writeable == enumerable == 1).
The following test will fail for example
var a = [1,2]; Object.defineProperty(a, '1', {configurable:false});
Object.getOwnPropertyDescriptor(a, '1').configurable == false
The problem is, njs_array_t is array of njs_value_t, not njs_object_prop_t.
I have some ideas how to fix this effectively. Because non-standard property attributes (!configurable|| !writeable|| !enumerable) for array values are rarely used, we can store them in object->hash. So for ordinary operations like array[0] we can quickly get an array value, but if we need to consider property attributes, we get them from object->hash.
Here's the draft, what do you think of the design?
Looks good.
njs_object_attribute_t enumerable:8; /* 2 bits */
njs_object_attribute_t writable:8; /* 2 bits */
njs_object_attribute_t configurable:8; /* 2 bits */
// BTW, why not to store pointer to the function here? Can setter for example be non-function?
+ njs_value_t getter;
+ njs_value_t setter;
} njs_object_prop_t;
xeioex
on 2 Jun 2019
@xeioex @drsm
why not to store pointer to the function here? Can setter for example be non-function?
- All the below are OK. So I store getter/setter in njs_value_t type.
Object.defineProperty(o, 'r', { get: undefined})
Object.defineProperty(o, 'r', { get: parseInt})
Object.defineProperty(o, 'r', { get: function() {}})
Fields
getterandsetterbehave the same asvaluein redefine.Here are the patches.
Refactored njs_descriptor_prop(). (Ready)
Added get/set property support. (Review it first, I'll add more tests.)
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
hongzhidao
on 3 Jun 2019
@hongzhidao
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
After.
https://gist.github.com/760c045344a80f45bb5497fb57e2d244
I see a lot if fixes, but a lot of newly failed tests. I hope to look into it deeper tomorrow.
xeioex
on 3 Jun 2019
@hongzhidao
This shouldn't fail:
>> Object.defineProperty({}, 'r', { get: undefined })
TypeError: Getter must be a function
at Object.defineProperty (native)
at main (native)
>> Object.defineProperty({}, 'r', { set: undefined })
TypeError: Setter must be a function
at Object.defineProperty (native)
at main (native)
@hongzhidao
Fields getter and setter behave the same as value in redefine.
No, here is the spec: 9.1.6.3 ValidateAndApplyPropertyDescriptor
>> Object.defineProperty({}, 'r', { get: () => {} }).r = 1
TypeError: Cannot assign to read-only property "r" of object
$ node --use-strict
> Object.defineProperty({}, 'r', {}).r = 1
Thrown:
TypeError: Cannot assign to read only property 'r' of object '#<Object>'
> Object.defineProperty({}, 'r', { set: undefined }).r = 1
Thrown:
TypeError: Cannot set property r of #<Object> which has only a getter
>
@drsm @xeioex
Here are the patches.
Refactored njs_descriptor_prop(). (Ready)
Added get/set property support. (Review it first, I'll add more tests.)
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
Ignore them, sorry that I didn't make it clear.
var o = {}
Object.defineProperty(o, 'a', {get: undefined})
Object.defineProperty(o, 'a', {value: undefined})
o.a
Reported
Cannot redefine property: a
I'm on this. It may change above patches.
@drsm welcome to explain why it throws redefine error, it would help me.
hongzhidao
on 4 Jun 2019
@hongzhidao
var o = {} Object.defineProperty(o, 'a', {get: undefined}) Object.defineProperty(o, 'a', {value: undefined}) o.aReported
Cannot redefine property: aI'm on this. It may change above patches.
@drsm welcome to explain why it throws redefine error, it would help me.
this Object.defineProperty(o, 'a', {get: undefined}) becomes
Object.defineProperty(o, 'a', {get: undefined, set: undefined, configurable:false, enumerable: false })
after 9.1.6.3 ValidateAndApplyPropertyDescriptor 2.d.ii
If the value of an attribute field of Desc is absent, the attribute of the newly created property is set to its default value.
then Object.defineProperty(o, 'a', {value: undefined}) fails at 6.a
The property type is determined by presence of get (accessor) or value|wiritable (data) keys in the descriptor object hash, not by value:
6.2.5.1 - 6.2.5.3 https://tc39.github.io/ecma262/#sec-isaccessordescriptor
and
6.2.5.5 https://tc39.github.io/ecma262/#sec-topropertydescriptor
drsm
on 4 Jun 2019
@xeioex
Making njs_descriptor_prop more generic.
https://gist.github.com/hongzhidao/5265dfa37505ecea6f5ea484525bc7ec
hongzhidao
on 4 Jun 2019
Making njs_descriptor_prop more generic.
Looks good, will be committed today with minor changes.
BTW, do you have some rationale for this change? It looks to me logically identical, except for order of checks.
- if (njs_is_valid(&desc->value)
- && current->writable == NJS_ATTRIBUTE_FALSE
- && !njs_values_strict_equal(&desc->value, ¤t->value))
- {
- goto exception;
+ if (current->writable == NJS_ATTRIBUTE_FALSE) {
+
+ if (njs_is_valid(&desc->value)
+ && !njs_values_strict_equal(&desc->value, ¤t->value))
+ {
+ goto exception;
+ }
}
will be committed today with minor changes.
I'm still on get/set property. I suggest to commit it later, I just want you to help check whether the improvement is OK :)
>
if (current->writable == NJS_ATTRIBUTE_FALSE) {
+
+ if (njs_is_valid(&desc->value)
+ && !njs_values_strict_equal(&desc->value, ¤t->value))
+ {
+ goto exception;
+ }
}
I think more codes will be added into inside if writable.
hongzhidao
on 4 Jun 2019
I think more codes will be added into inside if writable.
Now, I see.
xeioex
on 4 Jun 2019
@xeioex
Improved object property attributes.
https://gist.github.com/hongzhidao/5265dfa37505ecea6f5ea484525bc7ec
- See this https://tc39.github.io/ecma262/#exotic-object
@drsm the order of attributes need to be fixed, right? - Ignore this.
> I think more codes will be added into inside if writable.
hongzhidao
on 4 Jun 2019
@hongzhidao
https://gist.github.com/hongzhidao/5265dfa37505ecea6f5ea484525bc7ec#file-njs-0604-patch-L326
njs_value_t value addess should be aligned to 16 bytes.
xeioex
on 4 Jun 2019
@hongzhidao
https://gist.github.com/hongzhidao/5265dfa37505ecea6f5ea484525bc7ec#file-njs-0604-patch-L326
njs_value_t value addess should be aligned to 16 bytes.
What the order of fields do you suggest?
typedef struct {
/* Must be aligned to njs_value_t. */
njs_value_t name;
njs_object_property_type_t type:8; /* 3 bits */
njs_value_t getter;
njs_value_t setter;
njs_value_t value;
njs_object_attribute_t writable:8; /* 2 bits */
njs_object_attribute_t enumerable:8; /* 2 bits */
njs_object_attribute_t configurable:8; /* 2 bits */
} njs_object_prop_t;
@hongzhidao
I suggest to align all the njs_value_t values at the beginning of njs_object_prop_t.
1) We need to have 4 zero bits for all njs_value_t values addresses (16 bytes alignment), for scopes to work properly.
2) to save memory (see http://www.catb.org/esr/structure-packing/ Structure alignment and padding)
name
getter
setter
type
attributes
@xeioex take a look, again.
Improved object property attributes.
https://gist.github.com/hongzhidao/5265dfa37505ecea6f5ea484525bc7ec
hongzhidao
on 4 Jun 2019
@hongzhidao
tabs removed https://gist.github.com/xeioex/a8d2c0253bfae16612a1aaf87072fc7e . Otherwise looks ready for commit.
xeioex
on 4 Jun 2019
@xeioex @drsm
Review the design, please. There is a bit of work left, I'll recommit it tomorrow.
Added get/set property. (Ignore tabs problem)
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591Introduce njs_object_property.c.
Do you think it's a good idea to separate object property to a individual file?
Now it's kind of large.
welcome to suggest and test.
hongzhidao
on 4 Jun 2019
@hongzhidao
new tests results are much better
https://gist.github.com/xeioex/1f21ace634cea3dacbbea770f3edf4f3
- - Passed 7808 tests (28.8%)
- - Failed 19311 tests (71.2%)
+ - Passed 8188 tests (30.2%)
+ - Failed 18931 tests (69.8%)
@hongzhidao
=== built-ins/Array/prototype/fill/return-abrupt-from-setting-property-value failed in strict mode ===
---- errors ---
- Test262Error: Expected a to be thrown but no exception was thrown at all
- at anonymous (:22)
- at anonymous (:82)
- at main (native)===
+Warning: empty output===
Such segfaults are on me.
xeioex
on 4 Jun 2019
@hongzhidao
=== built-ins/Array/prototype/fill/return-abrupt-from-setting-property-value failed in strict mode === ---- errors --- - Test262Error: Expected a to be thrown but no exception was thrown at all - at anonymous (:22) - at anonymous (:82) - at main (native)=== +Warning: empty output===Such segfaults are on me.
When this case happens, how to dump the js code threw error?
hongzhidao
on 4 Jun 2019
@hongzhidao
cd test262-harness-py
TEST=$(find ./test262/ | grep 'built-ins/Array/prototype/fill/return-abrupt-from-this-length'); cat test262/harness/assert.js test262/harness/propertyHelper.js test262/harness/sta.js $TEST > test.js
test.js https://gist.github.com/f25f4ee8b13c476773c04fa98b9bf732
>> var o = {}; Object.defineProperty(o, 'length', {get:()=>{throw TypeError("Boom")}}); Array.prototype.fill.call(o, 1)
ASAN:SIGSEGV
=================================================================
==24614==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x00000044dda7 bp 0x7ffd76da2470 sp 0x7ffd76da2370 T0)
#0 0x44dda6 in njs_array_prototype_fill_continuation njs/njs_array.c:1475
#1 0x44d9c7 in njs_array_prototype_fill njs/njs_array.c:1435
#2 0x463232 in njs_function_native_call njs/njs_function.c:587
#3 0x41cc0a in njs_vmcode_continuation njs/njs_vm.c:2329
#4 0x413d10 in njs_vmcode_interpreter njs/njs_vm.c:159
#5 0x412be5 in njs_vm_start njs/njs.c:594
#6 0x4049a7 in njs_process_script njs/njs_shell.c:770
#7 0x4037ad in njs_interactive_shell njs/njs_shell.c:500
#8 0x402a03 in main njs/njs_shell.c:270
#9 0x7f73b8fce82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
@xeioex I get it.
Now, njs_value_property() may return NJS_APPLIED, we need to check all the places calling the function. The same to njs_value_property_set. It needs more work.
grep -r njs_value_property njs/
njs/njs_vm.c: ret = njs_value_property(vm, object, property, retval);
njs/njs_vm.c: ret = njs_value_property_set(vm, object, property, value);
njs/njs_vm.c: ret = njs_value_property(vm, constructor, &prototype_string, &value);
njs/njs_vm.c:njs_value_property(njs_vm_t *vm, const njs_value_t *value,
njs/njs_vm.c:njs_value_property_set(njs_vm_t *vm, njs_value_t *object,
njs/njs_array.c: ret = njs_value_property(vm, &args[0], &njs_string_length, &slice->length);
njs/njs_array.c: ret = njs_value_property(vm, this, &name, value);
njs/njs_array.c: ret = njs_value_property(vm, this, &njs_string_length, &fill->length);
njs/njs_array.c: ret = njs_value_property_set(vm, &vm->retval, &name,
njs/njs_object.h:njs_ret_t njs_value_property(njs_vm_t *vm, const njs_value_t *value,
njs/njs_object.h:njs_ret_t njs_value_property_set(njs_vm_t *vm, njs_value_t *object,
njs/njs_function.c: ret = njs_value_property(vm, arr_like, &njs_string_length, &length);
njs/njs_function.c: ret = njs_value_property(vm, arr_like, &name, &args[i]);
Suggestions for newly comment with NJS_APPLIED are welcome.
/*
* ES5.1, 8.12.3: [[Get]].
* NXT_OK property has been found in object,
* retval will contain the property's value
*
* NXT_DECLINED property was not found in object,
* NJS_TRAP the property trap must be called,
* NJS_APPLIED the property getter must be called, ???
* NXT_ERROR exception has been thrown.
* retval will contain undefined
*/
njs_ret_t
njs_value_property(njs_vm_t *vm, const njs_value_t *value,
const njs_value_t *property,
/*
* NXT_OK property has been set successfully
* NJS_TRAP the property trap must be called
* NJS_APPLIED the property setter must be called ???
* NXT_ERROR exception has been thrown.
*/
njs_ret_t
@hongzhidao
Now, njs_value_property() may return NJS_APPLIED, we need to check all the places calling the function. It needs more work.
yes, agree.
Do you think it's a good idea to separate object property to a individual file?
not sure, maybe later. Can you give me a list of function to be moved?
NJS_APPLIED the property getter must be called, ???
I would say the property getter was applied
xeioex
on 4 Jun 2019
@xeioex
Don't merge all the commits related to this ticket, it seems better to process them after the whole feature is done. Leave them for review and discussion.
separate object property to a individual file. not sure, maybe later.
Agree.
- {get prop() { ... } }
I'll try to implement it.
- These settings are not good, suggestions are welcome.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591#file-njs-118-patch-L32
hongzhidao
on 4 Jun 2019
@hongzhidao
Don't merge all the commits related to this ticket, it seems better to process them after the whole feature is done. Leave them for review and discussion.
OK.
xeioex
on 4 Jun 2019
@xeioex
Updated.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
These settings are not good, suggestions are welcome.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591#file-njs-118-patch-L32
Solved. Now getting the type of descriptor is simple.
#define NJS_PROPERTY_IS_DATA(prop) \
((prop)->writable != NJS_ATTRIBUTE_UNSET)
Now, njs_value_property() may return NJS_APPLIED, we need to check all the places calling the function. It needs more work.
/*
* Non-primitive length values are not supported yet. To handle non-primitive
* values a continuation is needed. Currently, only one continuation per frame
* is supported. apply() is a special function which can add a second
* continuation to the continuation of the underling function.
*
* TODO:
* String.prototype.concat.apply('a', { length:{ valueOf:
* function() { return 2; } },
* 0:'b', 1:'c'})
*/
static njs_ret_t
njs_function_prototype_apply(njs_vm_t *vm, njs_value_t *args, nxt_uint_t nargs,
njs_index_t retval)
See this TODO, it seems the same problem. I think I need to figure the solution out first.
It looks like it belongs to NJS_TRAP_XXX, I haven't deep into it.
hongzhidao
on 5 Jun 2019
@hongzhidao
BTW, found another
njs
>> var a = []; Object.defineProperty(a, 'length', {get:()=>3}); a.length
0
node
var a = []; Object.defineProperty(a, 'length', {get:()=>3}); a.length
Thrown:
TypeError: Cannot redefine property: length
at Function.defineProperty (<anonymous>)
@xeioex good reporting.
Updated (see the third patch)
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
some more questions:
- What about
NJS_PROPERTY_REF? I'm not familiar with this. - I checked the places calling
njs_value_propertyandnjs_value_property_set.
Sincelengthcan't be redefined, so I think it is OK after introducing get/set property. - > See this TODO, it seems the same problem. I think I need to figure the solution out first.
It looks like it belongs to NJS_TRAP_XXX, I haven't deep into it.
This can be a separate ticket.
After above questions.
- I think get/set property is done. (I'll rearrange these patches).
- Then I plan to implement
{get prop() { ... } }.
hongzhidao
on 5 Jun 2019
@hongzhidao
What about NJS_PROPERTY_REF? I'm not familiar with this.
It is only used in njs_value_property_set() for njs_array_t because it does not have njs_object_prop_t for array values. See https://github.com/nginx/njs/blob/master/njs/njs_object.c#L519
This can be a separate ticket.
Agree.
xeioex
on 5 Jun 2019
@xeioex @drsm
Improved object property attributes.
Added get/set property support.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
Welcome to test, thanks.
BTW, I think the first patch can be merged now.
hongzhidao
on 5 Jun 2019
@hongzhidao
- - Passed 8188 tests (30.2%)
- - Failed 18931 tests (69.8%)
+ - Passed 8093 tests (29.8%)
+ - Failed 19026 tests (70.2%)
https://gist.github.com/f50e28de5196f96d935c1bc45b014b94
Passes drop versus yesterday patch. On it. Will update the comment with actual tests.
> var obj = {}; Object.defineProperty(obj, "foo", {get:()=>10}); Object.preventExtensions(obj); Object.isFrozen(obj)
false // true expected
7.3.15 TestIntegrityLevel ( O, level )
The abstract operation TestIntegrityLevel is used to determine if the set of own properties of an object are fixed. This abstract operation performs the following steps:
Assert: Type(O) is Object.
Assert: level is either "sealed" or "frozen".
Let status be ? IsExtensible(O).
If status is true, return false.
NOTE: If the object is extensible, none of its properties are examined.
Let keys be ? O.[[OwnPropertyKeys]]().
For each element k of keys, do
Let currentDesc be ? O.[[GetOwnProperty]](k).
If currentDesc is not undefined, then
If currentDesc.[[Configurable]] is true, return false.
If level is "frozen" and IsDataDescriptor(currentDesc) is true, then
If currentDesc.[[Writable]] is true, return false.
Fix
@@ -2483,7 +2487,11 @@ njs_object_is_frozen(njs_vm_t *vm, njs_v
break;
}
- if (prop->writable || prop->configurable) {
+ if (prop->configurable) {
+ goto done;
+ }
+
+ if (NJS_PROPERTY_IS_DATA(prop) && prop->writable) {
goto done;
}
}
@hongzhidao
some code improvements
NJS_PROPERTY_IS_DATA() -> njs_is_data_descriptor() // to match the spec ( IsDataDescriptor(currentDesc))
diff --git a/njs/njs_object.c b/njs/njs_object.c
--- a/njs/njs_object.c
+++ b/njs/njs_object.c
@@ -1806,6 +1806,7 @@ njs_descriptor_prop(njs_vm_t *vm, const
{
nxt_bool_t data, accessor;
njs_object_prop_t *prop, *pr;
+ const njs_value_t *setter, *getter;
nxt_lvlhsh_query_t pq;
data = 0;
@@ -1817,9 +1818,7 @@ njs_descriptor_prop(njs_vm_t *vm, const
return NULL;
}
- prop->getter = njs_value_invalid;
- prop->setter = njs_value_invalid;
-
+ getter = &njs_value_invalid;
pq.key = nxt_string_value("get");
pq.key_hash = NJS_GET_HASH;
@@ -1831,9 +1830,12 @@ njs_descriptor_prop(njs_vm_t *vm, const
}
accessor = 1;
- prop->getter = pr->value;
+ getter = &pr->value;
}
+ prop->getter = *getter;
+
+ setter = &njs_value_invalid;
pq.key = nxt_string_value("set");
pq.key_hash = NJS_SET_HASH;
@@ -1845,9 +1847,11 @@ njs_descriptor_prop(njs_vm_t *vm, const
}
accessor = 1;
- prop->setter = pr->value;
+ setter = &pr->value;
}
+ prop->setter = *setter;
+
pq.key = nxt_string_value("value");
pq.key_hash = NJS_VALUE_HASH;
@xeioex take a look again.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
if (prop->writable || prop->configurable) {
After introducing property getter/setter, the writable value may be NJS_ATTRIBUTE_UNSET.
hongzhidao
on 5 Jun 2019
@hongzhidao
>> var obj = {}; Object.defineProperty(obj, "foo",{get: ()=>10, enumerable: true, configurable: true});
{
foo: <empty>
}
>> function get_func2() {return 20}; Object.defineProperty(obj, "foo", {get:get_func2, enumerable: false, configurable: false})
{
}
>> Object.getOwnPropertyDescriptor(obj, "foo").get === get_func2
false // should be true
9.1.6.3 ValidateAndApplyPropertyDescriptor ( O, P, extensible, Desc, current )
When the abstract operation ValidateAndApplyPropertyDescriptor is called with Object O, property key P, Boolean value extensible, and Property Descriptors Desc, and current, the following steps are taken:
NOTE
If undefined is passed as O, only validation is performed and no object updates are performed.
Assert: If O is not undefined, then IsPropertyKey(P) is true.
If current is undefined, then
If extensible is false, return false.
Assert: extensible is true.
If IsGenericDescriptor(Desc) is true or IsDataDescriptor(Desc) is true, then
If O is not undefined, create an own data property named P of object O whose [[Value]], [[Writable]], [[Enumerable]] and [[Configurable]] attribute values are described by Desc. If the value of an attribute field of Desc is absent, the attribute of the newly created property is set to its default value.
Else Desc must be an accessor Property Descriptor,
If O is not undefined, create an own accessor property named P of object O whose [[Get]], [[Set]], [[Enumerable]] and [[Configurable]] attribute values are described by Desc. If the value of an attribute field of Desc is absent, the attribute of the newly created property is set to its default value.
Return true.
If every field in Desc is absent, return true.
If current.[[Configurable]] is false, then
If Desc.[[Configurable]] is present and its value is true, return false.
If Desc.[[Enumerable]] is present and the [[Enumerable]] fields of current and Desc are the Boolean negation of each other, return false.
If IsGenericDescriptor(Desc) is true, no further validation is required.
Else if IsDataDescriptor(current) and IsDataDescriptor(Desc) have different results, then
If current.[[Configurable]] is false, return false.
If IsDataDescriptor(current) is true, then
If O is not undefined, convert the property named P of object O from a data property to an accessor property. Preserve the existing values of the converted property's [[Configurable]] and [[Enumerable]] attributes and set the rest of the property's attributes to their default values.
Else,
If O is not undefined, convert the property named P of object O from an accessor property to a data property. Preserve the existing values of the converted property's [[Configurable]] and [[Enumerable]] attributes and set the rest of the property's attributes to their default values.
Else if IsDataDescriptor(current) and IsDataDescriptor(Desc) are both true, then
If current.[[Configurable]] is false and current.[[Writable]] is false, then
If Desc.[[Writable]] is present and Desc.[[Writable]] is true, return false.
If Desc.[[Value]] is present and SameValue(Desc.[[Value]], current.[[Value]]) is false, return false.
Return true.
Else IsAccessorDescriptor(current) and IsAccessorDescriptor(Desc) are both true,
If current.[[Configurable]] is false, then
If Desc.[[Set]] is present and SameValue(Desc.[[Set]], current.[[Set]]) is false, return false.
If Desc.[[Get]] is present and SameValue(Desc.[[Get]], current.[[Get]]) is false, return false.
Return true.
If O is not undefined, then
For each field of Desc that is present, set the corresponding attribute of the property named P of object O to the value of the field.
Return true.
@hongzhidao
After introducing property getter/setter, the writable value may be NJS_ATTRIBUTE_UNSET.
Not sure this is a good idea to release UNSET values into the wild. Initially code was written with the following assumption
/*
* Attributes are generally used as Boolean values.
* The UNSET value is used internally only by njs_define_property().
*/
Otherwise we cannot reference attributes directly, only through NJS_PROPERTY_IS_DATA() like macros.
Can we confine UNSETs within njs_define_property()?
+ if (prop->writable == NJS_ATTRIBUTE_TRUE || prop->configurable) {
goto done;
}
I think the better approach is to closely follow specs, to simplify the reasoning and review. Specs here says
If level is "frozen" and IsDataDescriptor(currentDesc) is true, then
If currentDesc.[[Writable]] is true, return false.
we cannot reference attributes directly, only through NJS_PROPERTY_IS_DATA() like macros.
Agree.
Can we confine UNSETs within njs_define_property()?
First, we need to have a way to get the type of descriptor, prop->getter is not a good way, because it can be null, see.
/* scratch is used to get the value of an NJS_PROPERTY_HANDLER property. */
njs_object_prop_t scratch;
So, prop->writable is a right way, and I think it works well.
Object.getOwnPropertyDescriptor(obj, "foo").get === get_func2
Sorry that I missed some codes (including tests) related to njs_object_property_descriptor should be commited.
hongzhidao
on 5 Jun 2019
@hongzhidao
I think we need to agree on realizations of IsAccessorDescriptor(), IsDataDescriptor(), IsGenericDescriptor().
First, we need to have a way to get the type of descriptor, prop->getter is not a good way, because it can be null, see.
Is there any problems with invalid type?
I guess the problem is with IsDataDescriptor() definition, how to defined absent writable value.
If both Desc.[[Value]] and Desc.[[Writable]] are absent, return false
Is there any problems with invalid type?
Yes, actually we need to do the below. Now I didn't since we use writable to check is_data_descriptor.
diff -r 71a0fe0c7807 njs/njs_object.c
--- a/njs/njs_object.c Thu Jun 06 01:50:12 2019 +0800
+++ b/njs/njs_object.c Thu Jun 06 01:53:11 2019 +0800
@@ -524,7 +524,9 @@
prop->writable = 1;
prop->enumerable = 1;
prop->configurable = 1;
-
+ prop->getter = njs_value_invalid;
+ prop->setter = njs_value_invalid;
+
pq->lhq.value = prop;
I guess the problem is with IsDataDescriptor() definition
Agree.
I think we need to agree on realizations of IsAccessorDescriptor(), IsDataDescriptor(), IsGenericDescriptor().
I think only IsDataDescriptor is used.
how to defined absent writable value.
If both Desc.[[Value]] and Desc.[[Writable]] are absent, return false
If it is data, [value] and [writable] have value.
if it is accsssor, [getter] and [setter] have value.
So check it again, I think the below works.
#define njs_is_data_descriptor(prop) \
((prop)->writable != NJS_ATTRIBUTE_UNSET)
@xeioex
Updated.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
hongzhidao
on 5 Jun 2019
@hongzhidao
Updated.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
https://gist.github.com/xeioex/e4c8500c704733c8d6e35a69d6e6b6ed
- - Passed 8188 tests (30.2%)
- - Failed 18931 tests (69.8%)
+ - Passed 8171 tests (30.1%)
+ - Failed 18948 tests (69.9%)
@hongzhidao
there is a problem with this trick:
$ node --use-strict
> var setter = function(value) { Object.defineProperty(this, 'writeOnce', { value: value }); };
undefined
> var o = Object.defineProperty({}, 'writeOnce', { enumerable: true, configurable: true, set: setter });
undefined
> o.writeOnce
undefined
> o
{ writeOnce: [Setter] }
> o.writeOnce = 42
42
> o
{ writeOnce: 42 }
> o.writeOnce = 40
Thrown:
TypeError: Cannot assign to read only property 'writeOnce' of object '#<Object>'
> Object.getOwnPropertyDescriptor(o, 'writeOnce')
{ value: 42,
writable: false,
enumerable: true,
configurable: true }
vs.
>> var setter = function(value) { Object.defineProperty(this, 'writeOnce', { value: value }); };
undefined
>> var o = Object.defineProperty({}, 'writeOnce', { enumerable: true, configurable: true, set: setter });
undefined
>> o.writeOnce
undefined
>> o
{
writeOnce: <empty>
}
>> o.writeOnce = 42
42
>> o
{
writeOnce: 42
}
>> o.writeOnce = 40
40
>> o
{
writeOnce: 40
}
>> Object.getOwnPropertyDescriptor(o, 'writeOnce')
{
get: undefined,
set: [Function],
enumerable: true,
configurable: true
}
9.1.6.3 ValidateAndApplyPropertyDescriptor 6.c.
An implicit writable is the problem:
>> var o = Object.defineProperty({}, 'p', { enumerable: true, configurable: true, set: undefined });
undefined
>> void Object.defineProperty(o, 'p', { value: 42 });
undefined
>> o.p
undefined
>> void Object.defineProperty(o, 'p', { value: 42, writable: false });
undefined
>> o.p
42
@hongzhidao
Failed tests:
// For non-configurable properties, step 9a of
// [[DefineOwnProperty]] rejects changing the kind of a property
> var o = {}; Object.defineProperty(o, "foo", {get:()=>1}); Object.defineProperty(o, "foo", {value:1})
{
}
// expected TypeError: Cannot redefine property: foo
Get it, but I have to be on it next Monday:)
hongzhidao
on 6 Jun 2019
@hongzhidao
OK, I am on it.
xeioex
on 6 Jun 2019
@hongzhidao
Rewrote njs_define_property() according to ValidateAndApplyPropertyDescriptor
https://gist.github.com/c81483593cccc42b7d8d3ff14ea93a6b
Current status
- Passed 7808 tests (28.8%)
- - Failed 19311 tests (71.2%)
+ - Passed 8224 tests (30.3%)
+ - Failed 18895 tests (69.7%)
Current diff: https://gist.github.com/b57e23d223ae3d2bec220928b8ea9616
On addiional built-ins/Object/defineProperty fails.
xeioex
on 7 Jun 2019
@xeioex
So, now we should respect accessors everywhere:
// built-ins/Object/define Property/15.2.3.6-3-211 failed in strict mode
>> var a = Object.defineProperty({}, 'get', { get: () => () => 'accessor' });
undefined
>> var o = Object.defineProperty({}, 'prop', a);
TypeError: Getter must be a function
at Object.defineProperty (native)
at main (native)
and also
>> var a = Object.defineProperty({}, 'value', { get: () => 1 });
undefined
>> var o = Object.defineProperty({}, 'p', a);
undefined
>> o.p
undefined
>> Object.getOwnPropertyDescriptor(o, 'p')
{
value: undefined,
writable: false,
enumerable: false,
configurable: false
}
>> var o = Object.defineProperty({}, 'length', { get: () => 1 })
undefined
>> o.length
1
>> Array.prototype.fill.call(o, 42)
Segmentation fault
@drsm
So, now we should respect accessors everywhere:
Yes, that is exactly what I am doing.
xeioex
on 7 Jun 2019
@xeioex great job.
- I've reviewed it, looks good.
- I plan to implement
objects literal syntax, tell me if there are any others need to be processed first.
hongzhidao
on 10 Jun 2019
@hongzhidao
tell me if there are any others need to be processed first.
I suggest to try to implement #120. Because many test262 tests depends on it.
xeioex
on 10 Jun 2019
@hongzhidao
https://gist.github.com/xeioex/c81483593cccc42b7d8d3ff14ea93a6b#file-hd-118-7-patch-L731
Did you notice njs_function_activate(..., 0)? We should not assume the current instruction for this code to work in other functions. Now, it works out of the box for continuations, but for 'property get' instruction we need to manually update return address. Better ideas are welcome.
xeioex
on 10 Jun 2019
OK.
Some questions.
- Do you think
Object.defineProperty()is done? - Are
Object.defineProperty()and{get prop() { ... } }independent?
If Yes, I think it's no problem to implement the eval feature.
hongzhidao
on 10 Jun 2019
@xeioex
We should not assume the current instruction for this code to work in other functions. Now, it works out of the box for continuations, but for 'property get' instruction we need to manually update return address. Better ideas are welcome.
- I get it, agree.
- I would prefer to put retval and advance together since it support
active function.
Even we can improve the naming of the two functions ofnjs_value_propertyandnjs_value_property_set, but the current naming are OK.
diff -r d227b063e8b2 njs/njs_array.c
--- a/njs/njs_array.c Thu Jun 06 02:13:08 2019 +0800
+++ b/njs/njs_array.c Mon Jun 10 02:29:46 2019 -0400
@@ -528,7 +528,8 @@ njs_array_prototype_slice(njs_vm_t *vm,
slice = njs_vm_continuation(vm);
slice->u.cont.function = njs_array_prototype_slice_continuation;
- ret = njs_value_property(vm, &args[0], &njs_string_length, &slice->length);
+ ret = njs_value_property(vm, &args[0], &njs_string_length, &slice->length,
+ 0);
if (nxt_slow_path(ret == NXT_ERROR
|| ret == NJS_TRAP
|| ret == NJS_APPLIED))
@@ -678,7 +679,7 @@ njs_array_prototype_slice_copy(njs_vm_t
njs_uint32_to_string(&name, start++);
value = &array->start[n++];
- ret = njs_value_property(vm, this, &name, value);
+ ret = njs_value_property(vm, this, &name, value, 0);
if (ret != NXT_OK) {
*value = njs_value_invalid;
@@ -1433,7 +1434,8 @@ njs_array_prototype_fill(njs_vm_t *vm, n
fill = njs_vm_continuation(vm);
fill->u.cont.function = njs_array_prototype_fill_continuation;
- ret = njs_value_property(vm, this, &njs_string_length, &fill->length);
+ ret = njs_value_property(vm, this, &njs_string_length, &fill->length,
+ 0);
if (nxt_slow_path(ret == NXT_ERROR
|| ret == NJS_TRAP
|| ret == NJS_APPLIED))
@@ -1542,7 +1544,7 @@ njs_array_prototype_fill_object_continua
njs_uint32_to_string(&name, fill->start++);
ret = njs_value_property_set(vm, &vm->retval, &name,
- (njs_value_t *) value);
+ (njs_value_t *) value, 0);
if (nxt_slow_path(ret == NXT_ERROR || ret == NJS_APPLIED)) {
return ret;
}
diff -r d227b063e8b2 njs/njs_function.c
--- a/njs/njs_function.c Thu Jun 06 02:13:08 2019 +0800
+++ b/njs/njs_function.c Mon Jun 10 02:29:46 2019 -0400
@@ -1042,7 +1042,7 @@ njs_function_prototype_apply(njs_vm_t *v
return NXT_ERROR;
}
- ret = njs_value_property(vm, arr_like, &njs_string_length, &length);
+ ret = njs_value_property(vm, arr_like, &njs_string_length, &length, 0);
if (nxt_slow_path(ret == NXT_ERROR)) {
return ret;
}
@@ -1064,7 +1064,7 @@ njs_function_prototype_apply(njs_vm_t *v
for (i = 0; i < nargs; i++) {
njs_uint32_to_string(&name, i);
- ret = njs_value_property(vm, arr_like, &name, &args[i]);
+ ret = njs_value_property(vm, arr_like, &name, &args[i], 0);
if (nxt_slow_path(ret == NXT_ERROR)) {
return ret;
}
diff -r d227b063e8b2 njs/njs_object.h
--- a/njs/njs_object.h Thu Jun 06 02:13:08 2019 +0800
+++ b/njs/njs_object.h Mon Jun 10 02:29:46 2019 -0400
@@ -106,9 +106,9 @@ njs_array_t *njs_object_enumerate(njs_vm
njs_array_t *njs_object_own_enumerate(njs_vm_t *vm, const njs_object_t *object,
njs_object_enum_t kind, nxt_bool_t all);
njs_ret_t njs_value_property(njs_vm_t *vm, const njs_value_t *value,
- const njs_value_t *property, njs_value_t *retval);
+ const njs_value_t *property, njs_value_t *retval, size_t advance);
njs_ret_t njs_value_property_set(njs_vm_t *vm, njs_value_t *object,
- const njs_value_t *property, njs_value_t *value);
+ const njs_value_t *property, njs_value_t *value, size_t advance);
njs_object_prop_t *njs_object_property(njs_vm_t *vm, const njs_object_t *obj,
nxt_lvlhsh_query_t *lhq);
njs_ret_t njs_property_query(njs_vm_t *vm, njs_property_query_t *pq,
diff -r d227b063e8b2 njs/njs_vm.c
--- a/njs/njs_vm.c Thu Jun 06 02:13:08 2019 +0800
+++ b/njs/njs_vm.c Mon Jun 10 02:29:46 2019 -0400
@@ -540,16 +540,13 @@ njs_vmcode_property_get(njs_vm_t *vm, nj
code = (njs_vmcode_prop_get_t *) vm->current;
retval = njs_vmcode_operand(vm, code->value);
- ret = njs_value_property(vm, object, property, retval);
+ ret = njs_value_property(vm, object, property, retval,
+ sizeof(njs_vmcode_prop_get_t));
if (ret == NXT_OK || ret == NXT_DECLINED) {
vm->retval = *retval;
return sizeof(njs_vmcode_prop_get_t);
}
- if (ret == NJS_APPLIED) {
- njs_function_update_return_address(vm, sizeof(njs_vmcode_prop_get_t));
- }
-
return (ret == NJS_APPLIED) ? 0 : ret;
}
@@ -675,15 +672,12 @@ njs_vmcode_property_set(njs_vm_t *vm, nj
code = (njs_vmcode_prop_set_t *) vm->current;
value = njs_vmcode_operand(vm, code->value);
- ret = njs_value_property_set(vm, object, property, value);
+ ret = njs_value_property_set(vm, object, property, value,
+ sizeof(njs_vmcode_prop_set_t));
if (ret == NXT_OK) {
return sizeof(njs_vmcode_prop_set_t);
}
- if (ret == NJS_APPLIED) {
- njs_function_update_return_address(vm, sizeof(njs_vmcode_prop_set_t));
- }
-
return (ret == NJS_APPLIED) ? 0 : ret;
}
@@ -950,7 +944,7 @@ njs_vmcode_instance_of(njs_vm_t *vm, njs
if (njs_is_object(object)) {
value = njs_value_undefined;
- ret = njs_value_property(vm, constructor, &prototype_string, &value);
+ ret = njs_value_property(vm, constructor, &prototype_string, &value, 0);
if (nxt_fast_path(ret == NXT_OK)) {
@@ -3067,7 +3061,7 @@ njs_primitive_value(njs_vm_t *vm, njs_va
*/
njs_ret_t
njs_value_property(njs_vm_t *vm, const njs_value_t *value,
- const njs_value_t *property, njs_value_t *retval)
+ const njs_value_t *property, njs_value_t *retval, size_t advance)
{
njs_ret_t ret;
njs_object_prop_t *prop;
@@ -3111,7 +3105,7 @@ njs_value_property(njs_vm_t *vm, const n
return njs_function_activate(vm, prop->getter.data.u.function,
value, NULL, 0, (njs_index_t) retval,
- 0);
+ advance);
case NJS_PROPERTY_HANDLER:
pq.scratch = *prop;
@@ -3162,7 +3156,7 @@ njs_value_property(njs_vm_t *vm, const n
*/
njs_ret_t
njs_value_property_set(njs_vm_t *vm, njs_value_t *object,
- const njs_value_t *property, njs_value_t *value)
+ const njs_value_t *property, njs_value_t *value, size_t advance)
{
njs_ret_t ret;
njs_object_prop_t *prop, *shared;
@@ -3221,7 +3215,8 @@ njs_value_property_set(njs_vm_t *vm, njs
return njs_function_activate(vm,
prop->setter.data.u.function,
object, value, 1,
- (njs_index_t) &vm->retval, 0);
+ (njs_index_t) &vm->retval,
+ advance);
}
goto found;
BTW. About refactoring out njs_object_property.c.
Do you think it's a good idea to separate object property to a individual file?
not sure, maybe later. Can you give me a list of function to be moved?
The object property is so important and takes so many codes in NJS,
I'll try to do this to check whether it's worth.
hongzhidao
on 10 Jun 2019
@hongzhidao
Do you think Object.defineProperty() is done?
Almost done, I think. It is on me.
Are Object.defineProperty() and {get prop() { ... } } independent?
I am sorry, I forgot about getter/setter literal support. Yes, we also need this first before eval. It can be a separate patch over the current one.
xeioex
on 10 Jun 2019
we also need this first before eval. It can be a separate patch over the current one.
OK, and I suggest to commit the patch after the whole feature is done, including getter/setter literal support. Maybe some designs will be changed while implementing literal support.
hongzhidao
on 10 Jun 2019
@xeioex
Take a look.
https://gist.github.com/hongzhidao/aa17c77e72835d4521623d243542987c
As I understand, in NJS, there two important models: njs property and JS object property.
- njs property: (Is there a better way of calling this?)
It's not about JS language grammar, but we need it in njs since there are so many queries happened in various njs_value_t objects. It includes these APIs: njs_value_property, njs_value_property_set, njs_property_query. - JS object property:
It's about Object, and I prefer to use the prefixnjs_object_prop_xxx. It includes these APIs: njs_object_property, njs_object_prop_define, njs_object_prop_descriptor.
That it'll be easy to know which are njs properties and object properties.
So I did two things.
- Introduce njs_object_property.c file.
- Rename some APIs.
And I think most of the property getter/setter support patch would change the file njs_object_property.c.
hongzhidao
on 10 Jun 2019
@hongzhidao
njs property:
I would call it 'value' methods. Bacause it can be applied to any njs value (not only for Object instances).
https://gist.github.com/hongzhidao/aa17c77e72835d4521623d243542987c
Looks good, can you also update the getter/setter patch? Or we can do it after we commit getter/setter patches.
I would prefer to put retval and advance together since it support active function.
I thought about it. At first I did not like it, but now it seems to me to be a better alternative to njs_function_update_return_address.
xeioex
on 10 Jun 2019
@xeioex
can you also update the getter/setter patch? Or we can do it after we commit getter/setter patches.
No problem. And I prefer to introduce njs_object_property.c file first, then we can focus on getter/setter, including literal. I guess most of the code change would happen in the njs_object_property.c.
Do you think it's better to do this by two patches?
Introduce njs_object_property.c file. (only move functions without renaming)
Rename some APIs.
hongzhidao
on 10 Jun 2019
@hongzhidao
Do you think it's better to do this by two patches?
Introduce njs_object_property.c file. (only move functions without renaming)
Rename some APIs.
2 patches are better.
xeioex
on 10 Jun 2019
@xeioex
Introduce njs_object_property.c.
https://gist.github.com/hongzhidao/aa17c77e72835d4521623d243542987cIs the renaming OK?
njs_define_property vs njs_object_prop_define
njs_object_property_descriptor vs njs_object_prop_descriptorI'm not sure how to deal with the two functions.
njs_ret_t njs_prop_private_copy(njs_vm_t *vm, njs_property_query_t *pq);
const char *njs_prop_type_string(njs_object_property_type_t type);
Would you like to move them into njs_object_property.c?
hongzhidao
on 10 Jun 2019
@hongzhidao
Is the renaming OK?
Would you like to move them into njs_object_property.c?
looks good.
xeioex
on 10 Jun 2019
@xeioex
Introduced njs_object_property and renaming/style.
https://gist.github.com/hongzhidao/aa17c77e72835d4521623d243542987c
Updated property get/set patch.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
hongzhidao
on 11 Jun 2019
@hongzhidao
https://gist.github.com/hongzhidao/aa17c77e72835d4521623d243542987c
Is it ready to be committed?
xeioex
on 11 Jun 2019
@hongzhidao
https://gist.github.com/hongzhidao/aa17c77e72835d4521623d243542987c
Is it ready to be committed?
Yes.
hongzhidao
on 11 Jun 2019
@xeioex @drsm
Take a look, here's the draft patch of getter/setter literal support.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
welcome to suggest.
BTW. I faced some problems.
- I can't find a place with a detailed specification of get/set literal.
- The
ValidateAndApplyPropertyDescriptorseems to change after adding literal support, but I also can't find the rules.
My suggestions are:
- Check the design first.
- Do some refactors before committing these patches.
- It seems the property get/set patch can be committed first, then focus on the literal support.
hongzhidao
on 16 Jun 2019
@hongzhidao
1.
I can't find a place with a detailed specification of get/set literal.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/set
it's simple:
var o = {
get property() {},
set property(value) {}
};
and maybe also add support for shorthand methods:
var o = {
method(parameters) {}
};
// ==
var o = {
method: function(parameters) {}
};
2.
the [expr] notation called computed property names is a separate feature:
+ { nxt_string("var expr = 'foo'; var o = { get [expr]() { return 'bar'; } }; o.foo"),
+ nxt_string("bar") },
```js
var k = 'abc'.split('');
var o = {
[k[0]]: 'old-style',
get k[1] { return 'accessor'; },
k[2] { return 'method'; }
};
3. we need checks there:
```js
$ node --use-strict
> var o = { get x(a) {} };
Thrown:
var o = { get x(a) {} };
^^^
SyntaxError: Getter must not have any formal parameters.
> var o = { set x(a,b) {} };
Thrown:
var o = { set x(a,b) {} };
^^^^^
SyntaxError: Setter must have exactly one formal parameter.
4.
node:
> Object.getOwnPropertyDescriptors({ get x(){} })
{ x:
{ get: [Function: get x],
set: undefined,
enumerable: true,
configurable: true } }
njs:
>> Object.getOwnPropertyDescriptors({ get x(){} })
{
x: {
get: [Function],
set: undefined,
enumerable: false,
configurable: false
}
}
- there is no difference between
{ ident: ... }and{ get ident() ....}, so this should work too:
> { get 1(){} }
{ '1': [Getter] }
> { get null(){} }
{ null: [Getter] }
> { get false(){} }
{ false: [Getter] }
> { get '1-1'(){} }
{ '1-1': [Getter] }
@drsm thanks a lot.
add support for shorthand methods
the [expr] notation called computed property names is a separate feature:
Agree, it's better to create a separate issue.
Object.getOwnPropertyDescriptors({ get x(){} })
get: [Function],
Now, njs does not support function name, it's a separate topic.
Getter must not have any formal parameters.
Setter must have exactly one formal parameter.
there is no difference between { ident: ... } and { get ident() ....}
Get it, will be fixed later.
hongzhidao
on 16 Jun 2019
@hongzhidao
Object.getOwnPropertyDescriptors({ get x(){} })
get: [Function],Now, njs does not support function name, it's a separate topic.
>
It's not about function's name, there should be enumerable: true, configurable: true :).
drsm
on 16 Jun 2019
It's not about function's name, there should be enumerable: true, configurable: true :).
Get it.
will be fixed like this.
diff -r dcc764fa3829 njs/njs_object_property.c
--- a/njs/njs_object_property.c Sun Jun 16 04:14:06 2019 -0400
+++ b/njs/njs_object_property.c Sun Jun 16 11:28:36 2019 -0400
@@ -822,6 +822,8 @@ njs_object_prop_define(njs_vm_t *vm, njs
if (nxt_fast_path(prop != NULL)) {
prop->getter = *value;
prop->setter = njs_value_invalid;
+ prop->enumerable = NJS_ATTRIBUTE_TRUE;
+ prop->configurable = NJS_ATTRIBUTE_TRUE;
}
break;
@@ -832,6 +834,8 @@ njs_object_prop_define(njs_vm_t *vm, njs
if (nxt_fast_path(prop != NULL)) {
prop->setter = *value;
prop->getter = njs_value_invalid;
+ prop->enumerable = NJS_ATTRIBUTE_TRUE;
+ prop->configurable = NJS_ATTRIBUTE_TRUE;
}
break;
}
Now the big problem is about the rules about ValidateAndApplyPropertyDescriptor.
Previously @xeioex do a lot work on it. But now I don't know the change after adding literal support.
See this.
https://tc39.es/ecma262/#sec-validateandapplypropertydescriptor
But it does not mention what the change is after introducing literal support.
hongzhidao
on 16 Jun 2019
Now the big problem is about the rules about
ValidateAndApplyPropertyDescriptor.
Previously @xeioex do a lot work on it. But now I don't know the change after adding literal support.
there is no changes required, i think.
but we have to merge down get x/set x:
{ get x() {} }
// ==
Object.defineProperty({}, 'x', { enumerable: true, configurable: true, get: function() {} })
{ get x() {}, set x(v) {} }
// ==
Object.defineProperty({}, 'x', { enumerable: true, configurable: true, get: function() {}, set: function(v) {} })
so, just do not overwrite any setter/getter previously set, if the property is an accessor.
the test:
> Object.getOwnPropertyDescriptors({ get x() {}, x: 1, set x(v) {} })
{ x:
{ get: undefined,
set: [Function: set x],
enumerable: true,
configurable: true } }
> Object.getOwnPropertyDescriptors({ x: 1, get x() {}, set x(v) {} })
{ x:
{ get: [Function: get x],
set: [Function: set x],
enumerable: true,
configurable: true } }
actually, this is forbidden by the spec (multiple properties with the same name in strict mode), but works well everywhere.
drsm
on 16 Jun 2019
updated.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
Getter must not have any formal parameters.
Setter must have exactly one formal parameter.
It's not about function's name, there should be enumerable: true, configurable: true :).
Fixed.
there is no difference between { ident: ... } and { get ident() ....}, so this should work too:
This needs more time, will fix it later.
hongzhidao
on 16 Jun 2019
@drsm @xeioex
I plan to implement the two syntaxes first, they may be re-used in get/set literal.
maybe also add support for shorthand methods:
var o = {
method(parameters) {}
};
// ==
var o = {
method: function(parameters) {}
};
the [expr] notation called computed property names is a separate feature:
var k = 'abc'.split('');
var o = {
[k[0]]: 'old-style',
get k[1] { return 'accessor'; },
k[2] { return 'method'; }
};It does not affect the review of get/set patch.
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
hongzhidao
on 17 Jun 2019
@hongzhidao
https://gist.github.com/hongzhidao/7ac1d56431bf533ae9dccd01f811a591
Unfortunately, the first patch does not include my latest changes in it.
https://gist.github.com/xeioex/60d52fa29153774db9d1db0dfbc3fa2e
See njs_object_prop_define in if (njs_is_data_descriptor(current) != njs_is_data_descriptor(desc)) {
I think it is a good time to commit the first patch to avoid desync.
Looks like separate style changes:
diff -r c3a70f0f6cdc -r 9f7d4f2bb335 njs/njs_object.c
--- a/njs/njs_object.c Fri Jun 14 21:20:25 2019 +0300
+++ b/njs/njs_object.c Sun Jun 16 02:29:17 2019 -0400
@@ -1114,7 +1114,7 @@ njs_object_define_property(njs_vm_t *vm,
{
nxt_int_t ret;
njs_value_t *value;
- const njs_value_t *name, *descriptor;
+ const njs_value_t *name, *desc;
if (!njs_is_object(njs_arg(args, nargs, 1))) {
njs_type_error(vm, "cannot convert %s argument to object",
@@ -1129,16 +1129,16 @@ njs_object_define_property(njs_vm_t *vm,
return NXT_ERROR;
}
- descriptor = njs_arg(args, nargs, 3);
-
- if (!njs_is_object(descriptor)) {
+ desc = njs_arg(args, nargs, 3);
+
+ if (!njs_is_object(desc)) {
njs_type_error(vm, "descriptor is not an object");
return NXT_ERROR;
}
I suggest to split out into separate changes:
1) njs_object_prop_define() prototype changes
2) Style: descriptor -> desc
xeioex
on 18 Jun 2019
@hongzhidao
Take a look: https://gist.github.com/7557b64d0fc141379aea29b30e3fdcc7
I plan to commit first two (style) patches.
xeioex
on 18 Jun 2019
@xeioex
Looks good except njs_object_prop_define.
What about this?
- descriptor -> value, after the literal support.
valuewill be better.
njs_object_prop_define(njs_vm_t *vm, njs_value_t *object,
const njs_value_t *name, const njs_object_t *value)
- desc -> prop, current -> prev.
njs_object_prop_t *desc, *current;
@hongzhidao
OK, will apply.
What about Added property getter/setter support in Object.defineProperty(). ?
@lexborisov also depends on it.
xeioex
on 18 Jun 2019
What about Added property getter/setter support in Object.defineProperty(). ?
It's OK.
hongzhidao
on 18 Jun 2019
deleted
drsm
on 27 Jun 2019
@xeioex @hongzhidao
please, take a look:
# HG changeset patch
# User Artem S. Povalyukhin <[email protected]>
# Date 1561659526 -10800
# Thu Jun 27 21:18:46 2019 +0300
# Node ID 7a92c61174bce22f4c8564448d5a0ddfc60d8efd
# Parent 40f26bb516a697a2ebd9a66d03c9426f10f76565
Fixed property setter lookup.
diff -r 40f26bb516a6 -r 7a92c61174bc njs/njs_object_property.c
--- a/njs/njs_object_property.c Thu Jun 27 18:55:34 2019 +0300
+++ b/njs/njs_object_property.c Thu Jun 27 21:18:46 2019 +0300
@@ -610,6 +610,19 @@
"Cannot set property \"%V\" of %s which has only a getter",
&pq.lhq.key, njs_type_string(object->type));
return NXT_ERROR;
+
+ } else {
+ if (nxt_slow_path(prop->type != NJS_PROPERTY)) {
+ njs_internal_error(vm, "unexpected property type \"%s\" "
+ "while setting",
+ njs_prop_type_string(prop->type));
+ }
+
+ return njs_function_activate(vm,
+ prop->setter.data.u.function,
+ object, value, 1,
+ (njs_index_t) &vm->retval,
+ advance);
}
if (prop->type == NJS_PROPERTY_HANDLER) {
@@ -629,14 +642,6 @@
break;
}
- if (njs_is_function(&prop->setter)) {
- return njs_function_activate(vm,
- prop->setter.data.u.function,
- object, value, 1,
- (njs_index_t) &vm->retval,
- advance);
- }
-
goto found;
case NJS_PROPERTY_REF:
diff -r 40f26bb516a6 -r 7a92c61174bc njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c Thu Jun 27 18:55:34 2019 +0300
+++ b/njs/test/njs_unit_test.c Thu Jun 27 21:18:46 2019 +0300
@@ -9364,6 +9364,10 @@
"Object.defineProperty(o, 'a', {get:()=>1}); o.a = 2"),
nxt_string("TypeError: Cannot set property \"a\" of object which has only a getter") },
+ { nxt_string("var o = Object.create(Object.defineProperty({}, 'x', { set: function(v) { this.y = v; }})); "
+ "o.x = 123; Object.getOwnPropertyDescriptor(o, 'y').value"),
+ nxt_string("123") },
+
{ nxt_string("var o = {};"
"Object.defineProperty(o, 'a', { configurable: true, value: 0 });"
"Object.defineProperty(o, 'a', { value: 1 });"
@drsm
Can you commit the patch of Fixed property setter lookup. again after issue https://github.com/nginx/njs/issues/182 closed.
@xeioex
I plan to continue get/set property literal support, it also depends on issue 182.
hongzhidao
on 4 Jul 2019
@hongzhidao
Currently I am working on synchronous version of njs_primitive_value() and njs_value_property() which would simplify many many places in code. I plan to remove NJS_TRAP and NJS_APPLIED.
xeioex
on 4 Jul 2019
@xeioex
I plan to remove NJS_TRAP and NJS_APPLIED.
It would be a great improvement, sounds very good.
Does it also include removing trap_values? Any patch now?
hongzhidao
on 5 Jul 2019
@hongzhidao
Any patch now?
in progress, many changes.
xeioex
on 5 Jul 2019
@hongzhidao
finally, once #190 and #182 we can proceed with getters/setters further.
literals support: https://github.com/nginx/njs/issues/118#issuecomment-502441243
xeioex
on 19 Jul 2019
we can proceed with getters/setters further
Will do.
BTW, do you plan to do njs.c, njs_vm.c and njs_vmcode.c split?
What about introducing njs_vmcode first?
Now, I need to add njs_vmcode_property_accessor for getter/setter literals.
hongzhidao
on 20 Jul 2019
@hongzhidao
BTW, do you plan to do njs.c, njs_vm.c and njs_vmcode.c split?
yes.
xeioex
on 20 Jul 2019
@hongzhidao
Can you commit the patch of
Fixed property setter lookup.again after issue #182 closed.
Updated:
(not sure the prop->type != NJS_PROPERTY check is necessary)
# HG changeset patch
# User Artem S. Povalyukhin <[email protected]>
# Date 1563618719 -10800
# Sat Jul 20 13:31:59 2019 +0300
# Node ID 550a69b2626c42ffb2027ac8b3510cd27aad60bd
# Parent 97ab91a7c7f560aa4817a55fdaff712ac3cae09f
Fixed property setter lookup.
diff -r 97ab91a7c7f5 -r 550a69b2626c njs/njs_object_property.c
--- a/njs/njs_object_property.c Tue Jul 02 22:24:11 2019 -0400
+++ b/njs/njs_object_property.c Sat Jul 20 13:31:59 2019 +0300
@@ -604,6 +604,16 @@
"Cannot set property \"%V\" of %s which has only a getter",
&pq.lhq.key, njs_type_string(object->type));
return NXT_ERROR;
+
+ } else {
+ if (nxt_slow_path(prop->type != NJS_PROPERTY)) {
+ njs_internal_error(vm, "unexpected property type \"%s\" "
+ "while setting",
+ njs_prop_type_string(prop->type));
+ }
+
+ return njs_function_call(vm, njs_function(&prop->setter),
+ object, value, 1, &vm->retval);
}
if (prop->type == NJS_PROPERTY_HANDLER) {
@@ -623,11 +633,6 @@
break;
}
- if (njs_is_function(&prop->setter)) {
- return njs_function_call(vm, njs_function(&prop->setter),
- object, value, 1, &vm->retval);
- }
-
goto found;
case NJS_PROPERTY_REF:
diff -r 97ab91a7c7f5 -r 550a69b2626c njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c Tue Jul 02 22:24:11 2019 -0400
+++ b/njs/test/njs_unit_test.c Sat Jul 20 13:31:59 2019 +0300
@@ -9629,6 +9629,10 @@
"Object.defineProperty(o, 'a', {get:()=>1}); o.a = 2"),
nxt_string("TypeError: Cannot set property \"a\" of object which has only a getter") },
+ { nxt_string("var o = Object.create(Object.defineProperty({}, 'x', { set: function(v) { this.y = v; }})); "
+ "o.x = 123; Object.getOwnPropertyDescriptor(o, 'y').value"),
+ nxt_string("123") },
+
{ nxt_string("var o = {};"
"Object.defineProperty(o, 'a', { configurable: true, value: 0 });"
"Object.defineProperty(o, 'a', { value: 1 });"
@drsm @xeioex
Do you know if there is a benchmark script?
We can compare njs to quickjs.
hongzhidao
on 22 Jul 2019
@hongzhidao
yes.
git clone https://github.com/v8/v8
cd v8/
patch -p1 < bench.patch # https://gist.github.com/xeioex/0941fd15dd817655f1f1bb936e15b4cb
cd benchmarks
cat base.js richards.js crypto.js splay.js navier-stokes.js run.js > bench.js
# njs LTO build
AR=gcc-ar CFLAGS='-O2 -flto' ./configure && make njs
# profile
valgrind --tool=callgrind ./build/njs ./bench.js
# vs
valgrind --tool=callgrind qjs ./bench.js
# visualize
kcachegrind callgrind.out.<pid>
@hongzhidao
Take a look: https://gist.github.com/72def343e5c34d3d9320d0aadfddf93e
xeioex
on 22 Jul 2019
Take a look: https://gist.github.com/72def343e5c34d3d9320d0aadfddf93e
Great job, looks good.
hongzhidao
on 22 Jul 2019
@xeioex @drsm
Added getter/setter literal support.
https://gist.github.com/hongzhidao/c1745ef5acb57996adc2809cff362bd0
What's left:
getter/setter disassembler is not supported yet.
Welcome to test. thanks.
hongzhidao
on 29 Jul 2019
What's left: getter/setter disassembler is not supported yet.
diff -r 8d2bd29ed12a njs/njs_disassembler.c
--- a/njs/njs_disassembler.c Mon Jul 29 03:31:59 2019 -0400
+++ b/njs/njs_disassembler.c Mon Jul 29 04:48:56 2019 -0400
@@ -177,6 +177,7 @@ njs_disassemble(u_char *start, u_char *e
njs_vmcode_equal_jump_t *equal;
njs_vmcode_prop_foreach_t *prop_foreach;
njs_vmcode_method_frame_t *method;
+ njs_vmcode_prop_accessor_t *prop_accessor;
njs_vmcode_try_trampoline_t *try_tramp;
njs_vmcode_function_frame_t *function;
@@ -326,6 +327,22 @@ njs_disassemble(u_char *start, u_char *e
continue;
}
+ if (operation == NJS_VMCODE_PROPERTY_ACCESSOR) {
+ prop_accessor = (njs_vmcode_prop_accessor_t *) p;
+
+ nxt_printf("%05uz PROPERTY %s %04Xz %04Xz %04Xz\n",
+ p - start,
+ (prop_accessor->type == NJS_OBJECT_PROP_GETTER)
+ ? "ACCESSOR/GET" : "ACCESSOR/SET",
+ (size_t) prop_accessor->value,
+ (size_t) prop_accessor->object,
+ (size_t) prop_accessor->property);
+
+ p += sizeof(njs_vmcode_prop_accessor_t);
+
+ continue;
+ }
+
if (operation == NJS_VMCODE_TRY_START) {
try_start = (njs_vmcode_try_start_t *) p;
How to display better? @xeioex
hongzhidao
on 29 Jul 2019
@hongzhidao
Works fine, thanks!
What's left:
getter/setter disassembler is not supported yet.
- njs.dump() & JSON.stringify() support
drsm
on 29 Jul 2019
@hongzhidao
https://gist.github.com/hongzhidao/c1745ef5acb57996adc2809cff362bd0
Great job!
node
> function get() {return 1}; get()
1
njs
>> function get() {return 1}; get()
SyntaxError: Unexpected token "get" in shell:1
get/set are not tokens per se.
xeioex
on 29 Jul 2019
@xeioex
get/set are not tokens per se.
- What about considering it with this issue https://github.com/nginx/njs/issues/132 together? Since there is similar issue in NJS now.
function eval() {} // should pass but broken in NJS
- Do you think
getandsetare keywords? @drsm
hongzhidao
on 29 Jul 2019
@hongzhidao
reserved words: https://www.w3schools.com/js/js_reserved.asp
xeioex
on 29 Jul 2019
@hongzhidao
What about considering it with this issue #132 together?
I would rather not. 'get' is fairly typical name for a function, I am afraid many perfectly valid js files will be rejected.
xeioex
on 29 Jul 2019
I get it. Agree.
hongzhidao
on 29 Jul 2019
@xeioex
What about the disassemble name of getter/setter?
PROPERTY ACCESSOR ... GET
PROPERTY ACCESSOR ... SET
@hongzhidao
- What about considering it with this issue #132 together? Since there is similar issue in NJS now.
function eval() {} // should pass but broken in NJS
No it's OK, eval is special in strict mode:
https://tc39.es/ecma262/#sec-identifiers-static-semantics-early-errors
- Do you think
getandsetare keywords? @drsm
No.
drsm
on 29 Jul 2019
@xeioex @drsm
Updated.
https://gist.github.com/hongzhidao/0b255fe34689f3d8833012de76d5214e
hongzhidao
on 29 Jul 2019
@hongzhidao
BTW, njs_descriptor_prop should also support descriptors with getters.
For example:
> var o = {}; Object.defineProperty(o, 'a', { get value() { return 'bar'; } })
// {a:'bar'}
What about the disassemble name of getter/setter?
Looks good.
xeioex
on 29 Jul 2019
@xeioex
@hongzhidao
Found a problem:
>> +{ get valueOf() { return () => 2; } }
NaN
>> ({ get test() { return () => 1; }}).test()
TypeError: (intermediate value)["test"] is not a function
at main (native)
>> ({ get test() { return () => 1; }}).test
[Function]
>> var f = ({ get test() { return () => 1; }}).test; f()
1
@xeioex @drsm
+{ get valueOf() { return () => 2; } }
Fixed (don't merge these patches before the ticket can be closed.)
https://gist.github.com/hongzhidao/3a15d1070d1f1479cac9684ed38ef375
njs_descriptor_prop should also support descriptors with getters.
({ get test() { return () => 1; }}).test()
Need more work. The core function njs_property_query() need to be changed after introducing getter literal. But there are many places need to get an object property value. I'm not clear yet.
hongzhidao
on 30 Jul 2019
@hongzhidao
Introduced njs_object_property_value() and remove njs_object_property()
I guess njs_object_property() should be replaced with njs_value_property().
The same is for njs_vmcode_method_frame().
xeioex
on 30 Jul 2019
@hongzhidao
njs_run: unexpected output 'SyntaxError: Unexpected token ";" in lib1.js:19' vs 'passed!'
while executing
"njs_run {"-p" "njs/test/module/libs" "./njs/test/module/normal.js"} \
"passed!""
(file "njs/test/njs_expect_test.exp" line 630)
build/Makefile:751: recipe for target 'expect_test' failed
and may be related:
>> ({ get() { return 1; }, set(v) {} })
SyntaxError: Unexpected token "(" in shell:1
>> ({ get: 1, set: 2 })
SyntaxError: Unexpected token "get" in shell:1
gcc warnings:
njs/njs_parser_terminal.c:671:20: error: ‘token_line’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
expression = njs_parser_reference(vm, parser, prop_token, &name,
~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hash, token_line);
~~~~~~~~~~~~~~~~~
njs/njs_parser_terminal.c:597:34: note: ‘token_line’ was declared here
uint32_t hash, token_line;
^~~~~~~~~~
njs/njs_parser_terminal.c:671:20: error: ‘hash’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
expression = njs_parser_reference(vm, parser, prop_token, &name,
~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hash, token_line);
~~~~~~~~~~~~~~~~~
njs/njs_parser_terminal.c:597:28: note: ‘hash’ was declared here
uint32_t hash, token_line;
^~~~
In file included from njs/njs_core.h:40:0,
from njs/njs_parser_terminal.c:7:
njs/njs_parser.h:168:15: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
@drsm
njs_run: unexpected output 'SyntaxError: Unexpected token ";" in lib1.js:19' vs 'passed!'
while executing
"njs_run {"-p" "njs/test/module/libs" "./njs/test/module/normal.js"} \
"passed!""
(file "njs/test/njs_expect_test.exp" line 630)
build/Makefile:751: recipe for target 'expect_test' failed
This is the same as in.
xeioex
on 30 Jul 2019
@xeioex
This is the same as in.
Yes. It's partially fixed in the latest version (works in CLI):
>> function get() {return 1}; get()
1
now it fails on export default {hash, inc, get};
drsm
on 30 Jul 2019
I guess njs_object_property() should be replaced with njs_value_property().
The same is for njs_vmcode_method_frame().
Agree. Now there are three functions related to value property.
njs_property_query(), njs_value_property(), njs_object_property().
It seems it's time to refactor.
And I found issues happend in property. There may be more.
Object.defineProperty(o, 'foo', { get: function() {return () => 3} });
o.foo();
Throws.
TypeError: (intermediate value)["foo"] is not a function
at main (native)
On this.
hongzhidao
on 31 Jul 2019
@xeioex
diff -r f4ac8168e856 src/njs_object_property.c
--- a/src/njs_object_property.c Tue Jul 30 21:12:08 2019 +0300
+++ b/src/njs_object_property.c Thu Aug 01 06:45:54 2019 -0400
@@ -498,7 +498,7 @@ njs_value_property(njs_vm_t *vm, const n
switch (prop->type) {
case NJS_METHOD:
- if (pq.shared) {
+ if (pq.shared) { // what does shared mean?
ret = njs_prop_private_copy(vm, &pq);
if (njs_slow_path(ret != NJS_OK)) {
diff -r f4ac8168e856 src/njs_vmcode.c
--- a/src/njs_vmcode.c Tue Jul 30 21:12:08 2019 +0300
+++ b/src/njs_vmcode.c Thu Aug 01 06:45:54 2019 -0400
@@ -1762,8 +1762,19 @@ njs_vmcode_method_frame(njs_vm_t *vm, nj
prop = pq.lhq.value;
switch (prop->type) {
+ case NJS_METHOD:
+ /*
+ if (pq.shared) { // what dont' need shared here?
+ ret = njs_prop_private_copy(vm, &pq);
+
+ if (njs_slow_path(ret != NJS_OK)) {
+ return ret;
+ }
+
+ prop = pq.lhq.value;
+ }
+ */
case NJS_PROPERTY:
- case NJS_METHOD:
break;
case NJS_PROPERTY_HANDLER:
Can you explain the two places of shared?
And under what case, we need to add shared condition?
hongzhidao
on 1 Aug 2019
@xeioex
Take a look at the early refactor.
https://gist.github.com/hongzhidao/d23661eaa672019e6581b838c1383ea5
- eliminate njs_object_property().
- introduced njs_property_string_query(). (This will be replaced by njs_value_property()).
Finally, there will only be one public function of querying value property. It's njs_value_property().
On it.
hongzhidao
on 1 Aug 2019
@hongzhidao
// what does shared mean?
pq.shared is 1 when a property was found in object->shared_hash.
shared_hash - is a hash of shared properties (for example Object.prototype functions are in it).
We have shared_hash to avoid copying Object.prototype properties during an object creation.
We have to treat shared properties in a different way:
1) shared_hash should never be changed (because it is shared between cloned VMs to save memory).
2) if we need to change a shared property, we do copy-on-write:
a) create a mutable copy of a share property in object->hash (so it will "overwrite" the property with the same name in shared_hash).
b) create a NJS_WHITEOUT stub in object->hash if we want to delete a shared property (so it hide the property with the same name in shared_hash).
Currently in shared_hash we have only NJS_METHOD and constants which are immutable.
So we only need to copy-on-write NJS_METHOD properties.
IMO this can be improved: I think NJS_METHOD can be eliminated, and replaced with NJS_PROPERTY to reduce the number of property types.
xeioex
on 1 Aug 2019
@hongzhidao
Finally, there will only be one public function of querying value property. It's njs_value_property().
On it.
Looks promising.
xeioex
on 1 Aug 2019
@xeioex
Updated.
https://gist.github.com/hongzhidao/d23661eaa672019e6581b838c1383ea5
The next step is making njs_property_query() static.
Notice about njs_value_property().
The value of return may be:
NJS_OK: found
NJS_DECLINED: not found
NJS_ERROR: error happened
And there two ways of calling njs_value_property().
Way 1: arbitrary property.
if (op == NJS_VMCODE_PROPERTY_GET) {
get = (njs_vmcode_prop_get_t *) pc;
retval = njs_vmcode_operand(vm, get->value);
ret = njs_value_property(vm, value1, value2, retval);
if (njs_slow_path(ret == NJS_ERROR)) {
goto error;
}
pc += sizeof(njs_vmcode_prop_get_t);
goto next;
}
Way 2: string property. This way only return NJS_OK or NJS_DECLINED.
static njs_function_t *
njs_object_to_json_function(njs_vm_t *vm, njs_value_t *value)
{
njs_value_t name, retval;
name = (njs_value_t) njs_string("toJSON");
(void) njs_value_property(vm, value, &name, &retval);
if (njs_is_function(&retval)) { // noticed here. It's enough using njs_is_function().
return njs_function(&retval);
}
return NULL;
}
One trick of processing the default value while query not found (NJS_DECLIENED).
Its default value is njs_value_undefined.
So somethings we use ret = njs_value_property(...) and sometimes use (void) njs_value_property(..., retval);.
The purpose is making codes concise, we needn't to write:
if (ret == NJS_ERROR) {}
if (ret == NJS_DECLINED) {}
/* NJS_OK */
@xeioex
diff -r f4ac8168e856 src/njs_vmcode.c
--- a/src/njs_vmcode.c Tue Jul 30 21:12:08 2019 +0300
+++ b/src/njs_vmcode.c Thu Aug 01 06:45:54 2019 -0400
@@ -1762,8 +1762,19 @@ njs_vmcode_method_frame(njs_vm_t *vm, nj
prop = pq.lhq.value;
switch (prop->type) {
+ case NJS_METHOD:
+ /*
+ if (pq.shared) { // what dont' need shared here?
+ ret = njs_prop_private_copy(vm, &pq);
+
+ if (njs_slow_path(ret != NJS_OK)) {
+ return ret;
+ }
+
+ prop = pq.lhq.value;
+ }
+ */
case NJS_PROPERTY:
- case NJS_METHOD:
break;
case NJS_PROPERTY_HANDLER:
Throws
njs_interactive("'str'.replace(/t/g, function(m) {return m.a.a})
") failed: "TypeError: cannot get property "a" of undefined
at anonymous (:1)
at String.prototype.replace (native)
at main (native)
" vs "TypeError: cannot get property "a" of undefined
at anonymous (:1)
at native (native)
at main (native)
"
Why dont' need shared in NJS_METHOD of njs_vmcode_method_frame?
hongzhidao
on 1 Aug 2019
@hongzhidao
name = (njs_value_t) njs_string("toJSON");(void) njs_value_property(vm, value, &name, &retval);
Maybe we need a special function for this, the same as njs_value_property() but with njs_str_t for property name.
Consider this API:
njs_value_property(vm, value, njs_value_t *name, uint32_t hash /*can be provided if known, otherwise 0*/, retval);
njs_value_str_property(vm, value, njs_str_t *name, uint32_t hash /*can be provided if known, otherwise 0*/, retval)?
xeioex
on 1 Aug 2019
@hongzhidao
/can be provided if known, otherwise 0/
I have a patch, when we calculate hash for PROPERTY GET instructions in compile time. It gives modest speedup on eliminating most of njs_djs_hash() calls.
https://gist.github.com/xeioex/664c6149f28af352d6fb33aed7326d48
The patch is already broken after recent changes, but worked.
xeioex
on 1 Aug 2019
@xeioex
IMO this can be improved: I think NJS_METHOD can be eliminated, and replaced with NJS_PROPERTY to reduce the number of property types.
I try to simplify it.
Take a look.
https://gist.github.com/hongzhidao/4402803437d9cf34dcafc182b518ab5d
If OK, I'll split it into two patches.
And I prefer to commit this after the patch of eliminating njs_mem_proto_t.
My thoughts.
- njs_property_value() and njs_value_property() belongs to njs_value.c since the value's type is arbitray.
- njs_object_prop.c is better than njs_object_property.c since query property has been included in njs_value.c. So the file njs_object_prop.c only do the things relate to njs_object_prop_xxx.
hongzhidao
on 2 Aug 2019
@hongzhidao
njs_property_value() and njs_value_property() belongs to njs_value.c since the value's type is arbitray.
Agree.
njs_object_prop.c is better than njs_object_property.c since query property has been included in njs_value.c. So the file njs_object_prop.c only do the things relate to njs_object_prop_xxx.
Agree, as a separate patch.
https://gist.github.com/hongzhidao/4402803437d9cf34dcafc182b518ab5d
Looks good.
xeioex
on 2 Aug 2019
@xeioex
Refactored out njs_object_prop.c
https://gist.github.com/hongzhidao/5a479afb043c28d2fe46cc3b3cc48d72
hongzhidao
on 2 Aug 2019
@xeioex
IMO this can be improved: I think NJS_METHOD can be eliminated, and replaced with NJS_PROPERTY to reduce the number of property types.
Regarding object prop method as NJS_PROPERTY
https://gist.github.com/hongzhidao/60cd8fcb22b9d4114addc8940ce5ef6a
hongzhidao
on 2 Aug 2019
@hongzhidao
https://gist.github.com/hongzhidao/5a479afb043c28d2fe46cc3b3cc48d72
src/njs_object_property.c is not removed, src/njs_object_prop.c is created from scratch.
Please use hg mv to move the file. We need this to preserve history of changes.
diff --git a/src/njs_object_prop.c b/src/njs_object_prop.c
new file mode 100644
--- /dev/null
+++ b/src/njs_object_prop.c
@xeioex
Updated.
https://gist.github.com/hongzhidao/d4129f388aa1636d94d944d9c329559c
hongzhidao
on 2 Aug 2019
@xeioex
Moving all common headers into njs_main.h.
http://hg.nginx.org/njs/rev/835b3e817b93
What about the files in src/test? Such as.
#include <njs_auto_config.h>
#include <njs_main.h>
#include <njs_lvlhsh.h>
#include <njs_djb_hash.h>
#include <njs_pcre.h>
#include <string.h>
#include <stdlib.h>
#include <sys/resource.h>
#include <time.h>
> Regarding object prop method as NJS_PROPERTY
https://gist.github.com/hongzhidao/60cd8fcb22b9d4114addc8940ce5ef6a
It seems it's better to commit this with property refactor, it's too early to commit.
But, Is the below correct?
diff -r 8905b4df1b97 src/njs_object_prop.c
--- a/src/njs_object_prop.c Fri Aug 02 20:27:52 2019 +0800
+++ b/src/njs_object_prop.c Fri Aug 02 08:43:53 2019 -0400
@@ -628,6 +628,12 @@ njs_prop_private_copy(njs_vm_t *vm, njs_
static const njs_value_t name_string = njs_string("name");
+ shared = pq->lhq.value;
+
+ if (!njs_is_function(&shared->value)) {
+ return NJS_OK;
+ }
+
prop = njs_mp_align(vm->mem_pool, sizeof(njs_value_t),
sizeof(njs_object_prop_t));
if (njs_slow_path(prop == NULL)) {
@@ -635,7 +641,6 @@ njs_prop_private_copy(njs_vm_t *vm, njs_
return NJS_ERROR;
}
- shared = pq->lhq.value;
*prop = *shared;
pq->lhq.replace = 0;
@@ -648,10 +653,6 @@ njs_prop_private_copy(njs_vm_t *vm, njs_
return NJS_ERROR;
}
- if (!njs_is_function(&prop->value)) {
- return NJS_OK;
- }
-
function = njs_function_value_copy(vm, &prop->value);
if (njs_slow_path(function == NULL)) {
return NJS_ERROR;
@xeioex
src/njs_object_property.c is not removed, src/njs_object_prop.c is created from scratch.
Please use hg mv to move the file. We need this to preserve history of changes.
Can you be more specific? It seems I did wrong operations.
> mv njs_object_property.c njs_object_prop.c
...
> hg add .
> hg commit -m "..."
@hongzhidao
hg move --help
hg rename [OPTION]... SOURCE... DEST
Mark dest as copies of sources; mark sources for deletion. If dest is a
directory, copies are put in that directory. If dest is a file, there can
only be one source
mv njs_object_property.c njs_object_prop.c
yes, this breaks history.
The correct way:
hg mv src/njs_object_property.c src/njs_object_prop.c
diff would look like
diff --git a/njs/njs.h b/src/njs.h
rename from njs/njs.h
rename to src/njs.h
--- a/njs/njs.h
+++ b/src/njs.h
@hongzhidao
But, Is the below correct?
It may work with the current code (where only shared mutable properties are NJS_METHOD and prop->value is always function), but I think njs_prop_private_copy can be made more generic in the future.
It seems that currently we do not need to make a copy of other types (because they are immutable).
xeioex
on 2 Aug 2019
@xeioex
Updated.
https://gist.github.com/hongzhidao/cac63ce7db2ec361c5195dd33922e3ed
hongzhidao
on 2 Aug 2019
@hongzhidao
Updated.
https://gist.github.com/hongzhidao/cac63ce7db2ec361c5195dd33922e3ed
I still see a copy:) https://gist.github.com/hongzhidao/cac63ce7db2ec361c5195dd33922e3ed#file-njs-prop-patch-L838
xeioex
on 2 Aug 2019
What I did:
978 hg clone http://hg.nginx.org/njs
979 cd njs/
980 hg mv src/njs_object_property.c src/njs_object_prop.c
981 cat src/njs_object_prop.c >> src/njs_value.c
982 vi src/njs_value.c
983 vi src/njs_object_prop.c
984 vi src/njs_object.h
985 vi src/njs_value.h
990 vi auto/sources
998 hg commit -u "hongzhidao <[email protected]>" -m "help added."
And I apply the patch to latest version, it does remove the file njs_object_property.c.
What's wrong with the operation?
hongzhidao
on 2 Aug 2019
@hongzhidao
$ hg qnew move.patch
$ hg mv src/njs_object_property.c src/njs_object_prop.c
$ hg qrefresh # to update the patch set
$ hg qrefresh -e # to edit the commit log
$ hg export -q qtip > move.patch
$ cat move.patch
# HG changeset patch
# User Dmitry Volyntsev <[email protected]>
# Date 1564752212 -10800
# Fri Aug 02 16:23:32 2019 +0300
# Node ID 2dc0ca85886a9f7c8cd1271669d6ec00a70b8949
# Parent 219a6024c4c8369aaaf7acb6c9c50ae1f8162422
[mq]: move.patch
diff --git a/src/njs_object_property.c b/src/njs_object_prop.c
rename from src/njs_object_property.c
rename to src/njs_object_prop.c
$ hg qseries # to see applied patches
move.patch
$ hg qpop # to pop the top-most patch
$ hg qseries
$ hg qpush # to apply the first imported patch
$ hg qimport fix.patch # to add existing patch from a file
$ hg qseries
fix.patch
@hongzhidao
Do you have mq (hg qnew/qpop/qpush/qseries/qimport) extension for hg?
$ cat ~/.hgrc
...
[extensions]
mq =
...
Also, do you use gist CLI tool for uploading to github gists? This is really convenient.
# to upload to github gist, you get the link
$ gist move.patch
https://gist.github.com/34ff1786c847dfbf25e00a418d82b04f
Updated.
xeioex
on 2 Aug 2019
Do you have patchbomb (hg qpop/qpush/qseries/qimport) extension for hg?
[extensions]
mq =
[ui]
username = my username
[extensions]
hgext.patchbomb =
@xeioex
1020 hg qnew move.patch
1021 hg mv src/njs_object_property.c src/njs_object_prop.c
1022 cat src/njs_object_prop.c >> src/njs_value.c
1023 vi src/njs_object_prop.c
1024 vi src/njs_value.c
1025 vi auto/sources
1026 cat src/njs_object.h >> src/njs_value.h
1027 vi src/njs_object.h
1028 vi src/njs_value.h
1047 hg qrefresh
1048 hg qrefresh -e
1050 hg export -q qtip > move.patch
1051 cat move.patch
But it still have a copy???
hongzhidao
on 2 Aug 2019
@hongzhidao
can you show the hg qdiff after the first hg mv?
$ hg qnew move.patch
$ hg mv src/njs_object_property.c src/njs_object_prop.c
$ hg qrefresh
$ hg qdiff
diff -r 219a6024c4c8 src/njs_object_prop.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/njs_object_prop.c Fri Aug 02 22:26:36 2019 +0800
@@ -0,0 +1,1404 @@
+
+/*
+ * Copyright (C) Igor Sysoev
+ * Copyright (C) NGINX, Inc.
+ */
+
+#include <njs_main.h>
+#include <string.h>
+
+
+static njs_int_t njs_object_property_query(njs_vm_t *vm,
+ njs_property_query_t *pq, njs_object_t *object,
+ const njs_value_t *property);
Mercurial Distributed SCM (version 1.4)
Copyright (C) 2005-2009 Matt Mackall <[email protected]> and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
@hongzhidao
1.4 looks too old.
$ hg --version
Mercurial Distributed SCM (version 3.7.3)
(see https://mercurial-scm.org for more information)
Copyright (C) 2005-2016 Matt Mackall and others
...
https://www.mercurial-scm.org/downloads
For ubuntu: https://launchpad.net/~mercurial-ppa/+archive/ubuntu/releases
sudo add-apt-repository ppa:mercurial-ppa/releases
sudo apt-get update
sudo apt-get install mercurial
@xeioex
Mercurial Distributed SCM (version 4.2.3)
(see https://mercurial-scm.org for more information)
Copyright (C) 2005-2017 Matt Mackall and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- show the hg qdiff after the first hg mv
$ hg qnew move.patch
$ hg mv src/njs_object_property.c src/njs_object_prop.c
$ hg qrefresh
$ hg qdiff
diff --git a/src/njs_object_property.c b/src/njs_object_prop.c
rename from src/njs_object_property.c
rename to src/njs_object_prop.c
md5-c34530d8bf9c7e170fbc49f6dbe2cb7f
$ hg qrefresh -e
$ hg export -q qtip > move.patch
md5-d01b19c978de860a98826dab03922900
# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1564758633 -28800
# Fri Aug 02 23:10:33 2019 +0800
# Node ID 388d4bb1acfb1c56894454ccb05169cfec011da1
# Parent 219a6024c4c8369aaaf7acb6c9c50ae1f8162422
help added.
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/src/njs_object_prop.c Fri Aug 02 23:10:33 2019 +0800
@@ -0,0 +1,1404 @@
+
+/*
+ * Copyright (C) Igor Sysoev
+ * Copyright (C) NGINX, Inc.
+ */
+
+#include <njs_main.h>
+#include <string.h>
@hongzhidao
I found the culprit:
$ cat ~/.hgrc
...
[diff]
showfunc = 1
git = 1 // <--
@xeioex
Thanks for you help, here's the final patch.
https://gist.github.com/hongzhidao/cac63ce7db2ec361c5195dd33922e3ed
hongzhidao
on 2 Aug 2019
It seems that currently we do not need to make a copy of other types (because they are immutable).
The purpose of doing this are:
- No function changes. it seems it do nothing if shared->value is not function?
645 ret = njs_lvlhsh_insert(&pq->prototype->hash, &pq->lhq); 646 if (njs_slow_path(ret != NJS_OK)) { 647 njs_internal_error(vm, "lvlhsh insert failed"); 648 return NJS_ERROR; 649 } 650 651 if (!njs_is_function(&prop->value)) { // why not put this in front? 652 return NJS_OK; 653 } - Prepare for regarding the type prop method as NJS_PROPERTY.
hongzhidao
on 2 Aug 2019
@hongzhidao
// why not put this in front?
Agree this can be done.
xeioex
on 2 Aug 2019
@hongzhidao
https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4
SEGFAULT:
https://gist.github.com/b478d49643d62671bef00e10aa2d88f6
> var obj = { __proto__: null, a:null }
shell:main
00000 OBJECT 263C2D0
00016 PROP INIT 263C1F0 263C2D0 263C240
00048 PROP INIT 263C1F0 263C2D0 263C2E0
00080 MOVE 263C1C0 263C2D0
00104 STOP 263C200
Segmentation fault (core dumped)
Relevant docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Prototype_mutation
xeioex
on 8 Aug 2019
@hongzhidao
IMO, it looks better:
>> var o = { get get() { return 'bar'; } }; o.get
shell:anonymous
00000 RETURN 1187200
shell:main
00000 OBJECT 11871E0
00016 FUNCTION 1187220 11896C0
00040 PROP GET ACCESSOR 1187220 11871E0 11871F0
00080 MOVE 11871D0 11871E0
00104 PROP GET 11871E0 11871D0 11871F0
00136 STOP 11871E0
'bar'
@hongzhidao
Some fixes, improvements:
```diff --git a/src/njs_vmcode.c b/src/njs_vmcode.c
--- a/src/njs_vmcode.c
+++ b/src/njs_vmcode.c
@@ -605,7 +605,7 @@ next:
accessor = (njs_vmcode_prop_accessor_t *) pc;
function = njs_vmcode_operand(vm, accessor->value);
- ret = njs_primitive_value_to_string(vm, &name, value2);
ret = njs_value_to_string(vm, &name, value2);
if (njs_slow_path(ret != NJS_OK)) {
njs_internal_error(vm, "failed conversion of type "%s" "
"to string while property define",
diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -10109,6 +10109,12 @@ static njs_unit_test_t njs_test[] =
{ njs_str("var expr = 'foo'; var o = { get expr { return 'bar'; } }; o.foo"),
njs_str("bar") },{ njs_str("var o = { get {toString(){return 'get'}} { return 'bar'; } }; o.get"),
- njs_str("bar") },
+ - { njs_str("var o = { get {toString(){return {} }} { return 'bar'; } }; o.get"),
- njs_str("InternalError: failed conversion of type "object" to string while property define") },
+
{ njs_str("var o = { get foo(v1, v2) { return 'bar'; } }; o.foo"),
```
xeioex
on 8 Aug 2019
var obj = { __proto__: null, a:null }
Yes, I also found it. It's a sepeate issue.
Can you help fix this?
1126 case NJS_OBJECT:
1127 ret = njs_value_to_string(vm, &name, key);
1128 if (njs_slow_path(ret != NJS_OK)) {
1129 return NJS_ERROR;
1130 }
1131
1132 njs_string_get(&name, &lhq.key);
1133 lhq.key_hash = njs_djb_hash(lhq.key.start, lhq.key.length);
1134 lhq.proto = &njs_object_hash_proto;
1135 lhq.pool = vm->mem_pool;
1136
1137 obj = njs_object(value);
1138
/* obj->__proto__ maybe null */
1139 ret = njs_lvlhsh_find(&obj->__proto__->shared_hash, &lhq);
1140 if (ret == NJS_OK) {
1141 prop = lhq.value;
@hongzhidao
Can you help fix this?
On it.
xeioex
on 8 Aug 2019
@xeioex
PROP GET ACCESSOR
Some fixes, improvements:
Updated.
https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4
hongzhidao
on 8 Aug 2019
@hongzhidao
Current version: https://gist.github.com/7090af154462c86d7eda76eb56d1f41c
xeioex
on 8 Aug 2019
Looks good.
BTW, the percentage of passed test262 is welcome to show.
hongzhidao
on 8 Aug 2019
@hongzhidao
https://gist.github.com/366af1e10d31ae1df0e652aa86bd92e0
=== Summary ===
- Ran 27119 tests
- - Passed 8324 tests (30.7%)
- - Failed 18795 tests (69.3%)
+ - Passed 8373 tests (30.9%)
+ - Failed 18746 tests (69.1%)
In fact, many many test cases depends on Array.prototype methods, they expected to work with any values not only arrays (@lexborisov is on it, at least +600 more tests).
the second huge chunk is related to eval() + new Function().
Overall: only ~11k tests ES5.1 (our first target).
xeioex
on 8 Aug 2019
@hongzhidao
Fixed: https://gist.github.com/8be3753d1372e2fdd681047bcbe6619b
The patch is good. I plan to finish review (and commit) on monday.
xeioex
on 8 Aug 2019
Related issues
porunov
·
4Comments
fishioon
·
3Comments
drsm
·
3Comments
porunov
·
3Comments
drsm
·
4Comments
Most helpful comment
@xeioex
@hongzhidao
Found a problem: