Proxy and Reflect implementation

[rbri]

Next try on the long story.... see #268

This is the follow up of #1431.

Closes #268

[p-bakker]

Not possible to continue with the previous PR? This way we may lose the (unresolved) comments

[rbri]

As promised i have recreated the Proxy/Reflect stuff based on the current status of the impl.
There are still a bunch of failing test but i can't move forward without breaking backward compatibility - already discussed at other places.

From my point of view this is already in a state that can be merged - i will not really work on this because i have some other priorities. Will try to rebase from time to time....

[p-bakker]

There are still a bunch of failing test but i can't move forward without breaking backward compatibility - already discussed at other places.

Could you elaborate on which breaking changes are needed to make Proxy more spec-compliant? From looking at all comment on this and the two preceding PRs, I gathered the following:

Included breaking change: defineProperty now returns boolean instead of void
Excluded breaking change: changing Scriptable.delete to return boolean

Are there more changes that ought to be made, but that you held off on as they would constitute a breaking change?

Also, some of my previous comments remain unanswered. Any chance you can have a look at them?

• implementation of Reflect & Proxy support #1431 (comment) (just the part about invariant checks)
• implementation of Reflect & Proxy support #1431 (comment)
• implementation of Reflect & Proxy support #1431 (comment)

And wrt to whether this is mergeable in its current state or not: I worry that there are details of the spec that haven't been implemented yet and that do not relate to requiring breaking changes, for example (some completely random checks on currently failing tests):

• https://github.com/tc39/test262/tree/main/test/built-ins/Reflect/get/return-value-from-receiver.js: unless there's something fundamentally wrong in the login of looking up a property value up the prototype chain in Rhino already, I would think this would be an implementation detail of Reflect that isn't quite correct yet
• https://github.com/tc39/test262/tree/main/test/built-ins/Reflect/set/receiver-is-not-object.js: Seems a Reflect impl. detail, unless this is an auto-boxing-related issue in Java?
• https://github.com/tc39/test262/tree/main/test/built-ins/Reflect/set/return-false-if-receiver-is-not-writable.js & https://github.com/tc39/test262/tree/main/test/built-ins/Reflect/set/return-false-if-target-is-not-writable.js: Also seem Reflect impl details, unless there are fundamental flaws in PropertyDescriptors on plain JS Objects in Rhino
• https://github.com/tc39/test262/tree/main/test/built-ins/Proxy/set/trap-is-null-receiver.js also looks like an implementation detail

Not trying to be pedantic about this, I really wanna see Proxy support merged (heck, I opened the issue about it 8 and a half years ago :-) ), am just worried about A: merging something that isn't as spec compliant as it can be, meaning we'll have to make (potentially) breaking changes later and B: that if we don't take care of the details now, we'll never get to tme

[rbri]

Current stats

	head	Proxy & Reflect	# solved
built-ins/Array	383/3074 (12.46%)	365/3074 (11.87%)	18
built-ins/ArrayBuffer	142/191 (74.35%)	140/191 (73.3%)	2
built-ins/BigInt	21/75 (28.0%)	20/75 (26.67%)	1
built-ins/Boolean	4/51 (7.84%)	3/51 (5.88%)	1
built-ins/DataView	254/550 (46.18%)	250/550 (45.45%)	4
built-ins/Date	90/770 (11.69%)	87/770 (11.3%)	3
built-ins/Error	6/41 (14.63%)	5/41 (12.2%)	1
built-ins/Function	186/508 (36.61%)	184/508 (36.22%)	2
built-ins/JSON	36/144 (25.69%)	26/144 (18.06%)	10
built-ins/Map	13/171 (7.6%)	12/171 (7.02%)	1
built-ins/Math	51/327 (15.6%)	16/327 (4.89%)	35
built-ins/NativeErrors	31/123 (25.2%)	23/123 (18.7%)	8
built-ins/Number	24/335 (7.16%)	23/335 (6.87%)	1
built-ins/Object	222/3408 (6.51%)	184/3408 (5.55%)	38
built-ins/Promise	406/631 (64.34%)	392/631 (62.12%)	14
built-ins/Proxy		76/311 (25.4%)	235
built-ins/Reflect		13/153 (8.5%)	140
built-ins/RegExp	1169/1854 (63.05%)	1168/1854 (63.0%)	1
built-ins/Set	167/381 (43.83%)	166/381 (43.57%)	1
built-ins/String	140/1182 (11.84%)	127/1182 (10.74%)	1
built-ins/Symbol	26/94 (27.66%)	20/94 (21.28%)	6
built-ins/TypedArray	1091/1422 (76.72%)	1084/1422 (76.23%)	7
built-ins/TypedArrayConstructors	597/735 (81.22%)	566/735 (77.01%)	31
built-ins/WeakMap	15/102 (14.71%)	14/102 (13.73%)	1
built-ins/WeakSet	12/85 (14.12%)	11/85 (12.94%)	1
language/expressions/typeof	2/16 (12.5%)	0/16 (0.0%)	2
language/statements/for-of	453/741 (61.13%)	452/741 (61.0%)	1
			566

