implementation of Reflect & Proxy support #1431

rbri · 2023-12-27T17:30:24Z

sorry made a mistake during the update/rebase of #1332

This is the current version

…he other symbol based methods)

milestone I reached

…/Reflect 20/139 (14.39%)]

SymbolKeys are shallowEq to symbols isArray has to take care of proxies

fix missing return for put traps

…tibility

…nit#641)

rbri · 2023-12-27T17:34:23Z

current status of the impl

built-ins/Array 188/2670 (7.04%)              built-ins/Array 174/2670 (6.52%)
built-ins/Function 186/505 (36.83%)           built-ins/Function 185/505 (36.63%)
built-ins/JSON 36/140 (25.71%)                built-ins/JSON 28/140 (20.0%)
built-ins/Object 122/3150 (3.87%)             built-ins/Object 102/3150 (3.24%)
built-ins/Promise 404/599 (67.45%)            built-ins/Promise 403/599 (67.28%)
                                              built-ins/Proxy 74/306 (24.18%)
                                              built-ins/Reflect 13/139 (9.35%)
language/expressions/typeof 1/16 (6.25%)      language/expressions/typeof 0/16 (0.0%)
language/statements/for-of 471/725 (64.97%)   language/statements/for-of 470/725 (64.83%)

…alue

rbri · 2024-08-08T07:21:05Z

As mentioned already somewhere, this code is in production in the HtmlUnit project (core-js) for a long time now.

I like to update the stats for the current status of the test suite

built-ins/Proxy 80/311 (25.72%)         was built-ins/Proxy 74/306 (24.18%) before
built-ins/Reflect 26/153 (16.99%)       was built-ins/Reflect 13/139 (9.35%) before

and of course many other minor improvements. You can see the whole diff here HtmlUnit@c926d0f

I think the number are still quite good and it's really frustrating to see this has to chance to get merged....

Or the other way around - @gbrail @p-bakker if you agree to merge this Reflect/Proxy impl, I will take the time and build a new pr that is mergable from this impl.

rbri · 2024-08-18T11:54:18Z

Another week week passed by without any response from @gbrail or @p-bakker. So i guess there is no interest in this. I'm not really able to understand why but i'm able to accept this.

p-bakker · 2024-08-18T12:09:42Z

I haven't been able to look into this, so dunno whether I agree this is mergeable at this moment

I think the previous comments (see €1332) about the behaviour of Proxy/Reflect in this PR not being 100% standard compliant (due to issues/limitations elsewhere in Rhino) still stands, but maybe if I get a chance to look into the details I agree those issues shouldn't block merging inow

However, even if the conclusion would be that we shouldn't merge this now, I'd like to keep this PR open and work towards being able to merge it in a hopefully not too distant future.

So I hope you don't mind, but I'll reopen it

p-bakker · 2024-08-18T12:33:15Z

By no means a thorough review, but:

it seems Reflect is much closer to being spec-compliant. Maybe an idea to merge that independent of Proxy?
for Proxy it seems a fair chunk of tests related to invariant checks seem to be failing. Those are the types of EcmaScript incompatibilities I think we don't want to merge, as fixing them later could break code that would've started uses the non-EcmaScript-compatible version

Again: the above 2 comments are by no means a thorough review, but based on a quick glance at the updated test262.properties file

gbrail · 2024-08-18T18:40:26Z

Yeah, sorry, I sometimes lose track of how far along people collectively think that various PRs are, and like email, once they fall to the bottom of the list I sometimes lose track and I need a reminder. (There are tools that help send reminders and add more workflow to PRs -- perhaps we should adopt one!)

Do you think that there are enough tests correctly passing in this PR to warrant merging it? It seemed like we were in an incomplete state from the comments. I'll look at the code and see what I think.

rbri · 2024-08-19T06:32:10Z

It seemed like we were in an incomplete state from the comments.

As is mention several times (here and at other places) i gave up waiting for get this merged an i use this in HtmlUnit/core-js since December 2023. Not sure why you think this is incomplete.

And i made all the statistics to show that this brings (from my point of view) the implementation of the two features in a similar state that we have for all the others (more than ~80% of the tests are passing).

And there are some problems i can't solve because we have to be backward compatible - also discussed several times that i'm in favor of making a hard cut and break some backward compatible to be able to get closer to the current spec. But without that i see no chance to fix more tests. Good ideas are welcome

p-bakker · 2024-08-19T15:59:02Z

