first implementation of Reflect & Proxy support

[rbri]

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

[tuchida]

Very interesting. I will look it properly later.

[tuchida]

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

[rbri]

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

[p-bakker]

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?

[rbri]

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.....

[p-bakker]

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]

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";

[rbri]

You are right, fixed

[tuchida]

Perhaps it goes like this.

Suggested change

	"Reflect.apply",
	"Reflect.defineProperty",

[rbri]

Stupid me, fixed

[gbrail]

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.

[rbri]

@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]

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]

[rbri]

@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]

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]

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

[p-bakker]

Closed?

[rbri]

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