Skip to content

Convert TopLevel from IdScriptableObject to being lambda based.#2155

Merged
gbrail merged 1 commit into
mozilla:masterfrom
aardvark179:aardvark179-top-level-refactor-2
Nov 15, 2025
Merged

Convert TopLevel from IdScriptableObject to being lambda based.#2155
gbrail merged 1 commit into
mozilla:masterfrom
aardvark179:aardvark179-top-level-refactor-2

Conversation

@aardvark179

Copy link
Copy Markdown
Contributor

This is another PR in the long road to separating scopes and scriptable objects, but it's a draft for now because it cast light on a really tricky corner of Rhino's API around ImporterTopLevel, and I don't have a good answer for how to square this circle.

ImporterTopLevel stores additional information against scopes when Java packages are imported, and then uses that to resolve class names at a later time. If two packages are imported with identically named classes then when those classes are looked up this will raise an error. As a top level scope, or an explicitly created scope this is fine if a little tricky, but it also allows for extremely sneaky additional behaviour as tested for in SealedSharedScopeTest which sets up scopes as follows:

        try (Context tmpCtx = Context.enter()) {
            sharedScope = new ImporterTopLevel(tmpCtx, true);
            sharedScope.sealObject();
        }

        ctx = Context.enter();
        ctx.setLanguageVersion(Context.VERSION_DEFAULT);
        scope1 = ctx.newObject(sharedScope);
        scope1.setPrototype(sharedScope);
        scope1.setParentScope(null);
        scope2 = ctx.newObject(sharedScope);
        scope2.setPrototype(sharedScope);
        scope2.setParentScope(null);

and these are then tested like this:

        evaluateString(scope1, "importPackage(javax.naming);");
        evaluateString(scope2, "importPackage(javax.xml.soap);");
        o = evaluateString(scope1, "Name");

For this to work importPackage() has to execute with the caller's scope, not it's own declaration scope, in order to store data against scope1 or scope2 rather than the sealed sharedScope, and it requires scope1 and scope2 to have the sharedScope set as their prototype because only prototype chain look ups pass in the target as the starting scope.

This test / API does not survive the separation of scopes and objects (scopes no longer have prototypes), and also either requires special handling of scopes, or special handling of this. It can barely even survive the change to lambdas.

Looking through the users of ImporterTopLevel on GH I think almost all of them are just using it as a top level scope and are not relying on the creating child scopes, at least not explicitly, but I thought this warranted discussion before breaking an API, especially when we have tests for it, and we even have had bug reports against (#1463).

I wonder if we should deprecate ImporterTopLevel except as a top level scope, (maybe even as that) and strongly push people to using Pacakges for this sort of thing like

var {Integer, String:JString} = Packages.java.lang;

@gbrail , @rPraml , @rbri, and @p-bakker I'm guessing you'll all have opinions on this. I've checked our internal GHE and we don't use importClass and or importPackage anywhere outside a few tests as far as I can tell.

@aardvark179

aardvark179 commented Nov 7, 2025

Copy link
Copy Markdown
Contributor Author

I've done some checks on what can be done with Packages and we might need to fix some bugs there. I should be able to translate

var swingNames = JavaImporter();

swingNames.importPackage(Packages.javax.swing);
swingNames.importPackage(Packages.java.awt);
swingNames.importPackage(Packages.java.awt.event);

into something like

var swingNames = Packages.javax.swing;
var awtNames = Packages.java.awt;
var eventNames = Packages.java.awt.event;

Object.setPrototypeOf(awtNames, eventNames);
Object.setPrototypeOf(swingNames, awtNames);

This mostly works, but there are some rough edges like we report the wrong names in when printing the retrieved classes.

Edit: Oh wait, the prototype thing doesn't work as expected at all.

@p-bakker

p-bakker commented Nov 7, 2025

Copy link
Copy Markdown
Collaborator

Just throwing some extra info into the mix: while refactoring should keep things backwards compatible if possible, the importClass/ImportPackages/Packages api has it's downsides. Graal and Nashorn took another approach, see #1058

@aardvark179

Copy link
Copy Markdown
Contributor Author

How about I try and keep this working for this refactor, but we deprecate the methods with a note that in 2.0 they will throw an error when called. If we then do a release before the main scope separation work lands then that would give people time to move to the package based approach.

We could also work to introduce a Nashorn/GraalJS compatible way of doing it.

@rbri

rbri commented Nov 9, 2025

Copy link
Copy Markdown
Collaborator

HtmlUnit / core-js does not use ImporterTopLevel at all - fine with everything

@aardvark179
aardvark179 force-pushed the aardvark179-top-level-refactor-2 branch from 68d8ae7 to 4b631a3 Compare November 12, 2025 18:46
@aardvark179

Copy link
Copy Markdown
Contributor Author

Okay, this should now all work. I haven't added deprecated annotations because, although the way to use a shared sealed parent scope and child "scopes" will need to change, I think the spirit of it can survive. At the moment, as I said above, setup is done like this:

        try (Context tmpCtx = Context.enter()) {
            sharedScope = new ImporterTopLevel(tmpCtx, true);
            sharedScope.sealObject();
        }

        ctx = Context.enter();
        ctx.setLanguageVersion(Context.VERSION_DEFAULT);
        scope1 = ctx.newObject(sharedScope);
        scope1.setPrototype(sharedScope);
        scope1.setParentScope(null);
        scope2 = ctx.newObject(sharedScope);
        scope2.setPrototype(sharedScope);
        scope2.setParentScope(null);

This won't survive the separation of scopes and scriptable, but if we can separate out the top level scope (a realm) and the global object then I think this can become something like…

        try (Context tmpCtx = Context.enter()) {
            sharedScope = new ImporterTopLevel(tmpCtx, true);
            sharedScope.sealObject();
        }

        ctx = Context.enter();
        ctx.setLanguageVersion(Context.VERSION_DEFAULT);
        scope1 = new TopLevel();
        scope1.getGlobalThis().setPrototype(sharedScope.getGlobalThis());
        scope2 = new TopLevel();
        scope2.getGlobalThis().setPrototype(sharedScope.getGlobalThis());

@aardvark179
aardvark179 marked this pull request as ready for review November 12, 2025 18:56
@gbrail

gbrail commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator

TBH I am not sure why people use TopLevel directly, although importClass and importPackage are in a lot of places. I do know that throughout history people have done just about everything however.

This particular fix is now pretty targeted though, and works fine for me, so I'm going to merge it. Thanks!

@gbrail
gbrail merged commit c1ee781 into mozilla:master Nov 15, 2025
11 checks passed
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.

4 participants