Changes to function signatures in preparation for better spec compliance.

[aardvark179]

There are a few functional interfaces in our code which don’t line up well with the spec. Putting this here so we can think about the impact of changing these, or whether we should introduce new interfaces, deprecate the old ones, and look for some combination of overloading and default implementation to maintain compatibility.

[aardvark179]

Needs a rebase for new set methods, I’ll do that later today, but it shows how widespread this change is, at least inside our code base.

Outside our code base I think the largest effect would be on classes that do the whole jsFunction_ style declaration of JS methods, and constructors. I think we could continue to support those with a little change, performing the toObject conversion for those methods when that becomes necessary.

[gbrail]

Would it be possible to write down somewhere, in an issue or discussion, possible fundamental changes to top-level objects like this? I actually think that we can handle a change like this in the project, but if we're going to go and do surgery on Callable and Constructable, we might as well do it once and for all!

[gbrail]

There are so many things to think about with this, since I think it will affect many projects that integrate with Rhino (but that doesn't mean that we shouldn't do it, just that it might be "rhino 2.0")

Two generic questions first. What do you think?

• Scopes have parents but not prototypes -- that sounds right to me and could lead to a lot of simplification
• Objects (Scriptables) have prototypes -- of course that is critical.
• Do Scriptables also have parents? what is their purpose -- do we know or is it historical?
• Should scopes have "delete" methods? If the scope is truly just that, then AFAIK there isn't a way to take something out of a scope.

[aardvark179]

I am inclined to say that Scriptable shouldn’t have a parent, but NativeWith needs one, and removing parent does make it harder to work through the compatibility side of things.

I think scopes need to support delete because of the delete operator, but I will check on the restrictions in the spec.

I’ll keep playing with this for a while before taking this out of draft. I am not sure where associated values belong (can they be internal slits?) and whether we can separate the two concepts further without making the compatibility story harder.

the current prototype does have the advantage that it is mostly a mechanici Change for downstream users. I did play around with some default implementations to make things easier, but for the general case they actually made it way easier easier to introduce subtle bugs.

I’ll create a couple of ancillary branches to show the other possible approaches.

[aardvark179]

2. If the binding for N in envRec cannot be deleted, return false.

Ah, delete is really well specified.

[gbrail]

Also, what do you think of "Scope" instead of "JSScope"? Does that create a conflict somewhere else? I think it's clear and looks nice and we already use super-generic terms like "Context" so it's not like it'd be a new precedent to use a popular term.

[gbrail]

Wish that "Comment" and "Close with Comment" button weren't so close together!

[aardvark179]

I had used Scope but changed because we had AST and JMH scopes in Rhino itself, and scopes in ServiceNow’s code base. Open to other suggestions.

[aardvark179]

Do Scriptables also have parents? what is their purpose -- do we know or is it historical?

I've been thinking about this. It’s mostly historical, and allowed for a fairly minimal API change. Maybe we can do something that separates the two concepts more fully, but I think that might have a lot of knock on effects because i don’t think we reliably distinguish between scope access and property access in our IR.

We do need something to pass in to some of slot map APIs, but maybe we could have a super class of scope and scriptable and handle it that way.

[aardvark179]

Unfortunately I think whole idea of separating scopes and scriptables may be slightly flawed. As I had previously noted with allows an object (which may have a prototype) to be used as a scope, but there is also the global object associated with the Realm record. Since it is a normal object you can do

proto = { foo: 'bar', fizz: 'buzz' }
Object.setPrototypeOf(globalThis, proto)
print(globalThis)
print(foo)

and in a compliant implementation that should print 'bar'.

Luckily that seems to be the limit of Object Environment Record use in the spec, so this might still be doable.

[aardvark179]

Getting rid of parent from scriptable requires fixing a bunch of places where we don’t pass a scope through. I think that’s all doable but might be best done in a separate preparatory PR. I think I also need to go through the spec regarding realms to be sure we aren’t going to cause problems down the road on that front.

[aardvark179]

I'm continuing to pick away at this, and may soon have a set of preparatory PRs. I've had a couple of "Ah ha!" moments that have really helped on this:

1. Apart from with we only ever need to worry about am ancestor. In the case of property lookup in an object we're traversing the prototype chain, and in the case of name lookup in a scope this is traversing the parent scope chain until we reach the global object, at which point we traverse the prototype chain.
2. While threading the scope through various AIPs I started passing the start object on slot map APIs, but this isn't actually right and we should add a separate scope parameter.
3. I created a specific ScriptableObject.NO_SCOPE object to insert wherever we don't currently have a scope in hand. This has been an incredibly useful technique for tracking my progress since I'm doing this in small chunks round other work.

