add Symbol primitive type

[drsm]

the spec
description

[drsm]

Here is the draft of the patch that adds only a primitive data type:
https://gist.github.com/drsm/1cf394eaf4cc2ff8f4ef50025bb687cd

[xeioex]

@drsm Good start!

I planned to implement this feature after #16.

BTW, we also need built-in tags (like to @@toStringTag).

[xeioex]

@drsm

BTW, cannot we reduce the number of tests by combining?

Something like

["0**", "1/"].every((v)=> {try {new Function(v + "Symbol()")() } catch (e) {return e.message === "Cannot convert a Symbol value to a number"}})

[drsm]

@xeioex

BTW, cannot we reduce the number of tests by combining?

Done.

[xeioex]

@drsm

Can you share current test262 update?

[drsm]

@xeioex

https://pastebin.com/cCaFxapK

 === Summary ===
  - Ran 31860 tests
- - Passed 10031 tests (31.5%)
- - Failed 21829 tests (68.5%)
+ - Passed 10210 tests (32.0%)
+ - Failed 21650 tests (68.0%)

[xeioex]

@drsm

Is it ready for review? What issues are left?

[drsm]

@xeioex

Is it ready for review?

Yes.

What issues are left?

Support for object property keys and Symbol.for()/.keyFor() are not a subject of the patch.
So, only wrong error message in Symbol() + 'abc'

[drsm]

Also, I forget about

> JSON.stringify(Symbol())
undefined
> JSON.stringify({ a: Symbol(), b: 1 })
'{"b":1}'
> 

But I think it's better to fix this after property keys.

[xeioex]

@drsm I see a lot of

TypeError: Cannot convert a Symbol value to a string

is it hard to implement?

[drsm]

@xeioex

is it hard to implement?

No. Just involves some weird logic, but on slow path.

Symbol() + 'abc' is fixed now.

[drsm]

Is it ready for review?

Yes.

Some files were forgotten after rebase.
Sorry about that, fixed now.

[xeioex]

@drsm

Support for object property keys .. are not a subject of the patch.

Do you mean Something like var o = {}; o[Symbol.isConcatSpreadable] = true right? Do you plan to add this later?

[xeioex]

@drsm

Great job! Will commit with small improvements over: https://gist.github.com/dd31240f02ed6c8edb1707c8f11aaf08

[drsm]

@xeioex

Thanks for reviewing this!

Do you mean Something like var o = {}; o[Symbol.isConcatSpreadable] = true right?

Yes. And also support for Object.defineProperty(), Object.getOwnPropertyDescriptor() and other related functions.

Do you plan to add this later?

Yes. I wil try.

[xeioex]

@drsm

I plan to add support for object property keys and "@@toStringTag" for builtin objects.

[drsm]

@xeioex

My plan was to abuse lhq->key value as

lhq->key = njs_str_t(njs_symbol_key(), NULL);
lhq->key_hash = njs_symbol_hash();
// https://stackoverflow.com/a/12996028
uint32_t njs_symbol_hash(uint32_t x) {
    x = ((x >> 16) ^ x) * 0x45d9f3b;
    x = ((x >> 16) ^ x) * 0x45d9f3b;
    x = (x >> 16) ^ x;
    return x;
}

I think we need something generic or union here, will be also useful for Map and Set implementation:

> var m = new Set([null, 1, Symbol(), 'asdf', {}, void 0]);
undefined
> [...m.keys()].map((x) => typeof x)
[ 'object', 'number', 'symbol', 'string', 'object', 'undefined' ]

[drsm]

Looks like there is no need for lhq->key at all in case of symbol lookup.
If we change key generation scheme to:

uint32_t njs_hash(uint32_t x) {
    x = ((x >> 16) ^ x) * 0x45d9f3b;
    x = ((x >> 16) ^ x) * 0x45d9f3b;
    x = (x >> 16) ^ x;
    return x;
}

uint32_t njs_unhash(uint32_t x) {
    x = ((x >> 16) ^ x) * 0x119de1f3;
    x = ((x >> 16) ^ x) * 0x119de1f3;
    x = (x >> 16) ^ x;
    return x;
}

// in constructor:
key = njs_hash(++vm->symbol_generator);

// toString, description (they are used mostly for debugging):
string_index = njs_unhash(key)

// precompute njs_hash for well-known symbols

[xeioex]

@drsm

Looks like there is no need for lhq->key at all in case of symbol lookup.

yes. since symbols Id are uniq we can simple do

njs_object_hash_test()

prop = data;
name = &prop->name;

if (njs_slow_path(njs_is_symbol(name))) {                                                                                  
          return (njs_symbol_key(name) == lhq->key_hash) ? NJS_OK                                                                
                                                         : NJS_DECLINED;                                                         
      }

