Njs: property issues

[hongzhidao]

@xeioex @drsm

There are some issues happened in defineProperty. (see tests)

Here are the patches fix them.
https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4

My thoughts.
Fix issues first, then continue refactoring.

[hongzhidao]

@xeioex

I'll do the refactor after the above issues, most of them are done.
One place I'm not sure. I want to rename all the object actually belong to value as value, property as name.

grep -r 'njs_value_t \*property' src/
src/njs_object_prop.c:    njs_value_t *value, njs_value_t *property)
src/njs_vmcode.c:    njs_value_t *object, njs_value_t *property, njs_value_t *retval);
...
src/njs_vmcode.c:njs_vmcode_property_in(njs_vm_t *vm, njs_value_t *object, njs_value_t *property)
src/njs_vmcode.c:    njs_value_t *property)
src/njs_value.h:    njs_value_t *object, njs_value_t *property);
src/njs_value.h:    njs_value_t *property, njs_value_t *retval);
src/njs_value.h:    njs_value_t *property, njs_value_t *value);
src/njs_object.h:    njs_value_t *value, njs_value_t *property);
src/njs_value.c:    const njs_value_t *property);
src/njs_value.c:    njs_value_t *property)
src/njs_value.c:    njs_object_t *object, const njs_value_t *property)
src/njs_value.c:njs_value_property(njs_vm_t *vm, njs_value_t *value, njs_value_t *property,
src/njs_value.c:    njs_value_t *property, njs_value_t *value)

What do you think?

BTW, I suggest cleaning the header files included in njs_clang.h and njs_value.h.
If OK, you help do this together with the above patches.

[drsm]

@hongzhidao

One place I'm not sure. I want to rename all the object actually belong to value as value, property as name.

Maybe better change property to key, as there also Symbol properties (not implemented yet, https://2ality.com/2014/12/es6-symbols.html#symbols-as-property-keys) .

[hongzhidao]

Maybe better change property to key

Like this.
https://gist.github.com/hongzhidao/c6e43e1e83c3e4c83c88cac8f5c9a249

[xeioex]

@hongzhidao

Agree, 'key' looks better.

[xeioex]

@hongzhidao

https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4#file-njs-propertys-patch-L406

Handling getters in njs_object_property() looks promising, but what about NJS_PROPERTY_HANDLER?

Why not directly use njs_value_property() in such places?

[hongzhidao]

Why not directly use njs_value_property() in such places?

We do the fixes first, the codes append to njs_object_property() is from njs_value_property().
Later, there are changes with these functions.

My main ideas are:

1. There are two ways of accessing value property. Such as:
   delete a['b']; // only query
   a'b' // get property value.
   So njs_proerty_query() will still be public. And njs_value_property() is kept unchanged.
   But getting property by string name is a helpful method. njs_object_property may be renamed as
   njs_value_str_property().
   You'll see there are some other APIs needed. For example.
   njs_object_prop_get() is used in njs_object_prop_descriptor().
   njs_object_prop_value() is used in njs_value_property() and njs_object_property().
   All there will be shown in the following patches committed tomorrow.

Handling getters in njs_object_property() looks promising, but what about NJS_PROPERTY_HANDLER?

I think NJS_PROPERTY_HANDLER does not need to be handled.
See https://github.com/nginx/njs/blob/master/src/njs_object_prop.c#L292

[xeioex]

@hongzhidao

I want to rename all the object actually belong to value as value, property as name.

Looks useful. (property as key)

[hongzhidao]

@xeioex

I suggest cleaning the header files included in njs_clang.h and njs_value.h.

Are you on it?

I want to rename all the object actually belong to value as value, property as name.

I'm on this.

[xeioex]

@hongzhidao

Are you on it?

will do it tomorrow.

[hongzhidao]

@xeioex

Updated.
https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4

help added.

1. cleaning the header files included in njs_clang.h and njs_value.h.
2. simpilify prop->value.data.u.prop_handler(vm, value, setv...).
3. speedup on eliminating most of njs_djs_hash() calls https://gist.github.com/xeioex/664c6149f28af352d6fb33aed7326d48

[hongzhidao]

@xeioex

njs.dump() and JSON.stringify issues.

>> var o = {}; Object.defineProperty(o, 'a', { value: 'bar' })
{
}
>> >> njs.dump(o)
'{}'
>> JSON.stringify(o)
'{}'

I can't find the definite places, help fix them.

[drsm]

@hongzhidao

njs.dump() and JSON.stringify issues.

>> var o = {}; Object.defineProperty(o, 'a', { value: 'bar' })
{
}
>> >> njs.dump(o)
'{}'
>> JSON.stringify(o)
'{}'

It's OK, as we're dumping only enumerable properties.

>> var o = Object.defineProperty({}, 'a', { value: 'bar', configurable: true })
undefined
>> njs.dump(o)
'{}'
>> var o = Object.defineProperty(o, 'a', { enumerable: true })
undefined
>> njs.dump(o)
'{a:'bar'}'

the problem:

>> var o = Object.defineProperty({}, 'a', { get: () => 1, enumerable: true })
undefined
>> JSON.stringify(o)
'{}'
>> njs.dump(o)
'{a:<empty>}'

stringify sholud invoke an accessor, dump should show what accessors are there:

> var o = Object.defineProperty({}, 'a', { get: () => 1, enumerable: true })
undefined
> JSON.stringify(o)
'{"a":1}'
> o
{ a: [Getter] }

[hongzhidao]

@drsm welcome to test.

https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4
Fixed Error().
Fixed njs_vmcode_method_frame().
Fixed njs_object_property().
Added getter/setter literal support.

what's left: fix dump() issue.

[drsm]

@hongzhidao
please update:

diff -r 68c97f3a6a2a src/njs_object_prop.c
--- a/src/njs_object_prop.c Mon Aug 05 01:32:54 2019 -0400
+++ b/src/njs_object_prop.c Mon Aug 05 08:48:27 2019 +0300
@@ -329,7 +329,6 @@
     njs_int_t           ret;
     njs_bool_t          data, accessor;
     njs_value_t         value;
-    njs_lvlhsh_query_t  lhq;

     data = 0;
     accessor = 0;
@@ -378,9 +377,6 @@
         return ret;
     }

