Implement `super` #1735

andreabergia · 2024-11-27T09:00:47Z

This is a huge PR that me and @0xe worked on for a while. We couldn't really figure out a way to make this a nice set of commits for easy review, so we ended up squashing everything because keeping the history wasn't really helpful. Some additional details are implemented as separated PRs stacked on top of this, but this is the smallest PR we could come up with that was coherent.
It is a bit complicated to review I think, sorry about that.

This implements the super operator - which can actually be used outside of classes and will do something that intuitively is "refer to the prototype". The actual details are rather complicated; the MDN page is very clear and detailed. Be sure to read that page carefully before reviewing this.

This PR is stacked on top of #1734 and also includes changes from #1740. It will need to be rebased once #1740 gets merged in. The following PRs are stacked on top of this one:

We propose to merge these stacked PR into this one and merge this one onto master, if accepted.

Parser changes

super, per the spec, should be allowed only inside methods (and class constructors, which are not part of this PR). A method is something that is written with the shorthand syntax:

o = {
  aValidMethod() { ... }
}

Note that the following is not a method:

o = {
  notAMethod: function() { ... } // Normal function call, or arrow functions
}

The changes in the parser mostly relate to keeping track of a flag insideMethod and handling the super token properly, allowing it only in methods. Using super() as a method call is forbidden at the moment - this will need to be changed when classes are implemented, because it needs to be allowed in constructors.

IR Factory

Most of the changes are done to set a property SUPER_PROPERTY_ACCESS on the IR nodes whenever we encounter a super reference; i.e. if we have a reference to super.x, a node GETPROP is created (as for any other property access), but we store the SUPER_PROPERTY_ACCESS flag on it. There's a bit of complication for handling stuff like super.x = expr but the logic is the same - we record that the SETPROP on the left is on super.

Finally, we mark every function that uses super as "requires activation".

Function objects

Every function now stores a property called "home object", following the ECMAScript spec. This is a hidden property of functions that, for methods, is is recorded as the object where the method is declared. For non methods, it is set to null. Thus, for the following JS:

var o = {
  method() {},
  notAMethod: function() {},
}
function freeFunction() {}

we record that method has a [[HomeObject]] set to o, whereas the other functions have it empty (null).

This is done by emitting a new bytecode/new instructions in the JVM bytecode whenever we are initializing a method. To do this, the changes done in #1734 are necessary, because we need to create the object o before we create the closure that represents the method.

Interpreter and java bytecode

