Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Let all early errors be SyntaxErrors. #1527

Merged
merged 1 commit into from
Jun 19, 2019

Conversation

rkirsling
Copy link
Member

@rkirsling rkirsling commented May 3, 2019

Resolves #691.

(This does not handle all concerns discussed in that thread, but it does serve as a solution for the OP.
See #691 (comment) onward for my own reasoning; in hindsight, I should've made a separate issue.
)

@bakkot bakkot added the needs consensus This needs committee consensus before it can be eligible to be merged. label May 3, 2019
@ljharb ljharb added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label May 3, 2019
@leobalter
Copy link
Member

Nice, I'd love to see the outcome of this in the meeting. I'm somehow neutral do this change (on how it is concluded), and interested to hear more feedback from TC39.

@mathiasbynens
Copy link
Member

Ross, could you please copy-paste the table from #691 (comment) to your top post here? Saves people a click :)

It’s interesting that these are ReferenceErrors in all engines except for SpiderMonkey, and so this proposal doesn’t really match reality. What’s the concrete benefit of this change?

@rkirsling
Copy link
Member Author

rkirsling commented May 3, 2019

@mathiasbynens My pleasure!

early ReferenceError early SyntaxError
UpdateExpression (0++, ++0) V8, Ch JSC, SM
AssignmentExpression (0 = 0, 0 += 0) V8, Ch, SM JSC

So the status is that JSC has yet to make these early errors at all implemented SyntaxError for both, while SM is refusing to implement an early ReferenceError for UpdateExpression to ensure that we have this conversation. 😁

Mainly, it's strange that out of 40 sections' worth of early errors in the spec, only two include early ReferenceError cases. In particular, [0] = [] and for (0 of []) {} are early SyntaxErrors. So we certainly can't say that the spec is consistent in its current state.

Now, one might question whether it's really worth going back on an earlier decision here, if early error types are mostly "cosmetic" anyway. But as @allenwb pointed out in #691 (comment), it's eval that makes this an observable concern. It would be one thing to have multiple error types that are exclusively "early", but as a user, it's problematic that I can't distinguish an early ReferenceError from a late one (short of matching on error message strings 😳).


Edit (5/16): Updated JSC status from late ReferenceError to early SyntaxError.

spec.html Outdated Show resolved Hide resolved
@allenwb
Copy link
Member

allenwb commented May 3, 2019

Mainly, it's strange that out of 40 sections' worth of early errors in the spec, only two include early ReferenceError cases. In particular, [0] = [] and for (0 of []) {} are early SyntaxErrors. So we certainly can't say that the spec is consistent in its current state.

It's not strange, it's history.

  1. Starting with ES1 these errors were specified to be runtime Reference Errors (in PutValue). But ES1 Clause 16 said that the spec. identifies the "last possible moment" where an error must be detected. Implementations are allowed to detect and report any error earlier including during parsing.

  2. ES3 changed clause 16 to say that runtime errors should be reported when the construct is executed. But it goes on to says that PutValue Reference Errors (and a few others) may be treated "as a syntax error and therefore report it early". It also says that other than the listed special cases, implementations shall not report runtime errors early.

  3. ES5 clause 16 says implementation must treat PutValue Reference Errors as early errors if an early determination can be made that the error will always occur.

  4. ES6 introduced the isValidSimpleAssignment abstraction operation that determines whether a PutValue operations will always produce a ReferenceError and provides static semantic rules that produces an early ReferenceError for these cases when isValidSimpleAssignment return false. It also says that if an eval fails because of an early error that eval throws a SyntaxError exception to report the error.

So the specified error for PutValue has always been ReferenceError. And reporting of early detectable errors has evolved from may be reported as soon as possible to must be an early error. The specification has always been mute about how early errors are reported and whether or not "syntax error" means the same thing as "SyntaxError".

