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.

@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.)
   

   rhino/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

   Lines 2128 to 2138 in 3577b83

   	/** 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.

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

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

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

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

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.

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.

@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 🙂

@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.

short info - working on carving out the nect part

@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?

@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.

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