-    lhq.key = njs_str_value("value");
-    lhq.key_hash = NJS_VALUE_HASH;
-
     ret = njs_value_str_property(vm, desc, &njs_str_value("value"),
                                  NJS_VALUE_HASH, &value);

@@ -394,9 +390,6 @@
         return ret;
     }

-    lhq.key = njs_str_value("writable");
-    lhq.key_hash = NJS_WRITABABLE_HASH;
-
     ret = njs_value_str_property(vm, desc, &njs_str_value("writable"), 
                                  NJS_WRITABABLE_HASH, &value);

@@ -410,9 +403,6 @@
         return ret;
     }

-    lhq.key = njs_str_value("enumerable");
-    lhq.key_hash = NJS_ENUMERABLE_HASH;
-
     ret = njs_value_str_property(vm, desc, &njs_str_value("enumerable"),
                                  NJS_ENUMERABLE_HASH, &value);

@@ -425,9 +415,6 @@
         return ret;
     }

-    lhq.key = njs_str_value("configurable");
-    lhq.key_hash = NJS_CONFIGURABLE_HASH;
-
     ret = njs_value_str_property(vm, desc, &njs_str_value("configurable"),
                                  NJS_CONFIGURABLE_HASH, &value);

diff -r 68c97f3a6a2a src/njs_parser_terminal.c
--- a/src/njs_parser_terminal.c Mon Aug 05 01:32:54 2019 -0400
+++ b/src/njs_parser_terminal.c Mon Aug 05 08:48:27 2019 +0300
@@ -544,7 +544,7 @@
 static njs_token_t
 njs_parser_object_token(njs_vm_t *vm, njs_parser_t *parser)
 {
-    size_t       offset;
+    size_t       offset = 0;
     njs_str_t    *name;
     njs_token_t  token;

@@ -593,7 +593,7 @@
 njs_parser_object_property(njs_vm_t *vm, njs_parser_t *parser,
     njs_lexer_t *lexer, njs_parser_node_t *obj, njs_token_t token)
 {
-    uint32_t               hash, token_line;
+    uint32_t               hash = 0, token_line = 0;
     njs_int_t              ret;
     njs_str_t              name;
     njs_token_t            prop_token;
diff -r 68c97f3a6a2a src/njs_value.c
--- a/src/njs_value.c   Mon Aug 05 01:32:54 2019 -0400
+++ b/src/njs_value.c   Mon Aug 05 08:48:27 2019 +0300
@@ -122,7 +122,6 @@
     njs_int_t           ret;
     njs_uint_t          tries;
     njs_value_t         fun, retval;
-    njs_lvlhsh_query_t  lhq;

     static const uint32_t  hashes[] = {
         NJS_VALUE_OF_HASH,
@@ -149,9 +148,6 @@
         if (njs_is_object(value) && tries < 2) {
             hint ^= tries++;

-            lhq.key_hash = hashes[hint];
-            lhq.key = names[hint];
-
             ret = njs_value_str_property(vm, value, (njs_str_t *) &names[hint],
                                          hashes[hint], &fun);

[drsm]

@hongzhidao

this still fails:

//node
> var get = 'get', o = { get }
undefined
> o
{ get: 'get' }
> var set = 'set', o = { set }
undefined
> o
{ set: 'set' }

https://github.com/nginx/njs/issues/118#issuecomment-516453211il

@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

[xeioex]

@hongzhidao

https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4
Fixed njs_object_property().
Refactored out njs_value_str_property().

I think these two patches should be combined into one patch (after "Style" patch).

IMO, it a bad idea to change a function which will be removed later (in the next patch).

I am on merging 'Style' patch.

[xeioex]

@hongzhidao

njs_int_t njs_value_property(njs_vm_t *vm, njs_value_t *value, njs_value_t *key, njs_value_t *retval);
njs_int_t njs_value_str_property(njs_vm_t *vm, const njs_value_t *value, njs_str_t *key, uint32_t key_hash, njs_value_t *retval);

I think njs_value_property() should convert key if necessary and call njs_value_str_property().
Otherwise the duplicated each other.

[hongzhidao]

@xeioex

I think njs_value_property() should convert key if necessary.

It does now.

I think njs_value_property() should convert key if necessary and call njs_value_str_property().
Otherwise the duplicated each other.

Notice that njs_object_proerty() (now is njs_value_str_property()) is only called under the condition njs_is_object().. So it seems the naming njs_value_str_property() is not ideal.
What about still renaming it njs_object_property()? But move it to njs_value.c.

BTW, do you think it's necessary to keep the comment? Since NJS_TRAP and NJS_APPLIED are already removed.

/*
 * ES5.1, 8.12.3: [[Get]].
 *   NJS_OK               property has been found in object,
 *      retval will contain the property's value
 *
 *   NJS_DECLINED         property was not found in object
 *   NJS_ERROR            exception has been thrown.
 *      retval will contain undefined
 */

[xeioex]

@hongzhidao

Notice that njs_object_property() (now is njs_value_str_property()) is only called under the condition njs_is_object().

I would like to have njs_value_str_property() as optimized version of njs_value_property() when key is known in advance (many places).

The main concern for me with current njs_value_str_property() is that it does not handle NJS_PROPERTY_HANDLER. While I see that currently NJS_PROPERTY_HANDLER is used only for "length", "__proto__", "prototype" and "constructor" and currently it is not possible to get NJS_PROPERTY_HANDLER in njs_value_str_property() call. But we may need this in the future.

[xeioex]

@hongzhidao

I suggest to eliminate NJS_METHOD first.

[hongzhidao]

@hongzhidao

I suggest to eliminate NJS_METHOD first.

I thought about it. It seems we can distinguish method and property by is_function after eliminate NJS_METHOD. But all the benefits of eliminating NJS_METHOD are limited. So I kept it unchanged.

I suggest we do it later.

The most important thing is to fix current issues, such as njs_object_property(), dump() and stringify().

[xeioex]

@hongzhidao

The most important thing is to fix current issues, such as njs_object_property(), dump() and stringify().

Agree. But I think njs_value_str_property() vs njs_value_property() can be improved.

I am on it.

[hongzhidao]

OK, continue tomorrow.

[hongzhidao]

@drsm

What specification of JS language would you like to suggest?
Now, I usually refer to this. https://tc39.es/ecma262/

[drsm]

@hongzhidao

I'm also suggest to refer to the latest draft and proposals like BigInt

[xeioex]

@hongzhidao

At this point I do not see the crystal clear path on how to eliminate njs_object_property() in good way.

I still want to merge it with njs_value_property(). I am on simplifying njs_property_query(). API at this moment is too difficult.
What can be improved:
1) pq.shared can be removed. https://gist.github.com/a4ac11e8bb45db6027349491373f2216 (in progress).
2) NJS_METHOD type can be removed. (https://gist.github.com/xeioex/a7c82322e5b04a53d03878bae382fd2c).
..

[hongzhidao]

OK, tell me if there is anyting I can help.

[xeioex]

@hongzhidao

You can review these two patches.

[xeioex]

@hongzhidao

BTW, I think NJS_PROPERTY_HANDLER can also be eliminated.
converted to NJS_PROPERTY with function value (similar to NJS_METHOD).

This would allow us to to have your version of njs_object_property() which would handle all cases and be generic.

[hongzhidao]

It'll be very good if they are all removed. Wait for your patch :)
I'm on reivew now.

[hongzhidao]

I think NJS_PROPERTY_HANDLER can also be eliminated.

Can you also consider one place?

typedef struct {
    njs_lvlhsh_query_t          lhq;

    /* scratch is used to get the value of an NJS_PROPERTY_HANDLER property. */
    njs_object_prop_t           scratch;   // can we remove it? Just an idea.
...

If yes, these declarations in njs_value.h can be moved into njs_object.h. Such as.

njs_object_attribute_t
njs_object_prop_t

[xeioex]

@hongzhidao

Updated.
https://gist.github.com/0268b795b772a3d073029ea5372b0e1b

[hongzhidao]

@xeioex

pq.shared can be removed. https://gist.github.com/a4ac11e8bb45db6027349491373f2216

Looks good. But put this line in front.
(error happened if do this)https://github.com/nginx/njs/blob/master/src/njs_object_prop.c#L648

[xeioex]

@hongzhidao

NJS_PROPERTY_HANDLER is also copied by this function.

prop->value is invalid for it. But we need to copy it too, otherwise "delete" operation would SEGFAULT on writing to it (read only memory).

[hongzhidao]

@xeioex

The key point is that NJS_PROPERTY_HANDLER, NJS_METHOD and NJS_PROPERTY are copied in shared mode (found in shared_hash) and copy function if is function. Right?

[xeioex]

@hongzhidao

The key point is that NJS_PROPERTY_HANDLER, NJS_METHOD and NJS_PROPERTY are copied in shared mode (found in shared_hash) and copy function if is function. Right?

yes.

It slows down the first access a bit, but speeds up next queries (because prop now in first hash).

[hongzhidao]

I think NJS_PROPERTY_HANDLER can also be eliminated.

Are you on it?

It seems it'll make a big change. Like regard njs_prop_handler_t as njs_function_native_t.

[xeioex]

@hongzhidao

It seems it'll make a big change.

yes. I am on rethinking what NJS_PROPERTY_HANDLER is actually for. Maybe we should split it in parts.

It is not the same as ordinary getter/setter.

on demand properties:
1) njs_object_prototype_create
2) njs_object_prototype_create_constructor
3) njs_process_object_argv

like ordinary getter/setter:
1) njs_array_length (but according to spec it is NOT)
http://www.ecma-international.org/ecma-262/9.0/index.html#sec-properties-of-array-instances-length
http://www.ecma-international.org/ecma-262/9.0/index.html#sec-array-exotic-objects