Just a random test that fails and stands out to me is for example https://github.com/tc39/test262/blob/main/test/built-ins/Proxy/defineProperty/targetdesc-not-compatible-descriptor-not-configurable-target.js

Just judging the testcode, I don't think it failing has to do with limitations elsewhere in the Rhino codebase, or am I wrong?

I understand the frustration of it not getting merged, but I think we should aim to get each new feature as close as we can possibly get it to the spec, because the reality is that we otherwise never get to finishing up the details

Quite likely your MR as it currently stand already correctly handles the majority of usecases correctly, but unfortunately, when it comes to specs, the devil is in the details....

Besides fixing more of the invariant checks like the one below, I could even see this only getting merged in a v2 (or otherwise behind a flag) if the delete trap cannot currently be implemented in a spec compliant way I'm afraid.

Personally I hope to get a few things resolved in a 1.7.16 release, before we can start to focus on a v2.x.x release, particularly proper block-scoping and hopefully some strict-mode improvements

gbrail · 2024-08-22T04:55:38Z

Sadly I had trouble digesting this but it's very important work and I want to get it in to rhino. Unfortunately it will be a big job to resolve all the conflicts. Would you be willing if I am more diligent about merging it when it's mergeable? Otherwise I can spend time on it but it will take a bit.

rbri · 2024-08-22T07:54:35Z

I can do that but not during the next weeks. Have some workshops and a bit of vacation...

p-bakker · 2024-08-23T06:41:00Z

src/org/mozilla/javascript/ScriptableObject.java

