first implementation of Reflect & Proxy support #1332

rbri · 2023-06-06T16:10:25Z

@gbrail, @tuchida, @p-bakker please have a look....

tuchida

Very interesting. I will look it properly later.

tuchida · 2023-06-06T23:18:32Z

src/org/mozilla/javascript/Scriptable.java

@@ -126,6 +126,9 @@ public interface Scriptable {
     */
    public boolean has(int index, Scriptable start);

+    /** A version of delete for properties with Symbol keys. */
+    public boolean has(Symbol key, Scriptable start);


Is it safe compatibility-wise to add methods to Scriptable? If it is not safe, why not add it as Default Methods?

@tuchida you are right, have changed the impl to use the same approach as the other symbol based methods

p-bakker · 2023-06-07T06:52:00Z

testsrc/test262.properties

@@ -1309,7 +1301,36 @@ built-ins/Promise 405/599 (67.61%)

 ~built-ins/Proxy

-~built-ins/Reflect
+built-ins/Reflect 29/139 (20.86%)


For the failing Reflect tests (besides the ones failing because they rely on unsupported features like Proxy and computed-property-names or due to being strict mode related): why are these failing?

these are mainly setter tests, from a first look they fail because i found no good way to provide the correct return value (false) if the setter is 'ignored'.
At least i found no way to do it without changing the current impl.

But i think 20% failing are 80% passing and so it's worth to start with this.....

I think it matters why they fail: if those tests fail because of Reflect exhibiting wrong behavior it's a bigger issue that when they fail because something is not supported and errors out.

Because if it exhibits the wrong behavior, people might start to depend on it and thus fixing it becomes sort of a breaking change down the road

tuchida · 2023-06-07T12:36:11Z

testsrc/org/mozilla/javascript/tests/es6/NativeReflectTest.java

+
+    @Test
+    public void ownKeysDeleteObj() {
+        String js = "var o = { d: 42 };\n" + "delete o.d;\n" + "'' + Reflect.ownKeys({}).length";


Is this correct?

Suggested change

String js = "var o = { d: 42 };\n" + "delete o.d;\n" + "'' + Reflect.ownKeys({}).length";

String js = "var o = { d: 42 };\n" + "delete o.d;\n" + "'' + Reflect.ownKeys(o).length";

You are right, fixed

tuchida · 2023-06-07T12:41:12Z

src/org/mozilla/javascript/NativeReflect.java

+        if (args.length < 3) {
+            throw ScriptRuntime.typeErrorById(
+                    "msg.method.missing.parameter",
+                    "Reflect.apply",


Perhaps it goes like this.

Suggested change

"Reflect.apply",

"Reflect.defineProperty",

Stupid me, fixed

gbrail · 2023-08-02T00:51:55Z

This looks like a good change to me, although I share the concerns about test262 tests in the built-ins/Reflect suite that are failing. Do you think that there's a way to make more of those tests pass?

Otherwise, it depends on the failures because I don't either want to have users depend on an incorrect implementation if we're not passing the tests.

…pport

rbri · 2023-08-17T16:20:34Z

@gbrail, @tuchida, @p-bakker i think the Proxy support is now also worth to look at.

I have not updated the test262.properties because there is still some more work to be done
current status

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

This is now part of HtmlUnit - the implementation makes some huge test suites working.
Have to switch my focus to work on other things during the next days, but will come back from time to time to move this further.

And any help with this is appreciated (as always)

p-bakker · 2023-08-17T17:48:51Z

Nice!

Could you elaborate a bit on the implementation? Does it support Scriptables as target? The tests that currently aren't passing, are they related to specific features? Etc.

rbri · 2023-08-17T18:09:29Z

rbri · 2023-08-17T18:17:00Z

@p-bakker
The implementation supports all the traps and works (from my point of view) for most of the standard cases already. As always implementing all the details and border cases is a painful and time consuming business.
And there are a lot of other test cases related to the proxy support that are now enabled (and i guess ~50% failing). This means we have a lot more test passing but there is still a lot to do.

Like for the Reflect stuff (have that migrated to Lambda in between also) the most critical point is the delete stuff that has to have a return value but the whole Rhino engine does not. At the moment i have no real idea how to fix that in a backward compatible way.

If you like to have a closer look i suggest to

checkout the branch
remove the ~ in front of built-ins/Proxy in test262.properties and run ./gradlew test --tests org.mozilla.javascript.tests.Test262SuiteTest --rerun-tasks -DupdateTest262properties

This will give you a good impression of the effects this (monster) pr has.
And then - if you like - you can pick one test case and try to fix the impl - if you are lucky you will fix more than on in on shot but usually ...

rbri · 2023-08-17T18:21:00Z

Does it support Scriptables as target?

private NativeProxy(ScriptableObject target, Scriptable handler) {
    this.target = target;
    this.handler = handler;

    if (target == null || !(target instanceof Callable)) {
        typeOf = super.getTypeOf();
    } else {
        typeOf = target.getTypeOf();
    }
}

I guess this is the answer ;-)

But in the implementation i tried to be as generic as possible (i guess more generic as your impl) but of course there is room for improvement.

rbri · 2023-08-18T11:04:39Z

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 85/306 (27.78%)
                                              built-ins/Reflect 20/139 (14.39%)
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%)

…e; the object might be a NativeSymbol or a SymbolKey (found while working on mozilla#1332)

…pport

…object might be a NativeSymbol or a SymbolKey (found while working on #1332)

…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)

…pport

p-bakker · 2023-11-05T18:16:50Z

Closed?

rbri · 2023-11-05T18:34:35Z

Not really, looks like i made a mistake somewhere during the last rebase....

rbri force-pushed the reflect branch from 6d55d6e to 3df3c89 Compare June 6, 2023 16:47

tuchida reviewed Jun 6, 2023

View reviewed changes

rbri force-pushed the reflect branch from 83e04e1 to 689f813 Compare June 7, 2023 05:58

p-bakker reviewed Jun 7, 2023

View reviewed changes

tuchida reviewed Jun 7, 2023

View reviewed changes

rbri marked this pull request as draft August 12, 2023 10:12

rbri force-pushed the reflect branch 3 times, most recently from e1b1c67 to 64b4cf3 Compare August 16, 2023 05:06

rbri marked this pull request as ready for review August 16, 2023 05:13

rbri marked this pull request as draft August 16, 2023 05:14

rbri marked this pull request as ready for review August 16, 2023 05:21

rbri marked this pull request as draft August 16, 2023 05:26

rbri changed the title ~~first implementation of NativeReflect~~ first implementation of Reflect & Proxy support Aug 16, 2023

rbri marked this pull request as ready for review August 17, 2023 15:41

rbri marked this pull request as draft August 17, 2023 15:54

rbri added a commit to HtmlUnit/htmlunit-rhino-fork that referenced this pull request Aug 17, 2023

Pull Request mozilla#1332: first implementation of Reflect & Proxy su…

e8092bf

…pport

rbri marked this pull request as ready for review August 18, 2023 11:14

rbri added a commit to rbri/rhino that referenced this pull request Aug 18, 2023

again (see mozilla#1357) take care of ScriptRuntime.isSymbol() == tru…

76fec0a

…e; the object might be a NativeSymbol or a SymbolKey (found while working on mozilla#1332)

rbri mentioned this pull request Aug 18, 2023

take care of SymbolKey before casting #1377

Merged

rbri added a commit to HtmlUnit/htmlunit-rhino-fork that referenced this pull request Aug 18, 2023

Pull Request mozilla#1332: first implementation of Reflect & Proxy su…

5258229

…pport

gbrail pushed a commit that referenced this pull request Aug 18, 2023

again (see #1357) take care of ScriptRuntime.isSymbol() == true; the …

04a2e13

…object might be a NativeSymbol or a SymbolKey (found while working on #1332)

rbri force-pushed the reflect branch from 6770f69 to f0f9aa6 Compare August 19, 2023 07:15

rbri added 23 commits September 24, 2023 08:52

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

e405429

…he other symbol based methods)

fix typos

f90fb11

working on proxy support (wip)

010bd10

apply implemented

00e9444

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

2599d9a

milestone I reached

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

32e9945

convert to lambda

e6e51de

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

bce5b2b

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

b51b7e8

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

f385a1f

…/Reflect 20/139 (14.39%)]

more detailed get trap handling

3322155

NativeProxy has to support revoking from a trap

d794c8a

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

implement more details for defineProperty

0f82fc0

fix missing return for put traps

construct now supports newTarget

a31a210

current status of the impl

e85eab2

spotless

a51eeb0

remove leftover from lambda migration

c8f0609

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

298cbcc

…tibility

preventExtensions has to return a boolean (incompatible change)

22b5f41

one more get fix

bde73f9

next minor step

9d66995

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

c48cfa5

…nit#641)

fix rebase

11aabc5

rbri force-pushed the reflect branch from c6a4d92 to 11aabc5 Compare September 24, 2023 07:16

rbri marked this pull request as draft September 24, 2023 10:46

rbri added a commit to HtmlUnit/htmlunit-rhino-fork that referenced this pull request Oct 3, 2023

Pull Request mozilla#1332: first implementation of Reflect & Proxy su…

2c58f33

…pport

rbri closed this Nov 5, 2023

rbri mentioned this pull request Dec 27, 2023

implementation of Reflect & Proxy support #1431

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first implementation of Reflect & Proxy support #1332

first implementation of Reflect & Proxy support #1332

rbri commented Jun 6, 2023

tuchida left a comment

tuchida Jun 6, 2023

rbri Jun 7, 2023

p-bakker Jun 7, 2023

rbri Jun 7, 2023

p-bakker Jun 8, 2023

tuchida Jun 7, 2023

rbri Jun 7, 2023

tuchida Jun 7, 2023

rbri Jun 7, 2023

gbrail commented Aug 2, 2023

rbri commented Aug 17, 2023

p-bakker commented Aug 17, 2023

rbri commented Aug 17, 2023

rbri commented Aug 17, 2023

rbri commented Aug 17, 2023 •

edited

Loading

rbri commented Aug 18, 2023

p-bakker commented Nov 5, 2023

rbri commented Nov 5, 2023

	String js = "var o = { d: 42 };\n" + "delete o.d;\n" + "'' + Reflect.ownKeys({}).length";
	String js = "var o = { d: 42 };\n" + "delete o.d;\n" + "'' + Reflect.ownKeys(o).length";

first implementation of Reflect & Proxy support #1332

first implementation of Reflect & Proxy support #1332

Conversation

rbri commented Jun 6, 2023

tuchida 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

gbrail commented Aug 2, 2023

rbri commented Aug 17, 2023

p-bakker commented Aug 17, 2023

rbri commented Aug 17, 2023

rbri commented Aug 17, 2023

rbri commented Aug 17, 2023 • edited Loading

rbri commented Aug 18, 2023

p-bakker commented Nov 5, 2023

rbri commented Nov 5, 2023

rbri commented Aug 17, 2023 •

edited

Loading