Array in fact has redefined [[DefineOwnProperty]]:
    Assert: IsPropertyKey(P) is true.
    If P is "length", then
   Return ? ArraySetLength(A, Desc).

2) njs_function_length

[xeioex]

@hongzhidao

My ideas:
1) rename NJS_PROPERTY_HANDLER to NJS_PROPERTY_ON_DEMAND
For properties like "prototype", "constructor" they should be called once at the first access.
NJS_PROPERTY_ON_DEMAND will be called inside njs_property_query() and hidden from top level functions. No setters. So NJS_PROPERTY_ON_DEMAND is basically an optimization, we create an NJS_PROPERTY at the first access.

2) for njs_array_length we need a special handling.
3) njs_function_length will be created in njs_prop_private_copy() (the same way as "name" now).
4) NJS_PROPERTY_HANDLER is also created on demand for "external" values (for SET and DELETE operations).

[hongzhidao]

@xeioex

See this first. https://raw.githubusercontent.com/ldarren/QuickJS/mod/quickjs.c (line 33270)
I'm also not sure it's OK to regard NJS_PROPERTY_HANDLER as getter.
But it can make things easier. NJS_PROPERTY_HANDLER can be easily removed, and the codes are concise. It's worth to rethink what the adverse effect is if regard them as getter.

