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.

@p-bakker
Copy link
Collaborator

@tonygermano any progress on this?

Noticed you started multiple PRs in a short timeframe, not sure how interdependent they are, but would be nice if (parts of) at least this PR could move forward: the bit about toString taking into account Symbol.toPrimitive() unbreaks quite a few test 🙂

if (val1 instanceof CharSequence && val2 instanceof CharSequence) {
return compareTo(val1.toString(), val2.toString(), op);
}
if (val1 instanceof BigInteger && val2 instanceof CharSequence) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can have one 'val1 instanceof CharSequence' base if with another if inside....

@rbri
Copy link
Collaborator

rbri commented Sep 25, 2024

@tonygermano i fear this will be another endless story to get this merged. Therefore i lik to suggest to split this up. First step might be a pr that has only the first two commits from this story.
What do you think?

If you like i can prepare such a pr.

rbri added a commit to HtmlUnit/rhino-development that referenced this pull request Sep 29, 2024
9664e77, e17cc25)

* more standard compliant implementation of ScriptRuntime.toPrimitive();
  * have not used the Optional for more backward compatibility
  * have changed the code a bit to be more in sync with the spec (explicit if stmt's)
* improved support for Symbol/BigInt in equals and compare

I did this to make it possible to merge this improvement soon - at least this is my hope. Will extract more smaller commits if this is merged.
rbri added a commit to HtmlUnit/rhino-development that referenced this pull request Oct 1, 2024
9664e77, e17cc25)

* more standard compliant implementation of ScriptRuntime.toPrimitive();
  * have not used the Optional for more backward compatibility
  * have changed the code a bit to be more in sync with the spec (explicit if stmt's)
* improved support for Symbol/BigInt in equals and compare

I did this to make it possible to merge this improvement soon - at least this is my hope. Will extract more smaller commits if this is merged.
gbrail pushed a commit that referenced this pull request Oct 2, 2024
…ermano) (#1661)

* this is an excerpt of from PR #1611 done by @tonygermano (commit 9664e77, e17cc25)

* more standard compliant implementation of ScriptRuntime.toPrimitive();
  * have not used the Optional for more backward compatibility
  * have changed the code a bit to be more in sync with the spec (explicit if stmt's)
* improved support for Symbol/BigInt in equals and compare
* adjusted based on retests with real browsers
@rbri
Copy link
Collaborator

rbri commented Oct 2, 2024

short info - working on carving out the nect part

@p-bakker
Copy link
Collaborator

p-bakker commented Oct 5, 2024

@rbri have you cherry-picked everything from this PR into separate PRs now, so this one can be closed? Or is there still something left in this one that warrants leaving it open?

@rbri
Copy link
Collaborator

rbri commented Oct 5, 2024

@p-bakker i do it one step after another - currently step 2 is waiting for review/merge (pr #1674). I plan at least two more steps (each requires the one one before i fear).

But finally i might not take all from this - i do not like to pick the 'Add asyncIterator and matchAll well-known Symbols' commit because i guess the related functions are not yet implemented and having only the symbols available makes not so much sense. But i have to have a closer look at this after step 4.

@p-bakker
Copy link
Collaborator

p-bakker commented Oct 5, 2024

Thx for the update

Agree with not including the Symbols: we have cases for matchAll and async support already and better to add the Symbols when those get implemented

gbrail pushed a commit that referenced this pull request Oct 7, 2024
…1611 done by @tonygermano) (#1674)

Implement the "Symbol.toPrimitive" standard symbol and use it in a number of areas. 

Thanks to @tonygermano for the original implementation.
gbrail pushed a commit that referenced this pull request Oct 11, 2024
…@tonygermano) (#1685)

* rewritten plus operator to use toPrimitive
* NativeDate#jsConstructor uses toPrimitive
* Date.prototype[Symbol.toPrimitive] implemented
* fix e4x handling
* fix other backward compatibility issues
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
3 participants