/* string. */  
...

Even more, we can avoid memcmp() even for strings altogether if we can make them uniq and compare just pointers or some ids similar to symbol ids. (I plan to do this in the middle-term future)
See https://docs.python.org/3/library/sys.html#sys.intern .

[xeioex]

@drsm Compare also: https://bellard.org/quickjs/quickjs.html#Atoms

[drsm]

@xeioex

-return (njs_symbol_key(name) == lhq->key_hash) ? NJS_OK                                                                
+return (njs_symbol_key(name) == lhq->key_hash) && !lhq->key.length ? NJS_OK                                                                

[xeioex]

@drsm

Take a look: https://gist.github.com/fcb8b28ac5fc6163db86fed7a9029fa5

Yes. And also support for Object.defineProperty(), Object.getOwnPropertyDescriptor() and other related functions.

done. + also Object.getOwnPropertyNames(), Object.getOwnPropertySymbols(), Object.keys().

[drsm]

One moment about this place.
Can't we get a hash collision here while looking up a string property with same hash as one of the object's symbols?

[drsm]

@xeioex

Sorry for that:

-return (njs_symbol_key(name) == lhq->key_hash) ? NJS_OK                                                                
+return (njs_symbol_key(name) == lhq->key_hash) && !lhq->key.length ? NJS_OK                                                                

two tries were not enough... )

$ build/njs
interactive njs 0.3.8

v.<Tab> -> the properties and prototype methods of v.

>> var o = {}, n = 5381 /* NJS_DJB_HASH_INIT */;
undefined
>> while(n--) o[Symbol()] = 'test';
undefined
>> o['']
'test'

# HG changeset patch
# User Artem S. Povalyukhin <artem.povaluhin@gmail.com>
# Date 1574364674 -10800
#      Thu Nov 21 22:31:14 2019 +0300
# Node ID 48821a4feda31a5189c0490ef30c7183a91ef9fd
# Parent  10a19a2e1d4feca045ca98a271db6c71bfaeb9cc
Fixed hash collision for empty string key.

diff -r 10a19a2e1d4f -r 48821a4feda3 src/njs_object.c
--- a/src/njs_object.c	Thu Nov 21 20:56:06 2019 +0300
+++ b/src/njs_object.c	Thu Nov 21 22:31:14 2019 +0300
@@ -171,7 +171,7 @@ njs_object_hash_test(njs_lvlhsh_query_t 
 
     if (njs_slow_path(njs_is_symbol(name))) {
         return ((njs_symbol_key(name) == lhq->key_hash)
-                && lhq->key.length == 0) ? NJS_OK : NJS_DECLINED;
+                && lhq->key.start == NULL) ? NJS_OK : NJS_DECLINED;
     }
 
     /* string. */
diff -r 10a19a2e1d4f -r 48821a4feda3 src/njs_object.h
--- a/src/njs_object.h	Thu Nov 21 20:56:06 2019 +0300
+++ b/src/njs_object.h	Thu Nov 21 22:31:14 2019 +0300
@@ -110,6 +110,7 @@ njs_object_property_key_set(njs_lvlhsh_q
     if (njs_is_symbol(key)) {
 
         lhq->key.length = 0;
+        lhq->key.start = NULL;
         lhq->key_hash = njs_symbol_key(key);
 
     } else {
diff -r 10a19a2e1d4f -r 48821a4feda3 src/njs_value.h
--- a/src/njs_value.h	Thu Nov 21 20:56:06 2019 +0300
+++ b/src/njs_value.h	Thu Nov 21 22:31:14 2019 +0300
@@ -871,6 +871,7 @@ njs_set_object_value(njs_value_t *value,
 #define njs_property_query_init(pq, _query, _own)                             \
     do {                                                                      \
         (pq)->lhq.key.length = 0;                                             \
+        (pq)->lhq.key.start = NULL;                                           \
         (pq)->lhq.value = NULL;                                               \
         (pq)->own_whiteout = NULL;                                            \
         (pq)->query = _query;                                                 \
diff -r 10a19a2e1d4f -r 48821a4feda3 src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c	Thu Nov 21 20:56:06 2019 +0300
+++ b/src/test/njs_unit_test.c	Thu Nov 21 22:31:14 2019 +0300
@@ -10302,6 +10302,10 @@ static njs_unit_test_t  njs_test[] =
               "Object.getOwnPropertyDescriptor(o, Symbol.isConcatSpreadable).value"),
       njs_str("true") },
 
+    { njs_str("var o = {}, n = 5381 /* NJS_DJB_HASH_INIT */;"
+              "while(n--) o[Symbol()] = 'test'; o[''];"),
+      njs_str("undefined") },
+
     /* String */
 
     { njs_str("String()"),