My goal is to remove every use of getParentScope on scriptable object that is not part of actual name resolution / actual scope traversal. Once I've reached that point I'll try separating Scriptable like this into a scriptable half and a scope half with a common ancestor. If that works out then I'll start tidying up and submitting the preparatory PRs.

[aardvark179]

Passing the scope explicitly creates a huge amount of change, I'm not convinced it's worth it and it would make merging this into forks horrible, it's hard to even stage it into more manageable chunks without some fairly severe refactoring to our invokeDynamic linkers (we expect a bunch of them to have the same signatures).

I'm going to try a plan B of maintaining the current scope on the Context itself, and see what that looks like. My aim is still to narrow down the use of getParentScope.

[aardvark179]

Hmm, plan B isn't great for benchmarks in some cases. It establishes a reference from a long lived object (the context) to a lot of short lived objects (activation frames) and so after a while GC starts to be much more problematic. In a lot of cases contexts are short lived and this isn't a problem, but if they are longer lived then it can be a problem.

I mean, we already have this for the interpreter (where we maintain the current frame) but this would add that for compiled mode as well.

[aardvark179]

Okay, plan B wasn't working out nicely at all, so I went back to thinking about why I needed to pass a scope around. Mostly this was to use in calling function objects which should in general be being passed the scope they were declared in. There were a couple of exceptions to this in the code, but as far as I could tell they were potential bugs and fixing them hasn't broken any 262. tests. So I've introduced a default method on Function called getDeclarationScope and changed all the cases that should be thet. Prerry much everything other use of getParentScope is actually to do with name lookup and actual navigation of scopes.

The one exception to this is property descriptor creation, but that is proving much simpler to sort out (they need to get the %Object.prototype% from the top level), and a couple of things round Java objects which I think we can handle.

This is all looking much more tractable. NativeWith will still need to be special, but I think we can greatly reduce the use that, and I found all the places to tidy that up while investigating plan B.

[rbri]

There were a couple of exceptions to this in the code, but as far as I could tell they were potential bugs and fixing them hasn't broken any 262. tests.

Maybe we can start by addressing them one after another with simple PR's. This way they don't get lost and we made some progress; and maybe having a more complete test suite.

And great to see al this deep design work...

[aardvark179]

Yeah, that seems reasonable. I was thinking they were something that should be separated out from the more mechanical changes anyway.

[aardvark179]

Another ah-ha moment. While going through adding the scope to property descriptor stuff I start to ask myself which scope should be passed in by proxy operations. The caller’s, the proxy’s , or the target’s and how this might affect things. Off to the spec I go…

The answer is actually none of them! The real problem we have is that we have conflated the internal method [GetOwnProperty] (which creates an internal object with fields like [value] and [configurable]) with the FromPropertyDescriptor operation which turns that into a JS object.

Disentangling those might make this a whole lot easier.

[aardvark179]

Making good progress on this. I've spent some time today removing uses of setParentScope as a way to find where were were depending on the parent scope of an object. I'm now very close to be able to actual separate scopes and scriptable as @gbrail suggested in #2054. There are a couple of unavoidable API changes in all this, but I'll separate things out into reasonably small PRs and make sure those parts are highlighted.

The remaining Scriptable objects that need to hold onto their parent scope (with the exception of NativeWith and friends) don't need it for name resolution, but only for creating objects within the correct realm, so this should help simplify future optimisations.

Next step is to start the series of changes to separate Scriptable and JSScope so they have a common ancestor but neither inherits from the other. I've been doing all this in stages to avoid any single giant change the world commit, and a lot of the prep work can be split out and submitted before the final change to the type hierarchy.

Currently there are six failing tests around __parent__, which in the general case are unavoidable. Internally we have a small set of cases that do rely on __parent__ for specific Java classes, so I think it might be worth putting in the infrastructure to continue supporting __parent__ in those cases, and for anybody else who needs it, but I would not propose that it continues to work for all Scriptables.

[aardvark179]

Just a quick update. I have a branch with all but 34 tests passing and scopes and scriptable separated with a common ancestor. I have introduced a NativeScope, made NativeCall a subclass of this, and am gradually moving catch scopes etc to use that rather than NativeWith. There are a couple of points I think I want others opinions on:

1. We have a test that __parent__ works in language level 1_8 and earlier. I think that would force paying the cost of parent scopes for a large number of cases where they are not needed, and so I think this is a case where a feature flag is reasonable. Checking internally we only have a very specific situation where we use __parent__ and could support that as a special case.
2. ScriptRuntime contains a lot of different property access methods, and I don’t know if our tests hit everything. So should we change the signatures of all these methods, and accept that some things will attempt to cast things to Scriptable and silently fail, or should we introduce a new scope accessor class, keep it private to the Rhino module, and make it clear that those APIs are internal and might change as we optimise things?
3. TopLevel pretty much equates to a realm, but conflates the global object and the declaration scope. I don’t think I strictly need to change that now, but it might be cleanest to do it as part of this. Then it would become a scope that owns an object, and sealing and freeze can remain things that are done to objects. Something similar applies to the scope for eval as it also separates the global object and a declaration scope for things that are not vars or functions.
4. Things that are both scopes and objects are special. We might want a method to mark these because writing two related instanceof checks gets tedious and error prone really quickly.
5. I think in general parent should be a final property of scopes, should we even include setParentScope on the interface? My inclination is no.

