Bug 147883 - [ES6] Implement computed accessors
Summary: [ES6] Implement computed accessors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 148860
  Show dependency treegraph
 
Reported: 2015-08-11 10:59 PDT by Geoffrey Garen
Modified: 2015-09-08 12:44 PDT (History)
10 users (show)

See Also:


Attachments
Parsing Patch (13.26 KB, patch)
2015-08-14 12:53 PDT, Matthew Mirman
rniwa: review-
rniwa: commit-queue-
Details | Formatted Diff | Diff
Parsing Patch (14.08 KB, patch)
2015-08-14 14:33 PDT, Matthew Mirman
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (652.06 KB, application/zip)
2015-08-14 15:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (783.84 KB, application/zip)
2015-08-14 15:04 PDT, Build Bot
no flags Details
Patch (38.69 KB, patch)
2015-09-04 17:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.26 KB, patch)
2015-09-08 11:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2015-08-11 10:59:41 PDT
Example test case:

=====

var x = 'y',
    valueSet,
    obj = {
      get [x] () { return 1 },
      set [x] (value) { valueSet = value }
    };
obj.y = 'foo';
return obj.y === 1 && valueSet === 'foo';

=====

This will probably require three new opcodes: op_put_getter_by_val, op_put_setter_by_val, op_put_getter_setter_by_val.

Like the equivalent by_id opcodes, these opcodes will mostly be a clone of get_by_val and put_by_val, with slightly different behavior for being getters and setters, and for not consulting the prototype chain.
Comment 1 Matthew Mirman 2015-08-14 12:53:36 PDT
Created attachment 259029 [details]
Parsing Patch
Comment 2 Filip Pizlo 2015-08-14 13:05:40 PDT
Comment on attachment 259029 [details]
Parsing Patch

Do any of these tests cover actually loading or storing to the accessor property?
Comment 3 Ryosuke Niwa 2015-08-14 13:11:42 PDT
Comment on attachment 259029 [details]
Parsing Patch

This patch doesn't handle interactions with non-accessor properties of the same name.
e.g.
var foo = 'a';
{a: 1, get [foo]() {}}
{get [foo]() {}, a: 1}
See http://trac.webkit.org/changeset/184324 which addressed this problem for accessors with regular name.
We probably need to add new op codes to add accessors by value.
Comment 4 Ryosuke Niwa 2015-08-14 13:12:33 PDT
Also see https://bugs.webkit.org/show_bug.cgi?id=142690.
Comment 5 Matthew Mirman 2015-08-14 13:21:01 PDT
(In reply to comment #4)
> Also see https://bugs.webkit.org/show_bug.cgi?id=142690.

(In reply to comment #2)
> Comment on attachment 259029 [details]
> Parsing Patch
> 
> Do any of these tests cover actually loading or storing to the accessor
> property?

As suggested by ggaren, this patch only addresses parsing and will still fail if the code is run.
Comment 6 Matthew Mirman 2015-08-14 14:33:34 PDT
Created attachment 259042 [details]
Parsing Patch

Adds support for classes.

Some overlap with https://bugs.webkit.org/show_bug.cgi?id=142690  will need to pull before submitting.
Comment 7 Ryosuke Niwa 2015-08-14 14:53:32 PDT
Comment on attachment 259042 [details]
Parsing Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259042&action=review

> Source/JavaScriptCore/ChangeLog:4
> +        Implements parsing for computed accessor properties on getters and setters.
> +        https://bugs.webkit.org/show_bug.cgi?id=147883

Can we file a new WebKit bug that blocks this bug and post this patch there?
This bug is about implementing the full feature.
Comment 8 Build Bot 2015-08-14 15:02:42 PDT
Comment on attachment 259042 [details]
Parsing Patch

Attachment 259042 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/59403

New failing tests:
js/parser-syntax-check.html
Comment 9 Build Bot 2015-08-14 15:02:48 PDT
Created attachment 259048 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 10 Build Bot 2015-08-14 15:04:38 PDT
Comment on attachment 259042 [details]
Parsing Patch

Attachment 259042 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/59400

New failing tests:
js/parser-syntax-check.html
Comment 11 Build Bot 2015-08-14 15:04:41 PDT
Created attachment 259049 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 12 Yusuke Suzuki 2015-09-03 23:55:17 PDT
I'll extract Matthew's parsing patch to the separated issue.
Comment 13 Yusuke Suzuki 2015-09-04 17:41:32 PDT
Created attachment 260661 [details]
Patch

WIP: not tested yet
Comment 14 Yusuke Suzuki 2015-09-04 17:45:59 PDT
Since the executing part does not become so large, I'll attempt to implement parsing & executing part in the one patch in this bug.
Comment 15 Yusuke Suzuki 2015-09-08 11:32:43 PDT
Created attachment 260772 [details]
Patch
Comment 16 Geoffrey Garen 2015-09-08 11:58:08 PDT
Comment on attachment 260772 [details]
Patch

r=me
Comment 17 WebKit Commit Bot 2015-09-08 12:44:08 PDT
Comment on attachment 260772 [details]
Patch

Clearing flags on attachment: 260772

Committed r189504: <http://trac.webkit.org/changeset/189504>
Comment 18 WebKit Commit Bot 2015-09-08 12:44:14 PDT
All reviewed patches have been landed.  Closing bug.