It doesn't matter when we're talking at the level of parse time error messages. It really only makes a difference when early errors are reflected back as actual exceptions via eval and ES6 said those should be SyntaxErrors.

@bakkot
Copy link
Contributor

bakkot commented May 3, 2019

Thanks for the history, @allenwb. It stops a little early, though: #156, included in ES2016, changed the semantics for PeformEval from

If the parse fails or any early errors are detected, throw a SyntaxError exception

to

If the parse fails, throw a SyntaxError exception. If any early errors are detected, throw a SyntaxError or a ReferenceError exception, depending on the type of the error

Which means that it currently is the case that the type of the early error is in fact reflected back as in the actual exception thrown by eval.


As a small aside, it's not quite the case that the types of parse errors at the top level (i.e., not in eval) are unobservable in practice in the most common ECMAScript hosts, i.e. web browsers: compare the alert you get when opening

<!doctype html>
<html>
<head><title>demo</title>
<script>
addEventListener('error', e => { alert(e.error.name); });
</script>
<script>
0++;
</script>
</head>

in Firefox (SyntaxError) vs Chrome (ReferenceError).

(Github doesn't allow data: URIs in links, but you can open

data:text/html;charset=utf-8,%3C%21doctype%20html%3E%0D%0A%3Chtml%3E%0D%0A%3Chead%3E%3Ctitle%3Edemo%3C%2Ftitle%3E%0D%0A%3Cscript%3E%0D%0AaddEventListener%28%27error%27%2C%20e%20%3D%3E%20%7B%20alert%28e.error.name%29%3B%20%7D%29%3B%0D%0A%3C%2Fscript%3E%0D%0A%3Cscript%3E%0D%0A0%2B%2B%3B%0D%0A%3C%2Fscript%3E%0D%0A%3C%2Fhead%3E

if you want.)

But that's neither here nor there.

@allenwb
Copy link
Member

allenwb commented May 3, 2019

Which means that it currently is the case that the type of the early error is in fact reflected back as in the actual exception thrown by eval.

Possibly an ill-advised changed. The intent in the ES6 text was that a caller of eval should only have to check for SyntaxError in their catch to dispatch on the early errors. Not fool proof because of a few unfortunate legacy runtime SyntaxError, but pretty close. Not turning early ReferenceErrors into SyntaxError makes it worse in that respect.

@bakkot
Copy link
Contributor

bakkot commented May 3, 2019

I agree. This patch resolves that difficulty with a slightly larger-than-necessary hammer.

@rkirsling
Copy link
Member Author

This is now implemented in JSC (as they needed to be turned into early errors regardless). 😄
https://trac.webkit.org/r245406

zdobersek pushed a commit to Igalia/webkit that referenced this pull request May 17, 2019
https://bugs.webkit.org/show_bug.cgi?id=197603

Reviewed by Keith Miller.

JSTests:

* test262/expectations.yaml:
Update expectations to reflect new SyntaxErrors.
(Ideally, these should all be viewed as passing in the near future.)

* stress/async-await-basic.js:
* stress/big-int-literals.js:
Update tests to reflect new SyntaxErrors.

* ChakraCore.yaml:
* ChakraCore/test/EH/try6.baseline-jsc:
* ChakraCore/test/Error/variousErrors3.baseline-jsc: Added.
Update baselines to reflect new SyntaxErrors.

Source/JavaScriptCore:

Since ES6, expressions like 0++, ++0, 0 = 0, and 0 += 0 are all specified as early errors:
  https://tc39.github.io/ecma262/#sec-update-expressions-static-semantics-early-errors
  https://tc39.github.io/ecma262/#sec-assignment-operators-static-semantics-early-errors

