Bug 143996 - [[Set]] should be properly executed in JS builtins
Summary: [[Set]] should be properly executed in JS builtins
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: 143926
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-21 09:44 PDT by Yusuke Suzuki
Modified: 2015-04-22 11:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.98 KB, patch)
2015-04-22 08:01 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (11.55 KB, patch)
2015-04-22 08:11 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 Yusuke Suzuki 2015-04-21 09:44:16 PDT
Currently, all assignments in builtins JS code is compiled into put_by_val_direct.
However, some functions (like Array.from) needs [[Set]]. (but it is now compiled into pu_by_val_direct, [[DefineOwnProperty]]).

By leveraging bytecode intrinsics, we'll fix it.
There's 2 plans to solve this.

1. Implement the bytecode intrinsic for [[DefineOwnProperty]] and remove the hack for builtins

Currently, assignments in builtins JS code are compiled into put_by_val_direct.
However, it's different from the default JS behavior.
In this plan, we implement the bytecode intrinsic emitting put_by_val_direct and use it explicitly.
And dropping the current hack for builtins.

2. Implement the bytecode intrinsic for [[Set]] and keep the current hack for builtins

However, a lot of builtins code requires [[DefineOwnProperty]] behavior rather than [[Set]].
So keeping the current hack for builtins. By default, assignments in builtins are compiled into put_by_val_direct.
And implementing the bytecode intrinsic emitting put_by_val ([[Set]])
Comment 1 Geoffrey Garen 2015-04-21 11:07:11 PDT
I prefer option (1) because it is more obvious. Bracket access in a builtin has the same meaning as bracket access in normal JavaScript, and if you need non-traditional behavior, you must specify that.

So far, every time I've encountered something in builtins that did not match regular JavaScript -- for example, conversion of put_by_val to put_by_val_direct, conversion of undefined to @undefined, not quite parsing the .js file as a normal program, ASSERTing that no capturing has taken place -- I have been surprised, and lost half a day to debugging. I'd like to remove these surprises over time.

I think it is also nice for our builtins to annotate which features they require that are not possible in the programming language. Over time, we should urge the standards committee to make these features a part of the language.
Comment 2 Yusuke Suzuki 2015-04-21 11:41:01 PDT
(In reply to comment #1)
> I prefer option (1) because it is more obvious. Bracket access in a builtin
> has the same meaning as bracket access in normal JavaScript, and if you need
> non-traditional behavior, you must specify that.
> 
> So far, every time I've encountered something in builtins that did not match
> regular JavaScript -- for example, conversion of put_by_val to
> put_by_val_direct, conversion of undefined to @undefined, not quite parsing
> the .js file as a normal program, ASSERTing that no capturing has taken
> place -- I have been surprised, and lost half a day to debugging. I'd like
> to remove these surprises over time.
> 
> I think it is also nice for our builtins to annotate which features they
> require that are not possible in the programming language. Over time, we
> should urge the standards committee to make these features a part of the
> language.

Agreed.
I think (1) is preferable too because it is intuitive that the witten JS code in builtins works as the same to the usual JS code.
Comment 3 Yusuke Suzuki 2015-04-22 08:01:51 PDT
Created attachment 251314 [details]
Patch
Comment 4 Yusuke Suzuki 2015-04-22 08:11:01 PDT
Created attachment 251315 [details]
Patch
Comment 5 Geoffrey Garen 2015-04-22 11:07:40 PDT
Comment on attachment 251315 [details]
Patch

r=me
Comment 6 Yusuke Suzuki 2015-04-22 11:10:42 PDT
Comment on attachment 251315 [details]
Patch

Thank you for your review!
Comment 7 WebKit Commit Bot 2015-04-22 11:59:38 PDT
Comment on attachment 251315 [details]
Patch

Clearing flags on attachment: 251315

Committed r183117: <http://trac.webkit.org/changeset/183117>
Comment 8 WebKit Commit Bot 2015-04-22 11:59:41 PDT
All reviewed patches have been landed.  Closing bug.