Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Symbol.prototype and other related Symbol and type coersion changes #1611

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

tonygermano
Copy link
Contributor

@tonygermano tonygermano commented Sep 5, 2024

There are quite a few changes, but they are spread out in multiple commits to hopefully make them manageable and many are dependent on the previous commits. If I need to break this into more than one PR, let me know.

fixes #1608
in commit e17cc25

Also improved Symbol handling in these methods. Removed checks, warnings, and return values for non-JS objects. The warnings must have been going unnoticed, because NativeArray::js_includes had been trying to convert UniqueTag.NOT_FOUND to a length for non-array-like 'this' values, so that was corrected as well.
ScriptRuntime.toPrimitive(Object, Class<?>) with a nullable instead of Optional type hint has also been added back to ScriptRuntime as a deprecated method.
Also, updated Symbol.prototype[Symbol.toPrimitive] to standardize the function name to what is in the spec.
@tonygermano
Copy link
Contributor Author

CI is still running, but it looks like I may have some test failures to work through. I only ran test262 locally, and there was only 1 new failure added with 100+ removed from the ignore list. The new failure was for a throw assertion, and it was only passing before because it was throwing the wrong error.

@tonygermano tonygermano marked this pull request as draft September 6, 2024 12:46
@tonygermano
Copy link
Contributor Author

@gbrail @p-bakker @rbri
I need some feedback.

  1. If language version >= ES6, should we assume that all Scriptables also implement SymbolScriptable?
  2. If we are not making that assumption, should ScriptableObject.getProperty(Scriptable, Symbol) return Scriptable.NOT_FOUND rather than throwing an exception when a Scriptable is not also a SymbolScriptable? (ensureSymbolScriptable(obj) throws the exception.)
    /** This is a version of getProperty that works with Symbols. */
    public static Object getProperty(Scriptable obj, Symbol key) {
    Scriptable start = obj;
    Object result;
    do {
    result = ensureSymbolScriptable(obj).get(key, start);
    if (result != Scriptable.NOT_FOUND) break;
    obj = obj.getPrototype();
    } while (obj != null);
    return result;
    }

https://github.com/mozilla/rhino/blob/master/rhino/src/test/java/org/mozilla/javascript/tests/IterableTest.java
This test specifically tests passing a Scriptable which is not a SymbolScriptable to a for..of loop using VERSION_ES6, and I don't know if it should throw an exception because it tried to look up a symbol property on an object which does not support symbols, or if it should just treat it like the symbol property is not defined on that object.

Having ScriptableObject.getProperty(Scriptable, Symbol) return Scriptable.NOT_FOUND for non-SymbolScriptables, also allows it to still go up the prototype chain in case a parent object has the symbol property defined.

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 6, 2024

Don't think Implementations of Scriptable van be assumed to also implement SymbolScriptable

That may hold true for built-ins, but I don't think NativeJavaXxxxx do and I know of at least embedding of Rhino that adds a ton of custom host objects that only implement Scriptable

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 6, 2024

I would say getting a Symbol property on a non-SymbolScriptable should not throw

As for setting a Symbol property on a non-SymbolScriptable: in non-strict mode I think just ignoring the put wood be fine, but in strict mode? Not sure, cannot think of anything similar to that in EcmaScript ottomh

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 6, 2024

Seems EcmaScript leaves room for non-standard host objects to be implemented as the implementation sees fit

But most common is the behavior I wrote above + throwing a TypeError in strict mode

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 6, 2024

Note that there also this PR that being worked on actively and which touches the Symbol implementation: #1579

@tonygermano
Copy link
Contributor Author

Thanks for pointing out the other PR. I skimmed it, and I don't think it will conflict with anything that I'm doing in this PR.

@tonygermano
Copy link
Contributor Author

I would say getting a Symbol property on a non-SymbolScriptable should not throw

As for setting a Symbol property on a non-SymbolScriptable: in non-strict mode I think just ignoring the put wood be fine, but in strict mode? Not sure, cannot think of anything similar to that in EcmaScript ottomh

I agree with everything you said. Regarding setting a symbol property on a non-SymbolScriptable, that would match the behavior of setting a property on a sealed or frozen (non-extensible) object running in non-strict vs strict mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue comparing bigint to string
2 participants