@@ -1581,7 +1581,7 @@ public void defineOwnProperty(Context cx, Object id, ScriptableObject desc) {
     * @param desc the new property descriptor, as described in 8.6.1
     * @param checkValid whether to perform validity checks
     */
-    protected void defineOwnProperty(
+    protected boolean defineOwnProperty(


Why the change from void to boolean returnType? AFAICT all implementations/overrides of this method only ever return true

Mmm, found that the returnType is used in (at least) the reflectImpl.

I wonder if this is the way to go about it: the spec says defineProperty doesn't return a value, it just throws if there's an issue with defining the property

So I wonder if instead of changing the returnType of the method and always returning true, it would not be better to leave the existing impl. as is and in the Reflect.defineProperty just add a return true after the call to defineProperty, as you already catch the exception and do a return false there

https://tc39.es/ecma262/#table-essential-internal-methods

p-bakker · 2024-08-23T06:45:19Z

Not all new methods in the AbstractEcmaObjectOperations class have the additional Context argument that they are said to have

p-bakker · 2024-08-23T07:22:43Z

src/org/mozilla/javascript/ScriptRuntime.java

@@ -3679,6 +3682,10 @@ public static boolean shallowEq(Object x, Object y) {
            if (y instanceof Boolean) {
                return x.equals(y);
            }
+        } else if (x instanceof SymbolKey) {


Don't x.equals(y) and y.equals(x) yield the same result, so you can so if (x instanceof symbolKey || y instanceof symbolKey) ?

Yes that is possible but then the code relies on the assumption that x == null. This is correct here but the null check is so many lines above that it might be overlooked some day.
For me this is the more robust and more readable (because it starts no questions in my mind when reading) version.

p-bakker · 2024-08-23T07:47:40Z

src/org/mozilla/javascript/NativeReflect.java

+            }
+        }
+
+        // hack to set the right prototype before calling the ctor


Maybe elaborate a bit on why this is considered a hack/why this hack is needed?

p-bakker · 2024-08-23T07:48:40Z

src/org/mozilla/javascript/NativeReflect.java

+        return newScriptable;
+    }
+
+    private static Object defineProperty(


Does this need to return Object? It always returns a boolean, no?

it is used by a LambdaFunction that expects an impl of Callable here - so i used the signature from Callable

p-bakker · 2024-08-23T07:51:35Z

src/org/mozilla/javascript/NativeReflect.java

+                Object prop = ScriptableObject.getProperty(target, (Symbol) args[1]);
+                return prop == Scriptable.NOT_FOUND ? Undefined.SCRIPTABLE_UNDEFINED : prop;
+            }
+            if (args[1] instanceof Double) {


Think you already mentioned this elsewhere, but likely should check for Number, not Double

fixed in the new pr

p-bakker · 2024-09-08T07:17:56Z

Converted to draft, to indicate that this PR needs some work before it can be considered for merge, just to help the committers to be able to focus on what is really ready to merge

rbri added 28 commits November 10, 2023 18:00

first implementation of NativeReflect

32c76f9

fix broken backward compatibility (use the same approach as for all t…

2af111f

…he other symbol based methods)

fix typos

6f2e843

working on proxy support (wip)

a14836c

apply implemented

a778d4b

built-ins/Proxy 152/306 (49.67%)

11e7ea1

milestone I reached

built-ins/Proxy 134/306 (43.79%)

ccb2bda

convert to lambda

18f010a

built-ins/Proxy 120/306 (39.22%)

bc02187

built-ins/Proxy 93/306 (30.39%)

42f8dfe

getOwnPropertyDescriptor [built-ins/Proxy 88/306 (28.76%) / built-ins…

166f81c

…/Reflect 20/139 (14.39%)]

more detailed get trap handling

8e0cab8

NativeProxy has to support revoking from a trap

7c16e1b

SymbolKeys are shallowEq to symbols isArray has to take care of proxies

implement more details for defineProperty

bbd5e5d

fix missing return for put traps

construct now supports newTarget

c1ff1fd

current status of the impl

1d8d99d

spotless

9b2993f

remove leftover from lambda migration

db5443e

defineOwnProperty() now return a boolean - THIS BREAKS backward compa…

f2caa7c

…tibility

preventExtensions has to return a boolean (incompatible change)

89fa899

one more get fix

cd353f1

next minor step

f12c678

fix handling of primitive values in Reflect.apply (see HtmlUnit/htmlu…

4f4944a

…nit#641)

fix rebase

976af2c

fix rebase

7288d22

some more test are fixed with the latest rebase

8518a8f

some fixes based on HtmlUnit#9

70bfa44

spotless

144e93c

spotbugs

2c87299

add test for ctor args fix

b51da8c

rbri mentioned this pull request Mar 27, 2024

❤️ Replace the Rhino based JavaScript engine ❤️ HtmlUnit/htmlunit#755

Open

p-bakker linked an issue Apr 9, 2024 that may be closed by this pull request

Support ES2015 Proxies & Reflect API #268

Open

set-callTrap should be called with the value, not the descriptor of v…

36d7615

…alue

rbri mentioned this pull request Aug 18, 2024

Aggressively try to use Integers for arithmetic #1563

Merged

rbri closed this Aug 18, 2024

p-bakker reopened this Aug 18, 2024

p-bakker reviewed Aug 23, 2024

View reviewed changes

p-bakker marked this pull request as draft September 8, 2024 07:15

rbri mentioned this pull request Sep 20, 2024

Proxy and Reflect implementation #1637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implementation of Reflect & Proxy support #1431

implementation of Reflect & Proxy support #1431

rbri commented Dec 27, 2023

rbri commented Dec 27, 2023

rbri commented Aug 8, 2024

rbri commented Aug 18, 2024

p-bakker commented Aug 18, 2024 •

edited

Loading

p-bakker commented Aug 18, 2024

gbrail commented Aug 18, 2024

rbri commented Aug 19, 2024

p-bakker commented Aug 19, 2024 •

edited

Loading

gbrail commented Aug 22, 2024

rbri commented Aug 22, 2024

p-bakker Aug 23, 2024

p-bakker Aug 23, 2024

rbri Sep 25, 2024

p-bakker commented Aug 23, 2024

p-bakker Aug 23, 2024

rbri Sep 21, 2024

p-bakker Sep 23, 2024

p-bakker Aug 23, 2024

rbri Sep 22, 2024

p-bakker Aug 23, 2024

rbri Sep 22, 2024

p-bakker Aug 23, 2024

rbri Sep 22, 2024

p-bakker commented Sep 8, 2024

implementation of Reflect & Proxy support #1431

Are you sure you want to change the base?

implementation of Reflect & Proxy support #1431

Conversation

rbri commented Dec 27, 2023

rbri commented Dec 27, 2023

rbri commented Aug 8, 2024

rbri commented Aug 18, 2024

p-bakker commented Aug 18, 2024 • edited Loading

p-bakker commented Aug 18, 2024

gbrail commented Aug 18, 2024

rbri commented Aug 19, 2024

p-bakker commented Aug 19, 2024 • edited Loading

gbrail commented Aug 22, 2024

rbri commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-bakker commented Aug 23, 2024

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

p-bakker commented Sep 8, 2024

p-bakker commented Aug 18, 2024 •

edited

Loading

p-bakker commented Aug 19, 2024 •

edited

Loading