Bug 212679 – Logical Assignment: perform NamedEvaluation of anonymous functions

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Devin Rousso]

```
    let foo = null;
    foo ??= function() {};
    console.log(foo.name); // should be "foo", but is currently ""
```
which matches the functionality of regular assignments
```
    let foo = function() {};
    console.log(foo.name); // "foo"
```

Issue: <https://github.com/tc39/proposal-logical-assignment/issues/23>
PR: <https://github.com/tc39/proposal-logical-assignment/pull/24>

[Devin Rousso]

Created attachment 400884 [details]
Patch

[Ross Kirsling]

Comment on attachment 400884 [details]
Patch

r=me as long as the test262 update is incoming.

[Joseph Pecoraro]

Nice. Yeah there should be some test associated with this!

[Devin Rousso]

I manually tested the test262 updates in <https://github.com/tc39/test262/pull/2649> and we passed them all :)

@yusukesuzuki suggested that I land this rather than wait for the tests so we can get additional fuzzing coverage.

I'm inclined to agree, especially with how simple this change is and the fact that we already pass the tests.

Thoughts?

[Saam Barati]

(In reply to Devin Rousso from comment #4)
> I manually tested the test262 updates in
> <https://github.com/tc39/test262/pull/2649> and we passed them all :)
> 
> @yusukesuzuki suggested that I land this rather than wait for the tests so
> we can get additional fuzzing coverage.
> 
> I'm inclined to agree, especially with how simple this change is and the
> fact that we already pass the tests.
> 
> Thoughts?

Why not just write your own test?

[Devin Rousso]

(In reply to Saam Barati from comment #5)
> (In reply to Devin Rousso from comment #4)
> > I manually tested the test262 updates in <https://github.com/tc39/test262/pull/2649> and we passed them all :)
> > 
> > @yusukesuzuki suggested that I land this rather than wait for the tests so we can get additional fuzzing coverage.
> > 
> > I'm inclined to agree, especially with how simple this change is and the fact that we already pass the tests.
> > 
> > Thoughts?
> 
> Why not just write your own test?

I could, but I would write basically the exact same tests, so I thought it was duplicate effort for no reward (same code being tested twice).

[Saam Barati]

(In reply to Devin Rousso from comment #6)
> (In reply to Saam Barati from comment #5)
> > (In reply to Devin Rousso from comment #4)
> > > I manually tested the test262 updates in <https://github.com/tc39/test262/pull/2649> and we passed them all :)
> > > 
> > > @yusukesuzuki suggested that I land this rather than wait for the tests so we can get additional fuzzing coverage.
> > > 
> > > I'm inclined to agree, especially with how simple this change is and the fact that we already pass the tests.
> > > 
> > > Thoughts?
> > 
> > Why not just write your own test?
> 
> I could, but I would write basically the exact same tests, so I thought it
> was duplicate effort for no reward (same code being tested twice).

it's helpful to have tests that live outside test262

[Yusuke Suzuki]

(In reply to Devin Rousso from comment #6)
> (In reply to Saam Barati from comment #5)
> > (In reply to Devin Rousso from comment #4)
> > > I manually tested the test262 updates in <https://github.com/tc39/test262/pull/2649> and we passed them all :)
> > > 
> > > @yusukesuzuki suggested that I land this rather than wait for the tests so we can get additional fuzzing coverage.
> > > 
> > > I'm inclined to agree, especially with how simple this change is and the fact that we already pass the tests.
> > > 
> > > Thoughts?
> > 
> > Why not just write your own test?
> 
> I could, but I would write basically the exact same tests, so I thought it
> was duplicate effort for no reward (same code being tested twice).

I like adding these tests into JSTests/stress/ even if test262 has duplicate ones.
test262 is shared with other engines. Due to some mistakes in test262, the coverage in JSC can be regressed.
One example is this[1]. test262 putted BigInt to utility file and every tests using this utility file failed in JSC even if the test is not using BigInt.

I'm seeing very similar issues in WPT, and I think dedicated JSC stress tests is better in terms of regression testing.

[1]: https://github.com/tc39/test262/issues/1252

[Devin Rousso]

Created attachment 401174 [details]
Patch

Thanks for the feedback @saamyjoon and @yusukesuzuki, I had not considered test262 itself having issues.  I've added some test cases to JSTests/stress/logical-assignment-{aoperator-nd,coalesce,or}.js :)

[EWS]

Committed r262638: <https://trac.webkit.org/changeset/262638>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401174 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64039177>