We currently throw late ReferenceErrors for these -- let's turn them into early SyntaxErrors.
(This is based on the expectation that tc39/ecma262#1527 will be accepted;
if that doesn't come to pass, we can subsequently introduce early ReferenceError and revise these.)

* bytecompiler/NodesCodegen.cpp:
(JSC::PostfixNode::emitBytecode): Add an assert for "function call LHS" case.
(JSC::PrefixNode::emitBytecode): Add an assert for "function call LHS" case.

* parser/ASTBuilder.h:
(JSC::ASTBuilder::isLocation): Added.
(JSC::ASTBuilder::isAssignmentLocation): Fix misleading parameter name.
(JSC::ASTBuilder::isFunctionCall): Added.
(JSC::ASTBuilder::makeAssignNode): Add an assert for "function call LHS" case.
* parser/SyntaxChecker.h:
(JSC::SyntaxChecker::isLocation): Added.
(JSC::SyntaxChecker::isAssignmentLocation): Fix incorrect definition and align with ASTBuilder.
(JSC::SyntaxChecker::isFunctionCall): Added.
* parser/Nodes.h:
(JSC::ExpressionNode::isFunctionCall const): Added.
Ensure that the parser can check whether an expression node is a function call.

* parser/Parser.cpp:
(JSC::Parser<LexerType>::isSimpleAssignmentTarget): Added.
(JSC::Parser<LexerType>::parseAssignmentExpression):
(JSC::Parser<LexerType>::parseUnaryExpression): See below.
* parser/Parser.h:
Throw SyntaxError whenever an assignment or update expression's target is invalid.
Unfortunately, it seems that web compatibility obliges us to exempt the "function call LHS" case in sloppy mode.
(tc39/ecma262#257 (comment))

Additional cleanup items:
  - Make use of `semanticFailIfTrue` for `isMetaProperty` checks, as it's equivalent.
  - Rename `requiresLExpr` to `hasPrefixUpdateOp` since it's now confusing,
    and get rid of `modifiesExpr` since it refers to the exact same condition.
  - Stop setting `lastOperator` near the end -- one case was incorrect and regardless neither is used.

LayoutTests:

* fast/events/window-onerror4-expected.txt:
* ietestcenter/Javascript/11.13.1-1-1-expected.txt:
* ietestcenter/Javascript/11.13.1-1-2-expected.txt:
* ietestcenter/Javascript/11.13.1-1-3-expected.txt:
* ietestcenter/Javascript/11.13.1-1-4-expected.txt:
* js/basic-strict-mode-expected.txt:
* js/dom/assign-expected.txt:
* js/dom/line-column-numbers-expected.txt:
* js/dom/line-column-numbers.html:
* js/dom/postfix-syntax-expected.txt:
* js/dom/prefix-syntax-expected.txt:
* js/dom/script-tests/line-column-numbers.js:
* js/function-toString-parentheses-expected.txt:
* js/parser-syntax-check-expected.txt:
* js/parser-xml-close-comment-expected.txt:
* js/script-tests/function-toString-parentheses.js:
* js/script-tests/parser-syntax-check.js:
Update tests & expectations to reflect new SyntaxErrors.

* js/script-tests/toString-prefix-postfix-preserve-parens.js:
* js/toString-prefix-postfix-preserve-parens-expected.txt:
None of the prefix/postfix tests make sense here now that they're all SyntaxErrors;
remove them and just leave the typeof tests.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@245406 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@rkirsling
Copy link
Member Author

rkirsling commented Jun 4, 2019

This reached consensus today at the Berlin meeting. 🎉

Tests
tc39/test262#2147

Implementations

@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jun 4, 2019
@ljharb ljharb requested review from zenparsing and a team June 4, 2019 15:47
@ljharb ljharb added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jun 12, 2019
@leobalter
Copy link
Member

leobalter commented Jun 12, 2019

tests landed. Edit: sorry, just noticed the label change. Thanks @ljharb

@rkirsling
Copy link
Member Author

(Resolved conflict.)

@ljharb ljharb self-assigned this Jun 19, 2019
@ljharb ljharb force-pushed the abolish-early-reference-error branch from a0a9f8e to a95e95a Compare June 19, 2019 21:40
@ljharb ljharb merged commit a95e95a into tc39:master Jun 19, 2019
@rkirsling rkirsling deleted the abolish-early-reference-error branch June 19, 2019 21:46
pull bot pushed a commit to Richienb/v8 that referenced this pull request Jun 28, 2019
Implement the spec change from the following TC39 PR:
tc39/ecma262#1527

Bug: v8:9326
Change-Id: I9639903b12e7621e323990e2335f00e0313a59c3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1643171
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62451}
pull bot pushed a commit to Richienb/v8 that referenced this pull request Jun 29, 2019
This reverts commit 99fd5b9.

Reason for revert: fails presubmit test:
https://ci.chromium.org/p/v8/builders/ci/V8%20Presubmit/5238
and a nosnap test
https://ci.chromium.org/p/v8/builders/ci/V8%20Win32%20-%20nosnap%20-%20shared/34143

Original change's description:
> Let all early errors be SyntaxErrors.
> 
> Implement the spec change from the following TC39 PR:
> tc39/ecma262#1527
> 
> Bug: v8:9326
> Change-Id: I9639903b12e7621e323990e2335f00e0313a59c3
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1643171
> Reviewed-by: Adam Klein <adamk@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Adam Klein <adamk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#62451}

TBR=adamk@chromium.org,verwaest@chromium.org,rkirsling@gmail.com

Change-Id: If63b97725e9737ad5a98800e1194caf8e9c1c43d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:9326
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1682393
Reviewed-by: Francis McCabe <fgm@chromium.org>
Commit-Queue: Francis McCabe <fgm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62452}
pull bot pushed a commit to Richienb/v8 that referenced this pull request Jul 3, 2019
This is a reland of 99fd5b9 which includes a missed update to
test/test262/test262.status.

