| Summary: | [JSC] add missing RequireObjectCoercible() step in destructuring | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Caitlin Potter (:caitp) <caitp> |
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | commit-queue, darin, ggaren, keith_miller, mark.lam, msaboff, saam, youennf, ysuzuki |
| Priority: | P2 | ||
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Caitlin Potter (:caitp)
2015-11-24 22:21:07 PST
Created attachment 266145 [details]
[JSC] add missing RequireObjectCoercible() step in destructuring
Add missing RequireObjectCoercible() for ObjectPatterns. For ArrayPatterns, an iterator is loaded regardless, so explicitly performing these steps is not strictly necessary. The result should be more or less equivalent. This fixes some failing test262 entries
Comment on attachment 266145 [details] [JSC] add missing RequireObjectCoercible() step in destructuring View in context: https://bugs.webkit.org/attachment.cgi?id=266145&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3849 > + emitJumpIfFalse(emitIsUndefined(newTemporary(), value), isNotUndefined.get()); We can save a branch in the normal expected fast path by doing the other check first; then do emitIsUndefined only the failure case to chose which error to throw; undefined is equal to null from the point of view of op_eq_null. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3853 > + emitJumpIfFalse(emitUnaryOp(op_eq_null, newTemporary(), value), target.get()); Unfortunately this emits the “masquerades as null” checking code, so it’s unnecessarily slow. Here we are not literally checking == null so we don’t want that extra logic; it just slows us down. One way to do this would be to add a new op_is_undefined_or_null that doesn’t bother with the masquerading logic. Or op_is_object_coercible. > Source/JavaScriptCore/tests/stress/destructuring-assignment-require-object-coercible.js:15 > +testTypeError(`({ } = null)`, "TypeError: null is not an object"); Would be much better to check all the other types here too, an object, number, boolean, string, and especially a “masquerades as null” value, making sure that they do not throw a type error. Cool, SGTM Created attachment 266330 [details]
[JSC] add missing RequireObjectCoercible() step in destructuring
Added some new opcodes (op_is_just_undefined and op_is_just_undefined_or_null) which remove the unnecessary masquerade-as-undefined code. Probably missing some bit here or there. Few more tests for more object-coercible types
Created attachment 266331 [details]
[JSC] add missing RequireObjectCoercible() step in destructuring
Added some new opcodes (op_is_just_undefined and op_is_just_undefined_or_null) which remove the unnecessary masquerade-as-undefined code. Probably missing some bit here or there. Few more tests for more object-coercible types
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3853
> > + emitJumpIfFalse(emitUnaryOp(op_eq_null, newTemporary(), value), target.get());
>
> Unfortunately this emits the “masquerades as null” checking code, so it’s
> unnecessarily slow. Here we are not literally checking == null so we don’t
> want that extra logic; it just slows us down. One way to do this would be to
> add a new op_is_undefined_or_null that doesn’t bother with the masquerading
> logic. Or op_is_object_coercible.
I hate to start a debate in patch review, but let me suggestion an alternative approach: Just use op_jneq_null. (Note the "eq null" covers null and undefined, as the == operator does. So, I'm suggested a shared error message along the lines of "is not a valid destructuring parameter".)
It's true that jneq_null / jeq_null includes a masquerading check in the interpreter and first-level JIT. However, that check is reasonably cheap, and it disappears to (literally) nothing in the optimizing compilers.
The benefit of reusing the existing opcode is that it's less code to maintain, and it's fewer bytecode dispatch operations in the interpreter.
As an example of maintenance, I believe these new opcodes are not listed in FTLCapabilities.cpp, and so their use will prohibit entering the FTL. (Perhaps I missed something? But I don't see FTLCapabilities.cpp in the diff.)
Perhaps I am being overly pessimistic, and we should introduce new opcodes when it suits us.
Either way, I think it is best not to emit runtime control flow to decide which error message to print. That control flow has a cost in memory and compile time. If a developer wants to know more about the value when things go wrong, we'll automatically stop in the debugger, and they can find out.
Created attachment 266334 [details]
[JSC] add missing RequireObjectCoercible() step in destructuring
Back to simpler version --- remove error selection from bytecode (for now just uses a static message). this causes the error to be different from the one emit for ArrayPatterns, though.
Comment on attachment 266145 [details] [JSC] add missing RequireObjectCoercible() step in destructuring View in context: https://bugs.webkit.org/attachment.cgi?id=266145&action=review >> Source/JavaScriptCore/tests/stress/destructuring-assignment-require-object-coercible.js:15 >> +testTypeError(`({ } = null)`, "TypeError: null is not an object"); > > Would be much better to check all the other types here too, an object, number, boolean, string, and especially a “masquerades as null” value, making sure that they do not throw a type error. Are there any "masquerades as null" values available in standalone jsc stress tests? I suppose this needs a layout test Also, without a new opcode, it won't do the "right" thing for masquerading types, eg `var [a, b, c] = document.all` would be a TypeError when it shouldn't be (HTMLCollection is iterable, and has an ArrayIterator, per http://heycam.github.io/webidl/#es-iterators) but it's fine if someone else wants to add the new opcodes later on Created attachment 266354 [details]
[JSC] add missing RequireObjectCoercible() step in destructuring
remove remaining bits of new opcodes
Comment on attachment 266354 [details] [JSC] add missing RequireObjectCoercible() step in destructuring View in context: https://bugs.webkit.org/attachment.cgi?id=266354&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3986 > + emitOpcode(op_jneq_null); I think we should have a FIXME right here explaining that this doesn’t do the right thing for “masquerades as undefined”; small performance and correctness problem. Comment on attachment 266354 [details] [JSC] add missing RequireObjectCoercible() step in destructuring View in context: https://bugs.webkit.org/attachment.cgi?id=266354&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3986 >> + emitOpcode(op_jneq_null); > > I think we should have a FIXME right here explaining that this doesn’t do the right thing for “masquerades as undefined”; small performance and correctness problem. We want to keep the comment short, but we also want it to address the issues Geoff mentioned; state that it’s worth having this small performance and correctness problem to keep from having to add a new opcode just for this. Created attachment 266368 [details]
[JSC] add missing RequireObjectCoercible() step in destructuring
Added comment indicating temporary bug
Comment on attachment 266368 [details] [JSC] add missing RequireObjectCoercible() step in destructuring Clearing flags on attachment: 266368 Committed r192899: <http://trac.webkit.org/changeset/192899> All reviewed patches have been landed. Closing bug. > Also, without a new opcode, it won't do the "right" thing for masquerading
> types, eg `var [a, b, c] = document.all` would be a TypeError when it
> shouldn't be (HTMLCollection is iterable, and has an ArrayIterator, per
> http://heycam.github.io/webidl/#es-iterators)
Interesting point. I hadn't considered that.
Well, to clarify, the RequireObjectObservable() stuff hasn't been added to ArrayPatterns, since they [[Get]] @@iterator anyways, which effectively does the same thing (albeit with a now-different error message).
But it would still cause a problem in a case like `var { title, lead } = document.all`; or something
|