What's your suggestion? @drsm
We want to make them as getter, like [].length, __proto__, etc.

[xeioex]

@hongzhidao

I'm also not sure it's OK to regard NJS_PROPERTY_HANDLER as getter.

Definitely not. Object.getOwnPropertyDescriptor(Object, 'prototype') should return data descriptor.
See NJS_PROPERTY_ON_DEMAND in https://github.com/nginx/njs/issues/203#issuecomment-518753753.

[xeioex]

@hongzhidao

I think the first step will be elimination of NJS_PROPERTY_HANDLER in NJS_PROPERTY_QUERY_GET query mode. Complete removal is harder. But, for njs_object_property() it is enough.

[hongzhidao]

I think the first step will be elimination of NJS_PROPERTY_HANDLER in NJS_PROPERTY_QUERY_GET query mode. Complete removal is harder. But, for njs_object_property() it is enough.

OK.

BTW, it's OK to keep NJS_PROPERTY_HANDLER, it's clear in NJS.

[xeioex]

@hongzhidao

BTW, it's OK to keep NJS_PROPERTY_HANDLER, it's clear in NJS.

Agree. I still want to improve NJS_PROPERTY_HANDLER but I think this can be done in the next refactoring (good refactoring requires many changes).

Fixed njs_object_property().
Added getter/setter literal support.

