Consider the following code: ```js function F() { if (!new.target) return new F } ``` WebKit Nightly throws `SyntaxError: new.target can't come after a prefix operator.` because of negation (no `++`/`--` there). However, that is valid code. Both Chrome Canary and Firefox parse it OK. This might be a regression caused by https://bugs.webkit.org/show_bug.cgi?id=151148
I ran into this bug, and can confirm that it is a problem. Here is a simple repro case: https://codepen.io/mdjasper/pen/eEWORY It seems related to this line in webkit https://github.com/WebKit/webkit/blob/cfccd2a389b08c585095f58b2cd400c1fe5f0042/Source/JavaScriptCore/parser/Parser.cpp#L4887 Related stack overflow question: https://stackoverflow.com/questions/45578781/new-target-with-a-prefix-operator
I'll take a look to this issue
(In reply to GSkachkov from comment #2) > I'll take a look to this issue I wrote a fix last night after being CC'd on the issue.
Ohh, ok.
I was thinking this might require a bytecode generator change as well depending on the operator. Did you need to change anything there Caitlin?
Created attachment 317706 [details] Patch
(In reply to Saam Barati from comment #5) > I was thinking this might require a bytecode generator change as well > depending on the operator. Did you need to change anything there Caitlin? In my testing I didn't run into any assertions or anything in BytecodeGenerator from this fix, but EWS will provide more info. There are also other problems with unary ops in general: For instance `++!new.target` is allowed, but `!<anything>` is not a valid reference expression. I didn't spend time fixing those separate issues in this patch.
Comment on attachment 317706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317706&action=review > Source/JavaScriptCore/parser/Parser.cpp:4886 > + if (UNLIKELY(isUpdateOp((JSTokenType)lastOperator) && context.isNewTarget(expr))) Style nit: static_cast > JSTests/stress/new-target-syntax-errors.js:91 > +let otherUnaryOperators = ["!", "~", "typeof ", "void ", "delete "]; Also unary “+” I think? > JSTests/stress/new-target-syntax-errors.js:94 > + shouldNotBeSyntaxError(functionBody); Please also run the code instead of just checking for a syntax error. In particular, one node I’m worried about is “delete”. I want to make sure we don’t do anything crazy there.
Comment on attachment 317706 [details] Patch Attachment 317706 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/4284654 New failing tests: stress/default-value-parsing-should-propagate-error.js.default stress/default-value-parsing-should-propagate-error.js.dfg-maximal-flush-validate-no-cjit stress/default-value-parsing-should-propagate-error.js.no-cjit-collect-continuously stress/default-value-parsing-should-propagate-error.js.ftl-no-cjit-b3o1 stress/default-value-parsing-should-propagate-error.js.ftl-no-cjit-validate-sampling-profiler stress/default-value-parsing-should-propagate-error.js.ftl-eager-no-cjit-b3o1 stress/default-value-parsing-should-propagate-error.js.ftl-eager-no-cjit stress/default-value-parsing-should-propagate-error.js.dfg-eager stress/default-value-parsing-should-propagate-error.js.dfg-eager-no-cjit-validate stress/default-value-parsing-should-propagate-error.js.ftl-no-cjit-no-inline-validate stress/default-value-parsing-should-propagate-error.js.ftl-no-cjit-no-put-stack-validate stress/default-value-parsing-should-propagate-error.js.ftl-no-cjit-small-pool stress/default-value-parsing-should-propagate-error.js.no-ftl stress/default-value-parsing-should-propagate-error.js.ftl-eager stress/default-value-parsing-should-propagate-error.js.no-cjit-validate-phases stress/default-value-parsing-should-propagate-error.js.no-llint
Created attachment 317718 [details] Patch
Comment on attachment 317706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317706&action=review >> Source/JavaScriptCore/parser/Parser.cpp:4886 >> + if (UNLIKELY(isUpdateOp((JSTokenType)lastOperator) && context.isNewTarget(expr))) > > Style nit: static_cast Done >> JSTests/stress/new-target-syntax-errors.js:91 >> +let otherUnaryOperators = ["!", "~", "typeof ", "void ", "delete "]; > > Also unary “+” I think? Hmm, good point. >> JSTests/stress/new-target-syntax-errors.js:94 >> + shouldNotBeSyntaxError(functionBody); > > Please also run the code instead of just checking for a syntax error. In particular, one node I’m worried about is “delete”. I want to make sure we don’t do anything crazy there. I've added a bunch of runtime tests as well
Created attachment 317720 [details] Patch
(In reply to Caitlin Potter (:caitp) from comment #12) > Created attachment 317720 [details] > Patch Fixed broken test found by EWS failure, and added more tests for unary abs/negation operators
Comment on attachment 317720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317720&action=review r=me > JSTests/stress/new-target.js:164 > + test(construct(unaryDelete), true, "`delete new.target` should be true"); > + test(call(unaryDelete), true, "`delete new.target` should be true"); FWIW, Chrome Canary gets this wrong, maybe it's worth filing a bug on them.
Comment on attachment 317720 [details] Patch Fire away
Comment on attachment 317720 [details] Patch Clearing flags on attachment: 317720 Committed r220481: <http://trac.webkit.org/changeset/220481>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33811245>
Wow you are awesome! I've never seen a bug report on a large public project seen and worked on so quickly. Kudos :) I'm curious, what is the process to release a fix like this?
Nice! Was there a test262 test covering this?