Implement the spec change from the following TC39 PR:
tc39/ecma262#1527

Bug: v8:9326
Change-Id: Ie3aac60db550e90fb648fc30886a05419fa41afe
TBR: adamk@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1682989
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62500}
pull bot pushed a commit to Richienb/v8 that referenced this pull request Jul 3, 2019
This reverts commit 89d93e3.

Reason for revert: Breaks layout tests: https://ci.chromium.org/p/v8/builders/ci/V8-Blink%20Linux%2064/32929

Original change's description:
> Reland "Let all early errors be SyntaxErrors."
> 
> This is a reland of 99fd5b9 which includes a missed update to
> test/test262/test262.status.
> 
> Implement the spec change from the following TC39 PR:
> tc39/ecma262#1527
> 
> Bug: v8:9326
> Change-Id: Ie3aac60db550e90fb648fc30886a05419fa41afe
> TBR: adamk@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1682989
> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#62500}

TBR=adamk@chromium.org,gsathya@chromium.org,verwaest@chromium.org,rkirsling@gmail.com

Change-Id: Ia56dcda6780a2b1249749e1e7978b35b5e33fbcf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:9326
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1687678
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62509}
pull bot pushed a commit to Richienb/v8 that referenced this pull request Jul 8, 2019
This is a reland of 89d93e3

Original change's description:
> Reland "Let all early errors be SyntaxErrors."
> 
> This is a reland of 99fd5b9 which includes a missed update to
> test/test262/test262.status.
> 
> Implement the spec change from the following TC39 PR:
> tc39/ecma262#1527
> 
> Bug: v8:9326
> Change-Id: Ie3aac60db550e90fb648fc30886a05419fa41afe
> TBR: adamk@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1682989
> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#62500}

Bug: v8:9326
Change-Id: Ic30280400dfa5b83a4a397888e563eee479446c5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1688271
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Tamer Tas <tmrts@chromium.org>
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62553}
@rkirsling
Copy link
Member Author

FWIW, this now has three landed implementations. 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wording for early reference errors
7 participants