[gbrail]

Can you guys (really @rbri and @p-bakker) try and keep status of "open issues" here? At some point very soon I really want to pull the ripcord and merge this and I don't know where we stand regarding @p-bakker's comments a few commits ago.

This all raises the question about big PRs versus feature branches -- we have raised the bar recently on merging incomplete stuff (versus, say, when I put in the native array stuff 10 or so years ago), which is good, but now we're encouraging these gigantic PRs that change dozens of files and require a dedicated coder (thanks, @rbri) with the patience to constantly rebase their stuff...

[rbri]

@gbrail will spend some time tomorrow to (hopefully finally) write some responses to @p-bakker comments. If this help to get this merged....

Outside of this i really support of working in smaller steps. I guess that some of the growing PR's are like that because we need sooo long to get them merged. But this is just my theory...
Maybe PR's like #1656 are a good example - i think we should trust more in our tests suite and if such an pr does not break anything and fixes some 262 test then we should merge as soon as possible.

[gbrail]

Yes, sometimes we are slow to merge things, but we do what we can. If there were a way to add a new language feature in smaller parts, while leaving it disabled (or via feature flag) until complete, that would help a lot. I bet that we could come up with some way to do that.

[rbri]

Now that everybody complains about the downsides of agile we have to start to work more agile in this project :-D :-D

But it is great to see that a bunch of peoples working hard to bring this forward.

[p-bakker]

I think we have to make a distinction between big PRs and PRs touching a lot of files

This PR is big in that the spec for Proxy and to a lesser extend Reflect are big, with a lot of nitty, gritty details, but most of those details are just within their respective implementation classes. It doesn't touch a ton of files and where it does, some of those changes could IMHO be merged separately.

On the flipside, we recently we had a bunch of PR that all touched upon the same areas, which led to a lot of (complicated) rebasing being required, but as detailed elsewhere, at least on the EcmaScript-language front we don't have ton of missing features (albeit those missing being quite big).

The fact that we have test262.properties to record the state of Rhino passing the Tests262 testsuite, which means it'll have changes in any PR that does something on the EcmaScript level plus the fact that some of the stuff being worked on (like in this case Proxy & Reflect) that are used throughout testcode and not just the test related to specific implementation itself is just a fact I don't see we can eliminate, until such time Rhino is more standards-compliant and the 'problem' guess away by itself

I do think it's good that we are more vigilant to making sure that any EcmaScript-related stuff that gets merged is as standard-compliant as possible and properly identify the areas where is not and then create follow-up cases to address those areas. IMHO that is the only way forward if we want to get closer to being spec-compliant.

[p-bakker]

Also: its possible to put comments behind entries in test262.properties. Maybe we ought to use that to refer to cases if we know the reason why a particular test doesn't pass, but can't be make to work with the context of the PR at hand

[rbri]

• implementation of Reflect & Proxy support #1431 (comment) (just the part about invariant checks)
  addressed and fixed
• implementation of Reflect & Proxy support #1431 (comment)
  have added an comment to the discussion - from my point of view the return value is also in the spec
• implementation of Reflect & Proxy support #1431 (comment)
  have documented my reasoning in the discussion - was accepted by paul

Regarding the still failing tests

• i made this pr 1.5 years ago and i spend a lot of time to make as many cases working as i can
• i'm sure that the remaining tests are still failing for good reasons (or because of my limited knowledge, or missing ideas how to fix them)
• have again spend two week to fight with some of the test cases - with some success (i guess 3 more tests are working)

For the remaining tests i hope someone more clever than i can jump in. @gbrail, @p-bakker if you like to see someone to work on further improvements in this area, i fear you have to merge this - otherwise it might be too hard to jump in. But who knows....

And finally i have prepared a new PR #1660 that is again a single commit based on the current head. I guess it will be simpler to merge. (as soon as all tests are passing i will mark this as draft to increase the confusion ;-))