search

mozilla / rhino

Rhino is an open-source implementation of JavaScript written entirely in Java
https://rhino.github.io
Other
4.18k stars 851 forks source link

Proxy and Reflect implementation #1637

Closed rbri closed 1 month ago

rbri commented 2 months ago

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

This is the follow up of #1431.

Closes #268

p-bakker commented 2 months ago

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

rbri commented 1 month ago

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 commented 1 month ago

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?

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

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Regarding the still failing tests

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

p-bakker commented 1 month ago

@rbri this one is now superceded by #1660, right?

rbri commented 1 month ago

@rbri this one is now superceded by #1660, right?

Yes!