Happy for feedback on the above, @gbrail, @rbri, &
@andreabergia, @0xe, and @balajirrao, or anybody else. I will push the new version of the branch once I’ve tidied up a little. Mostly I want to reorder the changes so they tell a logical story and look less like Fear And Loathing in EcmaScript.

[andreabergia]

Just a quick update. I have a branch with all but 34 tests passing and scopes and scriptable separated with a common ancestor. I have introduced a NativeScope, made NativeCall a subclass of this, and am gradually moving catch scopes etc to use that rather than NativeWith. There are a couple of points I think I want others opinions on:

1. We have a test that __parent__ works in language level 1_8 and earlier. I think that would force paying the cost of parent scopes for a large number of cases where they are not needed, and so I think this is a case where a feature flag is reasonable. Checking internally we only have a very specific situation where we use __parent__ and could support that as a special case.

I don't have strong opinion here, I trust your judgment. A feature flag sounds a good compromise.

2. ScriptRuntime contains a lot of different property access methods, and I don’t know if our tests hit everything. So should we change the signatures of all these methods, and accept that some things will attempt to cast things to Scriptable and silently fail, or should we introduce a new scope accessor class, keep it private to the Rhino module, and make it clear that those APIs are internal and might change as we optimise things?

I am not 100% sure I understand. You mean keep a smaller public interface in ScriptRuntime, and add some internal helper class?

3. TopLevel pretty much equates to a realm, but conflates the global object and the declaration scope. I don’t think I strictly need to change that now, but it might be cleanest to do it as part of this. Then it would become a scope that owns an object, and sealing and freeze can remain things that are done to objects. Something similar applies to the scope for eval as it also separates the global object and a declaration scope for things that are not vars or functions.

We talked about this. I think trying to do that in a separate PR would be a bit better purely for review purposes - this one is going to be giant as is :-)

4. Things that are both scopes and objects are special. We might want a method to mark these because writing two related instanceof checks gets tedious and error prone really quickly.

What did you have in mind?

5. I think in general parent should be a final property of scopes, should we even include setParentScope on the interface? My inclination is no.

How can you implement parent in a backward compatible way (i.e. point 1) if you don't make it mutable though?

[aardvark179]

2. ScriptRuntime contains a lot of different property access methods, and I don’t know if our tests hit everything. So should we change the signatures of all these methods, and accept that some things will attempt to cast things to Scriptable and silently fail, or should we introduce a new scope accessor class, keep it private to the Rhino module, and make it clear that those APIs are internal and might change as we optimise things?

I am not 100% sure I understand. You mean keep a smaller public interface in ScriptRuntime, and add some internal helper class?

I think I'm just worried that many of the existing property access methods have had to pull double duty for scopes and objects, and sometimes rely on complex logic along with instanceof and casting. They may need to remain for compatibility, but maybe we should deprecate them and create a new simpler set of methods for this stuff which we won't make public and can use internally.

3. TopLevel pretty much equates to a realm, but conflates the global object and the declaration scope. I don’t think I strictly need to change that now, but it might be cleanest to do it as part of this. Then it would become a scope that owns an object, and sealing and freeze can remain things that are done to objects. Something similar applies to the scope for eval as it also separates the global object and a declaration scope for things that are not vars or functions.

We talked about this. I think trying to do that in a separate PR would be a bit better purely for review purposes - this one is going to be giant as is :-)

Agreed, I think it's one to try and land before the main scoping changes because it makes them much simpler. I'll try and separate that out.

4. Things that are both scopes and objects are special. We might want a method to mark these because writing two related instanceof checks gets tedious and error prone really quickly.

What did you have in mind?

I was thinking we would want a method like isScopeAndObject() which would have a default implementation of false and things like NativeWith would override to be true. But having thought about it a little more the only objects that fell into this category were top level scopes and NativeWith, and both can be refactored to separate the two concepts. So maybe it won't be necessary.

5. I think in general parent should be a final property of scopes, should we even include setParentScope on the interface? My inclination is no.

How can you implement parent in a backward compatible way (i.e. point 1) if you don't make it mutable though?

It only needs to be mutable for Scriptable (because of where we set it in our runtime), it doesn't need to be mutable for Scope, so setParentScope doesn't need to be part of the common ancestor of the two. __parent__ is a mutable property in Rhino, but I haven't found any code that actually mutates it.