Separate top level scope and globalThis.

[aardvark179]

Another draft PR on the road to separating scopes and scriptable objects (#2163). I've put the rationale for doing this in a separate issue(#2164)

The global scope in EcmaScript is a a little subtle in that it consists of a declaration environment record and an object environment record for the global object (globalThis). The global object contains variable declarations, function declarations, generator function declarations, async function declarations, and async generator function declarations. Everything else (const declarations, let declarations, class declarations, etc.) goes on declaration environment and is not added to globalThis. This change could make that full separation, but currently there is one test in MozillaSuiteTest which depends on a const declaration being added to the global this. I wonder if we should gate that on a language version.

This is working well apart from a couple of edge cases that I think it's worth talking through:

1. ImporterTopLevel is mostly working, but I don't think I have a great model of how it should work. Currently The behaviour for package lookup is living on the scope half, but I'm wondering if ImporterTopLevel should really put all the behaviour on a custom GlobalThis.
2. One doc test is using a java method to get the XML implementation associated with the current global scope. Currently that is not part of globalThis, but maybe like java imports it should live on that half.
3. Module scopes. These have some existing awkward special handling on FunctionObject (which is currently the only way the existing customGlobal test works). The existing ModuleScope model also doesn't match require in node or common-js.

Let me expand on that last point slightly. If I have a file blah.js containing

function thing() { return this; }
exports.thing = thing;
exports.thing2 = thing();

and from node REPL I do

e = require("./blah.js");
thing = e.thing
thing2 = e.thing2
function thing3() { return this; }
thing4 = thing3();

Then we find thing2 === globalThis and thing2 === thing4. require simply introduces a new scope (just like a function does) rather than what we do with a module scope (a new top level scope that happens to have another top level scope as the prototype). What we have doesn't match that, but it also doesn't match the behaviour of import which should have an entirely separate realm (top level scope).

I think I can see two possible ways to resolve this:

1. If you want custom function provided by a java class on your scope then that java object has to be your globalThis, and the module scope becomes a normal child scope. This would be closest to node as far as I can tell. We might well want to keep the ModuleScope as a special class to mark that we are in a module for relative requires etc.
2. We keep module scope as a top level scope, but with a globalThis of whatever custom type provided. To avoid mutating the main globalThis when executing code in a module we'd need to ensure all modules execute on child scopes of this main module scope (but those need not themselves be ModuleScopes, and we'd need to find all instanceof ModuleScope checks which currently look at the thisObj and change them to appropriate top scope checks.

I don't know if @youngj is still around, still has their use case, and has an opinion on this. I can't tell how well covered this area really is because our Jacoco reports in rhino only cover the tests in rhino, while the coverage report in tests doesn't seem to be configured to show the core runtime classes.

[aardvark179]

Okay, I've almost got this sorted. There remain a small set of failing tests due to nested scopes and associated values, which think I can tackle. This has required an API change in how we create nested scopes from a common sealed parent. I'm going to try and distill that down to a simple API, maybe something like public Toplevel createIsolate() and public TopLevel.createIsolate(ScriptableObject customGlobal) or something like that. Since this would change our documented API we would need to make a change to the docs as well at some point.

[andreabergia]

One concern I have is that it's not great how in so many places you have to do:

        if (scope instanceof TopLevel) {
            scope = ((TopLevel) scope).getGlobalThis();
        }

I am unsure on how we could improve it, but it's really diffused basically everywhere we have Function.call. Feels easy to forget in some place, especially for new code added in the future.

I am not sure why you are doing all the delegation in TopLevel to the globalThis. I have some ideas, but if you could draw a diagram of the scope structure, it would be very helpful for me.

[aardvark179]

@andreabergia I agree that I don't really like the tests for top level that are now scattered through several bits of initialisation code. The original reason to not force the top scope to be a TopLevel was that we had historically allowed custom objects to be used, but I think the separation here allows users to have a custom object as their global this and still force the top level scope to be an actual TopLevel. This would certainly help make behaviour consistent with respect to generator functions and other objects whose prototypes are never created as normal properties of the global scope.

I've prototyped this by doing it in stages

1. Making the object we create if null is provided as the scope.
2. Throwing an error if we are passed something that isn't a TopLevel as the scope
3. Then changing the type.

I'm currently on step 2. and have had make small adjustments to four tests, and have another four which seem to need similar fixes. @gbrail how would you feel about this API change?

[aardvark179]

Okay, I've moved all the initialisation stuff over to requiring a TopLevel, and it wasn't a big change really. I still have one failing test case due to serialisation and class caches. I'll investigate that over the weekend.

There are still couple of explicit checks for TopLevel which I'm also going to try and work through to see which can reasonably be removed without causing too much disruption.

[aardvark179]

Well, that went well. 😆 so it turns out that even if we force the creation of all top level scopes to be actual TopLevels that isn’t enough because of two things

1. Objects that are created to be scopes but have their parent scope set to null.
2. Places where we manage to swap the scope and the global this In our code.

The serialisation problem is similar in that the relationship between the top level scope and the global this has been fully set up at the point when something is trying to rely on that.

I’m setting this back to draft until I can track down those problems.

[aardvark179]

Had a burst of inspiration this morning and got this down to about 30 failing test cases. I think all the remaining cases are actual bugs where we ended up with TypeInfoFactory cached against random objects that hadn't had their scopes set up yet. There's a couple of APIs around defining properties which need fixing, it turns out I even did those fixes on my prototype branch for scopes, so I'll try and bring those across to here.

The serialisation problem is occurring because of the way I handled associated values on the top level scope. To enable the creation of isolates I put the values on the global object rather than the scope, and relied on being able to reach that object before it has been associated with top level scope, so the lookup breaks.

I think the thing to do here is to delegate associated values explicitly when creating isolates. I think two top level scopes created from a sealed shared scope should be guaranteed to share the same type cache.

[gbrail]

I feel like I'm falling behind here, and apologies -- but it's a testament to the great work you're all doing.

I want to think more carefully about what we're doing to backward compatibility with all this stuff. I've never been super happy about the whole "parent scope" thing, but it's a fundamental part of Rhino and creating a "sealed shared scope" is used by a lot of examples and docs and probably real projects.

Can we document somewhere what we're proposing that will require people to change their current code, and then make a plan to stage it in upcoming releases?

Between this and other things we're working on, as well as the increased velocity of work on the project, it may indeed be time for a 1.9.0 or even a 2.0 with some reasonable changes to backward compatibility in favor of spec compliance.

[p-bakker]

Just a possible heads-up when doing stuff in the area of sealed/shared scopes: #1085

[aardvark179]

@gbrail Strongly agree. I think we're approaching the point where we want to do a release prior to any significant API change, and then work to get all those API changes into a 2.0.0 release. I think it might be worth it to create a 2.0 feature branch which we use for this and regularly rebase it on top of master.

I understand why people want to create a sealed parent object as it is a reasonably simple way to avoid repeated initialisation and prevent accidental modification, and we should absolutely ensure there is an API for doing so. I think that can be done, ad we can make it make it easier to use than the current setPrototype/setParentScope dance. That would give people a simple way to do this which we could encourage them to move to, and would hide most of the specifics allowing us to refactor things internally.

[aardvark179]

Okay. Looks like there is a small change required for some Android tests and a little tidy up required. I'll try and do that over the weekend.

[rbri]

Puh, this is huge (and i have to attend a workshop the next days). Will try it next week - hope that is ok

And a question - my top level scope is the window (or the globalworker)

• Window extends EventTarget
• EventTarget extends HtmlUnitScriptable
• HtmlUnitScriptable extends ScriptableObject

Do i have to make Window extending TopLevel?

[aardvark179]

Your Window should become the globalThis of a TopLevel, so you should be able to do something like var scope = new TopLevel(new Window) or use the createIsolate API if you want to make a sealed parent.

[aardvark179]

While preparing a refactoring of where we resolve this during function calls I'm seeing a couple of test failures involving isolates and globalThis. I don't think this was a problem introduced by this change, but I am going to try and debug this and work out the best way to resolve it.

[aardvark179]

Okay. The test failures I saw appear to be long term bugs in our code base which I've fixed as part of my refactoring and nothing introduced by this change.

[aardvark179]

So, reviewing my own change…

I don't like the way we build isolates with a custom Java class, but I didn't like the old way either. They both involved messing with the prototype chain making that custom class unusable as a normal object (or at least really weird since it exposed all of global this in its prototype chain) and it required special handling in FunctionObject.

I think I have a better model of how to do this, which works by using bound functions and avoids any messing with prototype chains.

[aardvark179]

After some experimentation I've gone for cleaning up ModuleScope, and have got a fairly clean special case in FunctionObject on my follow-up clean up of this handling. I still don't like the way we handle isolates with custom classes in RequireTest.customGlobal, but we appear to have specific tests that we aren't implementing this using bound functions or similar tricks, so I guess the people who use this fearure actually depend on that behaviour.

[rbri]

Still think we have a issue with NativeConsole.init(Scriptable scope, boolean sealed, ConsolePrinter printer)

Using the provided scope for all the calls looks good to me, but the last line

ScriptableObject.defineProperty(scope, "console", obj, ScriptableObject.DONTENUM);

should define the property on the globalThis. Therefore i expect to see also the destinationObject for the new property in the method parameter list.

[rbri]

During the last weeks i tried to apply this change to HtmlUnit. It was a hard fight, because there was no clear separation between the globalThis and the TopScope. I think i'm nearly done and fine with this - means: happy to see this merged!

But we have to have in mind, how big this change might be for other applications using Rhino.

Maybe it will be helpfuly to have a clear design sample document including some pictures and sample code that shows how to use Rhino as scripting language for an application.

Including

• the TopLevel / Scope idea
• the globalThis object
• the parentScope hirachy (including the parentScope setup for the globalThis object)
• the helper methods to walk to the TopLevel
• how to add you own implementation of
  • objects/prototypes
  • object properties
  • object functions
• if a function creates a new object - how to setup
  • the scope
  • the prototype (this includes a description how to find the prototype for all user defined objects)

And maybe more (hope the AI can help us a bit to make a shiny document here). Maybe we can start small and improve this step by step

What do you think?

[aardvark179]

So, any property defined on TopLevel is simply defined on the associated globalThis object. That should not remain true in the long term (because consts, classes, and some other things shouldn't end up there, but for now it's true, and is why it does not matter that scope is being passed in that property definition call rather than globalThis. Clearly I need to find a good way to communicate that.

@rbri would you be okay with the architectural docs and examples being a follow up PR so we can merge this and start to work through the PR stack? I think I agree with your list, but I think it's also going to need a little bit of a primer on what realms are, what intrinsic are, and a couple of other spec related things.

P.s. Sorry, that came over grumpier than I intended. My jet lagged brain clearly isn't doing tone well today.

means: happy to see this merged!

🎉 Ship it @gbrail! 😄

[aardvark179]

Oh wait, I need to rebase this before we can merge it.

[rbri]

@rbri would you be okay with the architectural docs and examples being a follow up PR so we can merge this and start to work through the PR stack? I think I agree with your list, but I think it's also going to need a little bit of a primer on what realms are, what intrinsic are, and a couple of other spec related things.

That was exactly the reason for my last post, i think we have to go this way and i no longer like to be something like the blocker for this and all the other cool things on top.

[rbri]

P.s. Sorry, that came over grumpier than I intended. My jet lagged brain clearly isn't doing tone well today.

@aardvark179 at first, my english is not good enought to even notice this ;-) And i fear sometimes my writings have the same 'problem'

And second: we are 'fighting' for the best solution - thats at least for me the fun part of all this

[gbrail]

Let's get this thing moving! It's an important next step in our evolution, and since it will affect every Rhino embedder it's time to rip the band aid off and work on this.

With that said, I have a few small suggestions but if other's are generally OK I think that we should move forward soon. Thanks!

[gbrail]

What breaks if this class overrides get, put, etc, but does not override getIds? Are there cases in which some code will need to enumerate the properties of the global object?

Also is there a reason to override get(int) and put(int)?

[aardvark179]

There's enough annoying edge cases with things like __parent__ that I thought it best to override everything.

[gbrail]

Sorry I was going to fast - why NOT override those two as well? Same with getIds?

[gbrail]

Have not verified this myself but by AI friend tells me that at this point in the constructor, getDeclarationScope() will always return null. That looks accurate to me. Is that going to cause a problem later with the Java embedding?

[aardvark179]

Good catch. This was an artefact of re-ordering a bunch of commits a while ago.

[gbrail]

Just a minor thing but in my new Java code I have been getting more judicious about using "var" in cases like this and if we had done so before then perhaps this would not have been necessary to make this particular change.

[aardvark179]

Yes, I've been using var a lot and it would have reduced a bunch of these changes, but I've had enough push back on changing explicit types to var that I'm wary of doing it as part of a complex behavioural change.

[gbrail]

@rbri this is the first of many that will now start to cause backward compatibility to break, but we've all been talking about this for long enough and we agree it's time. Are you OK? I want to start moving faster on Duncan's big rewrite!

[rbri]

Are you OK? I want to start moving faster on Duncan's big rewrite!

@gbrail all fine for me - let'S move on

[gbrail]

OK -- let's do it and move to the next one. Thanks, all!

[aardvark179]

Cool. Next in the queue should be the changes to function this resolution ( #2328 ), I'll get that prepped when I'm get back home tomorrow.

Oh, wait, I have WiFi. I've done the rebase and made that PR ready for review.

[andreabergia]

Why is this wiring done here, and not somewhere else like in the initialization of TopLevel or at the end of initStandardObject?

[andreabergia]

Why the name? I assume it has something to do with v8 isolates (which I don't know well), but since it actually creates a relationship with the existing toplevel via the prototype and builtins, it's a bit confusing.