Add ES2019 Symbol.prototype.description

[camnwalter]

This is my first contribution to this repository so please let me know if I need to change anything! I mainly looked at how NativeMap and NativeSet had their size properties implemented as mentioned in #963, but I'm not too sure if I did it correctly here.

This resolves #962

[camnwalter]

Okay I see some more issues now with the tests. Will try to get those handled.

edit: I think I figured out how to fix all but the this-non-val-symbol.js test that uses a Proxy

[p-bakker]

Thnx @camnwalter for your contribution!

Please see https://github.com/mozilla/rhino?tab=readme-ov-file#contributing-prs for general guidelines for contributing PRs and https://github.com/mozilla/rhino/blob/master/tests/testsrc/README.md for how to run (& update) our test262 coverage.

With regards to the latter: I expect some more tests to start passing with your fix applied

[p-bakker]

Based on https://github.com/mozilla/rhino/pull/1561/checks#step:5:174 it seems that indeed more tests are already passing in the Symbol.description area

So you'll need to run an update on test262.properties

[camnwalter]

I'm trying to update test262.properties but I keep getting the error

Execution failed for task ':rhino:test'.
> No tests found for given includes: [org.mozilla.javascript.tests.Test262SuiteTest](--tests filter)

Not sure what I'm doing wrong. I'm trying ./gradlew test --tests org.mozilla.javascript.tests.Test262SuiteTest --rerun-tasks -DupdateTest262properties

edit: Never mind, just was in the wrong directory!

[p-bakker]

Mmm, not at my laptop ATM, so cannot look into it, but I think this may have to do with the restructuring of the repo into Java modules recently and therefor the command needs to be adjusted a bit to find stuff in the proper location?

Or have you maybe not initialized & updated the test262 submodule?

[camnwalter]

I got it working, just had to change directory to tests

[gbrail]

I'm a little bit confused by this, actually -- it looks from the spec that the native "Symbol" object, represented in Rhino by the NativeSymbol class, should have a property named "description" that returns the description. However, this PR creates a new SymbolKey and it's using it somehow. You should be able to just follow the pattern of other classes that inherit from IdScriptableObject and add a case to the "findPrototypeId(String)" method that adds this properly -- classes like NativeString do this.

(I do understand that the IdScriptableObject thing is complicated and I'd like it to go away some day but it will take a bunch of work!)

[camnwalter]

Ok. I just didn't really understand what I was doing so I just copied the get size of NativeMap, and it used a SymbolKey. Will change it now.

edit: This also seems to be the same way #1183 does it? Now I am more confused 😭

[camnwalter]

So should I implement it as a data property or just wait for now until there's a good way to make accessor properties?

[p-bakker]

@gbrail there seems to be an issue/challenge in properly implementing accessor properties in IdScriptableObject, see #963 and #1183 (comment) and this PR runs into the same issue

Do you have any insight to offer how to (properly) implement assessor properties in IdScriptableId?
Or should we just go with EcmaScript-incompatible data properties for now and fix these implementations when converting the to Lambda's (assuming here that will make things easier, but I might be wrong about that)?

[p-bakker]

Or another option: convert NativeSymbol to a Lambda-based implementation, similar to what was done in #1384 for NativeJSON?

(I do understand that the IdScriptableObject thing is complicated and I'd like it to go away some day but it will take a bunch of work!)

@gbrail is there more to it than just doing the conversations for each NativeXxxxx class along the lines of what was done for NativeJSON?

[gbrail]

I agree that ScriptableObject is confusing but I thought that adding a simple protoype property was possible. For example, the implementation of "length" in NativeArray is implemented mostly here:

https://github.com/gbrail/rhino/blob/9dacff75fd87b76a07f31d991f9f58911b5e80fb/rhino/src/main/java/org/mozilla/javascript/NativeArray.java#L87

But yes, in general I'm excited about using lambdas more and perhaps it's time to start converting classes from IdScriptatbleOBject!

[p-bakker]

Mmm, don't quite see the length property(Descriptor) implementation at the place the link you provided points to

Haven't checked yet whether the propertyDescriptor of the Array.length property is indeed an accessor descriptor though

[camnwalter]

Suggested change

	desc.put("get", desc, obj.get(GETDESCRIPTION, obj));
	desc.put("get", desc, new LambdaFunction(scope, 0, NativeSymbol::js_getDescription));

I was able to change the code to look like this to not have to deal with the new SymbolKey and without having to deal with converting everything else to using lambda. Is this good?

[p-bakker]

I think that'll do, but would like @gbrail to have a look as well

[p-bakker]

For example, the implementation of "length" in NativeArray is implemented mostly here

@gbrail the NativeArray.length property is not defined as an accessor property, but as a value property (and correctly so)

Symbol.description is, together with a few other properties (see #963) to be implemented as an accessor property, so with a get property defined in the PropertyDescriptor.

To my knowledge the only place where we do this currently is for the size property of Set and Map, which is where the (convoluted) SymbolKey approach came from.

But I think the LambdaFunction approach @camnwalter came up with now is cleaner and gets the job done, agree?

[gbrail]

I decided to start to research this and see if using the new lambda-based classes works. For Symbol in particular, it takes a number of weirdnesses out of the current implementation, so I'm going to upload that PR soon, and you can let me know if it makes sense.

Also, I didn't really understand (or had to refresh my memory) about the nuances of accessor properties and that "descriptor" property here. My PR implements that and looks a bit like some of this code. I hope you don't mind waiting a few days until I have that ready.

[p-bakker]

FYI, just for reference: see here for a brief explanation of data vs. accessor property descriptors

[p-bakker]

Duplicate effort? #1579

[p-bakker]

@gbrail besides #1579 being related/duplicated effort, #1577 also seems potentially related to your lambda-related effort

[gbrail]

thanks for doing this and alerting me to the challenges. That other PR is going in a good direction, so at this point I'd like to thank you for this PR and close it in favor of the other one, and hope that you will find other things to contribute. (Actually you already did!)

[camnwalter]

Yeah no problem! Glad to help