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.