Bug 157970 – Early error on ANY operator before new.target

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

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

[Michael Jasper]

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

[GSkachkov]

I'll take a look to this issue

[Caitlin Potter (:caitp)]

(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.

[GSkachkov]

Ohh, ok.

[Saam Barati]

I was thinking this might require a bytecode generator change as well depending on the operator. Did you need to change anything there Caitlin?

[Caitlin Potter (:caitp)]

Created attachment 317706 [details]
Patch

[Caitlin Potter (:caitp)]

(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.

[Saam Barati]

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.

[Build Bot]

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

[Caitlin Potter (:caitp)]

Created attachment 317718 [details]
Patch

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 317720 [details]
Patch

[Caitlin Potter (:caitp)]

(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

[Saam Barati]

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.

[Caitlin Potter (:caitp)]

Comment on attachment 317720 [details]
Patch

Fire away

[WebKit Commit Bot]

Comment on attachment 317720 [details]
Patch

Clearing flags on attachment: 317720

Committed r220481: <http://trac.webkit.org/changeset/220481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33811245>

[Michael Jasper]

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?

[Joseph Pecoraro]

Nice! Was there a test262 test covering this?