The actual logic is implemented in the same way in the interpreter and the compiled classes. The token SUPER will push the "super" object (i.e. the home object's prototype) on the stack, similar to what THIS would do. Then, unfortunately, every kind of property access or method call needs to have special handling - we cannot reuse the logic for (say) GETPROP but we need to have a new token GETPROP_SUPER. There is quite a bit of duplication with the existing tokens, but we figured that this is worthwhile because it doesn't modify the (vastly more common) case of normal property access, and therefore it doesn't make it slower.
We have created various new tokens/bytecodes for the interpreter, and invokedynamic cases for the compiled classes.
The semantics of super access are basically "resolve property on the super object, then access it using the this object", which intuitively means that doing something like super.f() would resolve the f function in the prototype, but then call it with the current this object. An useful analogy is to think about normal virtual calls in other languages - although the semantics are not really the same, because JS is complicated. 🙂 The same idea also applies to getter or setters - they are resolved on the super object, but called with the current this.

Function activation

We mentioned earlier that we mark every method that uses super as "requires activation". The approach we ended up with is to store the current function's home object in the activation record (the NativeCall). The reason for this, rather than just looking it up in the current function, has to do with lambda functions defined inside a method: they need to capture the super, per the ECMAScript spec.

var o = {
  method() {
    return () => super.foo;
  }
}

We have done this by storing the captured home object in the activation. We have looked at what other engines do (v8, jsc, quickjs) and they all seem to be doing something similar, even if the implementation greatly differs between all of them.

Tests

We have implemented a lot of tests in a new class SuperTest, using junit5's nested classes to organize them. We pass most test262 cases except for those related to eval (fixed in #1736). All other test262 cases that do not pass use either class or spread syntax, which are not supported.

Not implemented

We have not added support for all XML stuff.
Also, if super.x is a property stored in the ExternalArrayData, it might not work correctly - we have not tested it.

gbrail · 2024-11-28T01:10:06Z

Wow -- this is really big, and I appreciate the attention to detail. I want to understand it better, since you're adding a bunch of new types of operations in the runtime, so give me (and a few others) a week or so to think carefully about it, but this is really cool and I'm looking forward to having it!

rbri · 2024-11-28T07:28:49Z

@andreabergia @0xe another great step forward, great work.

Will do the usual test with HtmlUnit over the weekend (had to do a release of HtmlUnit before).

Many, many thanks for all the hard work...

gbrail · 2024-12-01T02:00:56Z

Just so you know where I am on this:

Awesome work, love the detailed documentation, thank you!
Is there a way that we can get away without adding "withsuper" versions of a bunch of major operations, since the majority of the code path is actually the same? So far I'm concluding "no," but I'd appreciate a bit more time with the code.
Should any of the operations actually be in "AbstractEcmaObjectOperations"? I'm also concluding "no."

Thanks and we'll get this in to the product soon!

gbrail · 2024-12-02T03:36:02Z

I had a chance to look this over more and read the description -- I think that this is a good approach for implementing this.

Can you please rebase? There were one or two places where I could probably get it right but I'd rather that you all make doubly sure. Thanks!

andreabergia · 2024-12-02T07:36:49Z

I had a chance to look this over more and read the description -- I think that this is a good approach for implementing this.

Can you please rebase? There were one or two places where I could probably get it right but I'd rather that you all make doubly sure. Thanks!

Rebased.

andreabergia · 2024-12-02T08:11:20Z

@gbrail I would suggest approving this, and then checking out the related PRs that are stacked on top of this (#1736, #1737, #1738, #1739, #1741), so that we can merge them all in this branch in some coherent order before merging it to master, since they will likely all conflict with each other.

gbrail

I think this is a good approach and I'm happy to merge it.

It sounds like you'd prefer to merge the five or six other PRs either into this one or into a new PR -- I'm happy either way, just let me know when that is ready. I had a few comments on the other ones but only one was non-optional.

It'd probably help to have marked some of those PRs as "draft" while we go through this but FWIW I don't always look at drafts!

rbri · 2024-12-03T07:17:01Z

@andreabergia, @0xe sorry for not doing the smoke test so far, but there was not really enough time and because of this stacked commits it was complicated to merge into something testable.
Will to the test as soon as the code is on head.

andreabergia · 2024-12-03T16:06:49Z

Merged onto this #1737, #1739

andreabergia · 2024-12-04T10:23:45Z

Merged also #1741 onto this

gbrail · 2024-12-06T05:17:20Z

Looks like progress -- how many other PRs do you want to include in this one before we look at it one more time and merge it? It's looking really close.

andreabergia · 2024-12-06T08:32:37Z

Merged also #1736 onto this one.

@gbrail the only one left is #1738

gbrail · 2024-12-07T00:24:35Z

Thanks -- let me know when you've merged the last one, or if you want me to try. It looks like this one also might conflict with a few other PRs you wrote, so I'm going to hold off on those until we finish this.

andreabergia · 2024-12-07T12:20:34Z

@gbrail I haven't merged it just because it was not approved by you yet 😊
Conflicts won't be a problem, we'll handle them.

gbrail

Thanks for all the work on this! Since I was looking at code coverage for something else, I looked here too, and I saw a few things -- mostly minor, but definitely not new behavior -- that aren't covered by the existing tests.

Can you please take a look at these and see if there are ways that you could hit these few edge cases so that we can have excellent code coverage for all this new code? With those small exceptions, this stuff is very well covered by both new and existing tests.

gbrail · 2024-12-07T18:46:31Z

rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java

        } else if (op.isOperation(RhinoOperation.GETINDEX)) {
            mh = lookup.findStatic(ScriptRuntime.class, "getObjectIndex", mType);
+        } else if (op.isOperation(RhinoOperation.GETINDEXSUPER)) {
+            mh = lookup.findStatic(ScriptRuntime.class, "getSuperIndex", mType);


I'm looking at code coverage in a few places and I sese that this operation isn't covered by the existing test cases.

Removed completely that case because it can never happen!

The reason is that functions with super will always use activation, and Optimizer will not optimize functions that do. Therefore, we will never hit a case of having both properties ISNUMBER_PROP and SUPER_PROPERTY_ACCESS at the same time.
This means that, if we have a super access, we will not have an extra-optimized path for integer properties and we will go to the generic "element" case, but we think it's fine. We're talking about accessing things like super[0] cases here, not something that should be very common.

We haven't added any assertion or check that both properties are not true at the same time because, in any case, the generic "getelem" case can handle integer/double, so even if the optimizer changes, this won't break - it might be slower than for non-super access, but it is correct.

gbrail · 2024-12-07T18:46:44Z

rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java

        } else if (op.isOperation(RhinoOperation.SETINDEX)) {
            mh = lookup.findStatic(ScriptRuntime.class, "setObjectIndex", mType);
+        } else if (op.isOperation(RhinoOperation.SETINDEXSUPER)) {
+            mh = lookup.findStatic(ScriptRuntime.class, "setSuperIndex", mType);


This operation is not covered by code coverage.

Removed (see above)

gbrail · 2024-12-07T18:49:19Z

rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java

-                addDynamicInvoke("PROP:GETINDEX", Signatures.PROP_GET_INDEX);
+                if (isSuper) {
+                    cfw.addALoad(thisObjLocal);
+                    addDynamicInvoke("PROP:GETINDEXSUPER", Signatures.PROP_GET_INDEX_SUPER);


Code coverage is not reaching this new code.

Removed (see above)

gbrail · 2024-12-07T18:49:30Z

rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java

-                addDynamicInvoke("PROP:GETELEMENT", Signatures.PROP_GET_ELEMENT);
+                if (isSuper) {
+                    cfw.addALoad(thisObjLocal);
+                    addDynamicInvoke("PROP:GETELEMENTSUPER", Signatures.PROP_GET_ELEMENT_SUPER);


Code coverage is not reaching this new code.

Removed (see above)

gbrail · 2024-12-07T18:49:40Z

rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java

+        if (isSuper) {
+            cfw.addALoad(thisObjLocal);
+            if (indexIsNumber) {
+                addDynamicInvoke("PROP:SETINDEXSUPER", Signatures.PROP_SET_INDEX_SUPER);


Code coverage is not reaching this new code.

Removed (see above)

gbrail · 2024-12-07T18:52:43Z

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


+        if (result == Scriptable.NOT_FOUND) {
+            result = Undefined.instance;


We never get here in code coverage either, which makes me wonder whether this is really needed, since I've seen bugs before where the confusion between NOT_FOUND and Undefined creeps in.

Added coverage for this

gbrail · 2024-12-07T18:54:27Z

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


+        String s = toString(dblIndex);


We don't get here in code coverage either, which is a pretty minor use case, but still...

Added coverage for this (and similar for setSuperIndex which had the same issue)

gbrail · 2024-12-07T18:59:01Z

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

+                                        // how the home object will ever be null since `super` is
+                                        // legal _only_ in method definitions, where we do have a
+                                        // home object!
+                                        stack[++stackTop] = Undefined.instance;


This seems important but isn't covered by the existing tests.

We agree. But, as discussed by comment, I cannot imagine how that line would ever be hit - the super token should only be allowed inside methods, which will always have a home object set, and all the activations stuff
should work correctly and make the home object accessible. Otherwise, we get syntax errors before we reach the interpreter.

So, this is a case of where we implemented the spec word by word, but we can't imagine that this will ever happen given the actual implementation of the interpreter.

andreabergia · 2024-12-09T09:44:59Z

Merged also the last PR, #1738 onto this one

There is a code path, in compiled mode only (not in interpreted!), where something like `o = { [-1]: xxx }` would not work correctly. The problem is a combination of factors: - in the interpreter, we would treat `-1` as a `double`, not an `int`; - but there are various places in `ScriptRuntime` where integer keys are allowed _only_ for positive or zero numbers, per the spec - otherwise they need to be treated as strings. We have modified the existing test for computed properties and fixed the code. Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

See spec 13.5.1.2 at https://tc39.es/ecma262/#sec-delete-operator-runtime-semantics-evaluation Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Co-authored-by: Andrea Bergia <andrea.bergia@servicenow.com>

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

This reverts commit b845d14. Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Those invokedynamic bridges were never being executed because they where generated only in the combination of `super` with an integer index in optimized functions. However, to make `super` work, the function is marked with "requires activation", which disables the optimizer, which means that the combination `SUPER_PROPERTY_ACCESS` and `ISNUMBER_PROP` actually never occurs. We removed all the dead code to improve coverage. Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

andreabergia · 2024-12-09T11:10:00Z

Rebased on top of #1752 because we discovered a problem when adding tests 🙂

Also, thanks a lot @gbrail for the in-depth review!

gbrail · 2024-12-10T00:35:48Z

OK -- this looks good. Thanks for all the hard work!

This was referenced Nov 27, 2024

Add support for super inside eval #1736

Closed

Handle super.__proto__, which has a special implementation #1737

Closed

Handle super.x++ and similar #1738

Closed

Handle delete super.x by throwing an error #1739

Closed

andreabergia force-pushed the super-object-no-class-activation branch from b5a3170 to f0f46f3 Compare November 27, 2024 09:40

andreabergia mentioned this pull request Nov 27, 2024

Literal object creation rework #1734

Merged

0xe mentioned this pull request Nov 28, 2024

Allow super in default arguments #1741

Closed

andreabergia force-pushed the super-object-no-class-activation branch from f0f46f3 to f82d9aa Compare December 2, 2024 07:35

gbrail approved these changes Dec 3, 2024

View reviewed changes

andreabergia force-pushed the super-object-no-class-activation branch from 4244ad6 to 6bd0cf1 Compare December 4, 2024 10:23

gbrail reviewed Dec 7, 2024

View reviewed changes

andreabergia and others added 5 commits December 9, 2024 11:55

Implement super

8a7172d

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Handle super.__proto__, which has a special implementation

3adb438

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Handle delete super.x by throwing an error

5b2573b

See spec 13.5.1.2 at https://tc39.es/ecma262/#sec-delete-operator-runtime-semantics-evaluation Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Allow super in default arguments

4cf16a9

Co-authored-by: Andrea Bergia <andrea.bergia@servicenow.com>

andreabergia and others added 5 commits December 9, 2024 12:00

Allow super in eval

3c11f61

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Revert "Remove Icode_SWAP as it was unused"

8fc18c3

This reverts commit b845d14. Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Handle super.x++ and similar

8b5ebbb

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

Improved code coverage for super tests

2d7d455

Co-authored-by: Satish Srinivasan <satish.srinivasan@servicenow.com>

andreabergia force-pushed the super-object-no-class-activation branch from 9011ce0 to 2d7d455 Compare December 9, 2024 11:09

gbrail merged commit 9d33f5b into mozilla:master Dec 10, 2024
3 checks passed

andreabergia deleted the super-object-no-class-activation branch December 10, 2024 07:27

p-bakker mentioned this pull request Dec 17, 2024

Support ES2015 Method Definitions - prevent methods as constructor #1299

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `super` #1735

Implement `super` #1735

andreabergia commented Nov 27, 2024 •

edited

Loading

gbrail commented Nov 28, 2024

rbri commented Nov 28, 2024

gbrail commented Dec 1, 2024 •

edited

Loading

gbrail commented Dec 2, 2024

andreabergia commented Dec 2, 2024

andreabergia commented Dec 2, 2024

gbrail left a comment

rbri commented Dec 3, 2024

andreabergia commented Dec 3, 2024

andreabergia commented Dec 4, 2024

gbrail commented Dec 6, 2024

andreabergia commented Dec 6, 2024

gbrail commented Dec 7, 2024

andreabergia commented Dec 7, 2024

gbrail left a comment

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

gbrail Dec 7, 2024

andreabergia Dec 9, 2024

andreabergia commented Dec 9, 2024

andreabergia commented Dec 9, 2024 •

edited

Loading

gbrail commented Dec 10, 2024


		if (result == Scriptable.NOT_FOUND) {
		result = Undefined.instance;

Implement super #1735

Implement super #1735

Conversation

andreabergia commented Nov 27, 2024 • edited Loading

Parser changes

IR Factory

Function objects

Interpreter and java bytecode

Function activation

Tests

Not implemented

gbrail commented Nov 28, 2024

rbri commented Nov 28, 2024

gbrail commented Dec 1, 2024 • edited Loading

gbrail commented Dec 2, 2024

andreabergia commented Dec 2, 2024

andreabergia commented Dec 2, 2024

gbrail left a comment

Choose a reason for hiding this comment

rbri commented Dec 3, 2024

andreabergia commented Dec 3, 2024

andreabergia commented Dec 4, 2024

gbrail commented Dec 6, 2024

andreabergia commented Dec 6, 2024

gbrail commented Dec 7, 2024

andreabergia commented Dec 7, 2024

gbrail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreabergia commented Dec 9, 2024

andreabergia commented Dec 9, 2024 • edited Loading

gbrail commented Dec 10, 2024

Implement `super` #1735

Implement `super` #1735

andreabergia commented Nov 27, 2024 •

edited

Loading

gbrail commented Dec 1, 2024 •

edited

Loading

andreabergia commented Dec 9, 2024 •

edited

Loading