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

FEAT: Support default parameters #1640

Closed 0xe closed 1 month ago

0xe commented 1 month ago

Adds support for default values in function parameters and default values in destructuring assignment.

What's not implemented yet, that I plan to implement subsequently:

(Added ignored tests for these in DefaultParametersTest)

(same changes as https://github.com/mozilla/rhino/pull/1481, but squashed and on top of master)

gbrail commented 1 month ago

Thanks -- this has been outstanding for a long time, and it makes a lot more things work so I'm going to merge it.

I notice that there are test262 tests that actually pass but are marked as failing, so I'm going to fix that and manually merge. Thanks for working on this!

gbrail commented 1 month ago

Merged manually with some test262 fixes -- looking forward to more!

0xe commented 1 month ago

Merged manually with some test262 fixes -- looking forward to more!

Thank you!

p-bakker commented 1 month ago

@0xe would you say we can close #756 now as well?

Unless you'll open a PR for the three things you mentioned (that aren't supported yet) very shortly, cloud you maybe create a follow-up issue, so we don't lose track of it?

Just checking: this PR didn't move the needle on rest/spread syntax anywhere, correct?

0xe commented 1 month ago

@0xe would you say we can close #756 now as well?

Unless you'll open a PR for the three things you mentioned (that aren't supported yet) very shortly, cloud you maybe create a follow-up issue, so we don't lose track of it?

Just checking: this PR didn't move the needle on rest/spread syntax anywhere, correct?

Yeah, I will create issues for the TODO items from this PR tonight.

There should be no changes to rest/spread from this PR.

For https://github.com/mozilla/rhino/issues/756, I think the basic things should work. Yeah, I do think it makes sense to close that and perhaps create issues for things which are still broken.

p-bakker commented 1 month ago

Any chance you can have a look what, if anything, is still not working wrt default values in destructuring assignments?

I would think support/issues between destructuring assignments and default function parameter values to be quite similar, but I might be wrong.

For now I'm holding of on closing #756 a bit until we have some (more) insight into this

Osiris-Team commented 1 month ago

@0xe Wow amazing, 1k lines changed. Thanks for your work! I remember opening the issue 4 years ago as a complete Java noob. Time flies.

0xe commented 1 month ago

Added an issue for known unimplemented things from this PR: https://github.com/mozilla/rhino/issues/1641

0xe commented 1 month ago

Any chance you can have a look what, if anything, is still not working wrt default values in destructuring assignments?

I would think support/issues between destructuring assignments and default function parameter values to be quite similar, but I might be wrong.

For now I'm holding of on closing #756 a bit until we have some (more) insight into this

Makes sense. I will take a look at what's pending with destructuring assignments and update #756.

From my quick check in the shell, following expressions evaluate correctly:

js> let [a, b] = [1, 2];
js> a
1
js> b
2
js> let [c=3, d=4] = [];
js> c
3
js> d
4
js> let {e = 5, f = 6} = {};
js> e
5
js> f
6
js> let {g = 7, h = 8} = {g: 9, h: 10}
js> g
9
js> h
10

One thing that should be addressed in https://github.com/mozilla/rhino/issues/1641 is reading [Symbol.iterator] when destructuring default values from objects.