Implement Symbol.prototype.description and refactor Symbol to use LambdaConstructor

[andreabergia]

In draft, because it's stacked on top of both #1577 and #1578 and will need to be rebases.

This PR changes NativeSymbol so that it does not extend IdScriptableObject anymore, but rather uses the LambdaConstructor APIs. This, combined with #1577, allows for easily adding the Symbol.prototype.descriptor slot (which is a getter function).
It makes all relevant test262 cases pass (and a few other unrelated ones). The one that does not pass, prototype/description/this-val-non-symbol.js, does not because it uses Proxy.

[p-bakker]

Are you aware of #1561?

[andreabergia]

Yeah, I saw that PR tried the IdScriptableObject approach and is having some problems - that's why I opened this PR. It was a bug we fixed internally a while back, seemed useful to try and see if this can be helpful.

[gbrail]

Thanks! Let's get the other two PRs worked out and we'll look at this one.

I looked at doing this too, and I'm more happy to have you do it! There are other opportunities for improvement in this code since I had to work around all the limitations of our non-lambda world. For example:

1. We shouldn't need the weird trick of setting a thread local to know whether we should throw an exception any more
2. We shouldn't need to store the global Symbol table in a key on the top-level scope -- we can create it in the init function and pass it via capture to the two lambda functions that need it and via the magic of Java it will be accessible for all Symbol objects created using this prototype, and only those objects, at zero cost.

[gbrail]

Thanks for waiting a bit... @camnwalter added a new method to ScriptableObject and LambdaConstructor that will define the property the way that you want. I know that you did something similar in here. Now that that's merged in master, would you be willing to re-work this PR to use that? Then we're all set. Thanks!

[andreabergia]

Rebased this on top of master. A couple of comments:

1. We shouldn't need the weird trick of setting a thread local to know whether we should throw an exception any more

Yeah, it's horrible, but I'm not sure we can do better. The problem is in NativeSymbol::construct:

    public static NativeSymbol construct(Context cx, Scriptable scope, Object[] args) {
        cx.putThreadLocal(CONSTRUCTOR_SLOT, Boolean.TRUE);
        try {
            return (NativeSymbol) cx.newObject(scope, CLASS_NAME, args);
        } finally {
            cx.removeThreadLocal(CONSTRUCTOR_SLOT);
        }
    }

We wanna use cx.newObject to have prototypes set correctly, but that is going to invoke LambdaConstructor::construct - which should not be allowed in general, because you cannot create symbols via new Symbol().
If you can see a way to remove this, I would be very happy though - it is horrible.

2. We shouldn't need to store the global Symbol table in a key on the top-level scope -- we can create it in the init function and pass it via capture to the two lambda functions that need it and via the magic of Java it will be accessible for all Symbol objects created using this prototype, and only those objects, at zero cost.

I've attempted to implement that, but it seems to break the test262 case Symbol/for/cross-realm.

[p-bakker]

To my knowledge, the Symbol table isn't shared cross realm in Rhino currently, while according to the spec it should (see #1079)

[andreabergia]

To my knowledge, the Symbol table isn't shared cross realm in Rhino currently, while according to the spec it should (see #1079)

Well, this PR makes the test 26 case Symbol/keyFor/cross-realm.js pass, so... I guess I accidentally fixed it!

[p-bakker]

I thought you said you broke the test...🤔

[andreabergia]

3. We shouldn't need to store the global Symbol table in a key on the top-level scope -- we can create it in the init function and pass it via capture to the two lambda functions that need it and via the magic of Java it will be accessible for all Symbol objects created using this prototype, and only those objects, at zero cost.

No, implementing what Greg was suggesting in point 2) above would break the test (that is now passing, with this PR)

[p-bakker]

Any chance to sort out this one? prototype/description/this-val-non-symbol.js

[gbrail]

Looks like good progress and JavaScript has more weird angles than I expected. Are we waiting for another test to be fixed, or do we think that we're in a good place now?

[andreabergia]

Any chance to sit out this one? prototype/description/this-val-non-symbol.js

Does not pass because it uses Proxy.

Looks like good progress and JavaScript has more weird angles than I expected. Are we waiting for another test to be fixed, or do we think that we're in a good place now?

I don't have anything more planned for this PR.

[p-bakker]

We wanna use cx.newObject to have prototypes set correctly, but that is going to invoke LambdaConstructor::construct - which should not be allowed in general, because you cannot create symbols via new Symbol().

Maybe I'm not following the logic, but the the thread local is set before making calls to createStandardSymbol, which calls ctor.call, not ctor.construct.

Unless I'm missing something, the theadlocal doesn't seem to do much

[p-bakker]

Other than that: LGTM

[rbri]

Does not pass because it uses Proxy.

Will check after my vacation if my proxy impl solves this.

[gbrail]

I know that we keep changing the landscape but this work is exposing issues with the lambda stuff that we need to fix for everything.

Can you take a look at this PR:

#1622

It makes it possible to have different behaviors for a constructor when called via "new" and directly.

However, you should not necessarily need this for this PR -- the existing LambdaConstructor already has "flags" and you should be able to use those to implement the functionality that you cannot create a Symbol using "new".

[andreabergia]

I know that we keep changing the landscape but this work is exposing issues with the lambda stuff that we need to fix for everything.

Can you take a look at this PR:

#1622

It makes it possible to have different behaviors for a constructor when called via "new" and directly.

I'm currently on vacation this week and the next, so I'll take a look at it, but not for a while.

My 2 cents: if this PR looks good as it is, I would like to see it merged and do the removal of the thread-local in the (near) future, after #1622 has been merged. It's a small implementation detail anyway.

[rbri]

Is there anything that stops us from merging this? Looks like another real step forward and like @andreabergia suggested we can access the thread-local thing later.

At lease i like to have this merged....