[ESNext] Implement optional chaining
Created attachment 375035 [details] Naive approach (double-evaluates LHS)
Created attachment 375274 [details] Patch
Created attachment 375356 [details] Patch
Comment on attachment 375356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375356&action=review I’m not 100% qualified to review this, but it looks quite good to me. In particular, I think the test covers enough to give me confidence. The main part I don’t feel qualified to review is the byte code generator part; I am not super solid on understanding register allocation and the proper use of reference counting in the byte code generation. > Source/JavaScriptCore/parser/Nodes.h:1334 > + RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = 0) override; > + > + bool isOptionalChain() const override { return true; } In new code I think we’d want nullptr and final here rather than 0 and override. > Source/JavaScriptCore/parser/Parser.cpp:4819 > + semanticFailIfTrue(newCount, "Cannot call constructor in an optional chain"); > + semanticFailIfTrue(baseIsSuper, "super is not valid in this context"); Surprised to see inconsistent capitalization here. I guess because "super" is a keyword we can’t capitalize it. > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:181 > + // Don't display the ?. of an optional call. > + if (idx > 2 && sourceText[idx] == '.' && sourceText[idx - 1] == '?') > + idx -= 2; Which part of the test below covers this? > JSTests/stress/optional-chaining.js:72 > +shouldBe(({})?.a?.['b'], undefined); Do we need some test cases with sequences with more than two "?." to make sure we don’t have a mistake in the loops in the code? Or are two guaranteed to be enough?
(In reply to Darin Adler from comment #4) > Comment on attachment 375356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375356&action=review > > I’m not 100% qualified to review this, but it looks quite good to me. In > particular, I think the test covers enough to give me confidence. The main > part I don’t feel qualified to review is the byte code generator part; I am > not super solid on understanding register allocation and the proper use of > reference counting in the byte code generation. Thanks! > > Source/JavaScriptCore/parser/Nodes.h:1334 > > + RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = 0) override; > > + > > + bool isOptionalChain() const override { return true; } > > In new code I think we’d want nullptr and final here rather than 0 and > override. Sounds good. > > Source/JavaScriptCore/parser/Parser.cpp:4819 > > + semanticFailIfTrue(newCount, "Cannot call constructor in an optional chain"); > > + semanticFailIfTrue(baseIsSuper, "super is not valid in this context"); > > Surprised to see inconsistent capitalization here. I guess because "super" > is a keyword we can’t capitalize it. Yeah, I reused an existing message for the baseIsSuper case here, but I wouldn't need to -- "Cannot use super as the base of an optional chain" would work. > > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:181 > > + // Don't display the ?. of an optional call. > > + if (idx > 2 && sourceText[idx] == '.' && sourceText[idx - 1] == '?') > > + idx -= 2; > > Which part of the test below covers this? Good point, I should check the error messages and not just their types. > > JSTests/stress/optional-chaining.js:72 > > +shouldBe(({})?.a?.['b'], undefined); > > Do we need some test cases with sequences with more than two "?." to make > sure we don’t have a mistake in the loops in the code? Or are two guaranteed > to be enough? I believe two *suffices*, but I'm not opposed to adding some more thorough cases.
(In reply to Ross Kirsling from comment #5) > I believe two *suffices*, but I'm not opposed to adding some more thorough > cases. Ha, turns out two wasn't sufficient at all. This suggestion helped me identify a couple of remaining issues in the logic. :D
Created attachment 375536 [details] Patch
Comment on attachment 375536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375536&action=review Note to self: I suppose a good follow-up task here would be to add "jump if (not) nullish" ops. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2428 > + generator.emitNode(finalDest.get(), m_expr); Whoops, this ought to be emitNodeInTailPosition.
Comment on attachment 375536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375536&action=review >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2428 >> + generator.emitNode(finalDest.get(), m_expr); > > Whoops, this ought to be emitNodeInTailPosition. At times like this I always think: "What kind of test could I add that would have failed?"
(In reply to Darin Adler from comment #9) > > Whoops, this ought to be emitNodeInTailPosition. > > At times like this I always think: "What kind of test could I add that would > have failed?" Indeed. I'll add a couple of cases to tail-call-recognize.js.
Created attachment 375646 [details] Patch
Created attachment 375841 [details] Patch
Comment on attachment 375841 [details] Patch JITify basic tests and re-run EWS following r248426.
Comment on attachment 375841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375841&action=review Nice! I put quick comment. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4969 > + m_optionalChainTargetLabelStack.append(newLabel()); Why is stack required? All the labels in m_optionalChainTargetLabelStack is flushed at the same point. So, only holding one label while m_optionalChainDepth > 0 is enough.
(In reply to Yusuke Suzuki from comment #14) > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4969 > > + m_optionalChainTargetLabelStack.append(newLabel()); > > Why is stack required? All the labels in m_optionalChainTargetLabelStack is > flushed at the same point. So, only holding one label while > m_optionalChainDepth > 0 is enough. I had convinced myself that labels were single-use because I tried reusing them earlier and the jumps seemed to get all mixed up. Apparently I'm wrong though, because your suggestion appears to work -- thank you!
Created attachment 376179 [details] Patch
Can we nest the optional chaining? Like, xxx.?[yyy.?zzz.?aaa].?bbb Then, what happens when we fail inside []?
(In reply to Yusuke Suzuki from comment #17) > Can we nest the optional chaining? Like, > > xxx.?[yyy.?zzz.?aaa].?bbb > > Then, what happens when we fail inside []? Hmm, good point. If we have: let x = { undefined: 3 }; let y = null; Then: x[y?.z] > 3 But: x?.[y?.z] > undefined Will add tests and fix this afternoon, thanks.
(In reply to Ross Kirsling from comment #18) > (In reply to Yusuke Suzuki from comment #17) > > Can we nest the optional chaining? Like, > > > > xxx.?[yyy.?zzz.?aaa].?bbb > > > > Then, what happens when we fail inside []? > > Hmm, good point. > > If we have: > let x = { undefined: 3 }; > let y = null; > > Then: > x[y?.z] > > 3 > > But: > x?.[y?.z] > > undefined > > Will add tests and fix this afternoon, thanks. I think, if the nest happens, we need some sort of stack with scope, and depth needs to be managed by the scope instead of BytecodeGenerator. But if the nest does not happen, we can just put one label and depth in BytecodeGenerator. And label should be one-per-chain. So, maybe, the scope should have that, and scope should have the depth too.
Created attachment 376217 [details] Patch
(In reply to Yusuke Suzuki from comment #19) > I think, if the nest happens, we need some sort of stack with scope, and > depth needs to be managed by the scope instead of BytecodeGenerator. But if > the nest does not happen, we can just put one label and depth in > BytecodeGenerator. > And label should be one-per-chain. So, maybe, the scope should have that, > and scope should have the depth too. Yep, I reintroduced a stack for this purpose and added an m_isOutermost to OptionalChainNode to mark the nodes that need to interact with that stack.
Comment on attachment 376217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376217&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:963 > + void beginOptionalChain(); > + void endOptionalChain(RegisterID* dst, bool isDelete = false); Self-nit: It'd be clearer to call these push / pop now, as they're now guarded at the callsite.
Created attachment 376220 [details] Patch
Comment on attachment 376220 [details] Patch Attachment 376220 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12907455 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
(Grr, we really need to deal with that flaky test.)
Created attachment 376231 [details] Patch
Comment on attachment 376231 [details] Patch (Reuploaded the same patch to make EWS appear green this time...)
Created attachment 376265 [details] Patch
Created attachment 376278 [details] Patch
Added tests for (a?.b)?.() and (a?.b)() now too. Any other concerns, Yusuke?
Ping?
I would love to review+ but I think Yusuke should.
Comment on attachment 376278 [details] Patch r=me
Comment on attachment 376278 [details] Patch Clearing flags on attachment: 376278 Committed r248829: <https://trac.webkit.org/changeset/248829>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54437845>