You may want to update the patch. I think njs_object_property() name is OK for now (no need to rename it as njs_value_str_property()).
I plan to review it tomorrow.

[hongzhidao]

@xeioex

Fix njs_object_property() first, then consider continuning refactor or updating getter/setter patch.
https://gist.github.com/hongzhidao/4f0dbc8640804a47727444ff4744a34e

I think njs_object_property() name is OK for now

njs_object_property() is a handy function, so keep it as consistent as possible like njs_value_property() by adding retval argument. Currently, it's OK to put it still in njs_object_prop.c.

[xeioex]

@hongzhidao

https://gist.github.com/hongzhidao/4f0dbc8640804a47727444ff4744a34e#file-njs-0807-patch-L478

please, do not remove these 'case NJS_ERROR:'
1) it adds clarity
2) it is a separate change.

Otherwise looks good, will commit with some improvements and fixes:
1) njs_object_property() should set undefined value to retval in case of NJS_DECLINED
2) introduced njs_object_property_init() to set lhq->proto only once.
3) avoid using switch statement when only 3 possible return values (NJS_ERROR usually is treated separately).
4) more tests

https://gist.github.com/fc3ae94b77d4a210bf35c686379f351d

[hongzhidao]

@xeioex

1. All looks good, but forgot to use njs_object_property_init()?
   https://gist.github.com/xeioex/fc3ae94b77d4a210bf35c686379f351d#file-hd-object-prop-patch-L340

grep -r njs_object_property src/ -l
src/njs_object_prop.c
src/njs_date.c
src/njs_array.c
src/njs_object.h
src/njs_error.c
src/njs_json.c
src/njs_value.c

1. > do not remove these 'case NJS_ERROR:'

Make this as a separate patch?

1. Need I to update it?

[xeioex]

@hongzhidao

Forgot to use njs_object_property_init()?

No, because it is only init (sets lhq->proto).

Make this as a separate patch?

no, case NJS_ERROR: here is for a reason. Later I think many switch cases can be eliminated, because only 3 return codes are left.

[hongzhidao]

No problem. Then what do you plan to do after this?
https://gist.github.com/fc3ae94b77d4a210bf35c686379f351d

[xeioex]

1) I want to finish #40 first before #118. (#40 fixes 3 more V8 benchmarks.)
2) getter/setter literals.

[hongzhidao]

There are still bugs with njs.dump() and JSON.stringify()
https://github.com/nginx/njs/issues/203#issuecomment-518084627
It needs your help :).
Getter/setter patch depends on it.

But I also can commit getter/setter patch first, what's your suggestion?

[xeioex]

@hongzhidao

But I also can commit getter/setter patch first, what's your suggestion?

Yes, it looks like a separate issue, will do.

[xeioex]

@hongzhidao

Added support for accessor property descriptors in njs.dump().
https://gist.github.com/c890e75bac330e64c22bd56952d2736e

Please review.

[hongzhidao]

https://gist.github.com/c890e75bac330e64c22bd56952d2736e

Looks good.

1. One tiny detail: string_getter_setter vs string_getset?
2. *retval = prop->getter;
   https://gist.github.com/xeioex/c890e75bac330e64c22bd56952d2736e#file-hjs-dump-getter-patch-L261

BTW, I think this ticket can be closed.

[xeioex]

@hongzhidao @drsm

https://gist.github.com/660eaedbd6b0b85ac5a14b4c71da6476

Added support for accessor property descriptors in njs.dump().
Added support for accessor property descriptors in JSON.stringify().

[hongzhidao]

review it tomorrow, and I’ll update getter/setter patch.

[hongzhidao]

https://gist.github.com/660eaedbd6b0b85ac5a14b4c71da6476

Looks good.

[hongzhidao]

@drsm @xeioex

Added getter/setter literal support.
https://gist.github.com/hongzhidao/1c4bf3f02ffbdd605e6252a7f48373b4

Welcome to test.