Annex E incompatibilities should include "early Reference Error" #257
Comments
saurik
changed the title
This is specified as an early error since ECMAScript 5 (ES5, ch. 16). ECMAScript 3 made it an implementation dependent choice whether or not to report early errors for invalid assignments. Specifically this sentence from ES3:
was changed in ES5 to:
The assignment to a function call is a special case: |
|
It's still a dilemma in Chrome whether to prohibit these sorts of things. Does anyone have data about whether they are used on the web? |
|
So this has all been extensively discussed in TC39 when these changes were introduced. When the change from optional early error to mandatory early error was made in ES5, I believe the state of the web was that some of the "top 5" browsers (that was the criteria we used back then) reported early errors and others did not. That meant that code that that depended upon late error treatment was not interoperable across browsers on the web. In other words, it was a non-breaking change. Reference returning host functions (and the syntax that allowed their invocation) was in the spec. to support VBScript compatibility in early versions of IE. I'm pretty sure you can find meeting notes that document this decision. |
|
Thanks for the references, Allen. Unfortunately, not all browsers can ship something just because some other browser in the past has shipped it. In Chrome, we have to worry about compatibility with subsets of the web which currently work on Chrome, such as websites which are written for the 'mobile web', which in practice means WebKit-based browsers. In other parts of the web platform, WebKit prefixes are unfortunately being standardized for similar reasons. "Websites which work on non-Presto browsers" would probably be another similar subset (not sure how much that affected ES2015 decisions). Sometimes these subsets of web usage have things in common with each other which certain browsers don't follow. We would really like to both adhere to standards and not break the web. Is there a strong reason why this has to be an early error? What sort of bad consequences would come if we made these runtime errors in the spec? |
|
I just tested current SpiderMonkey, JSC, V8, and Chakra, and all fail to throw an error for the following code: function f() {
g() = 5;
}They all of course throw an error if Given the above, it seems like the most reasonable thing would be to specify this behavior. If it must be in an Annex, so be it, but I don't think we have any evidence that early errors in such cases are web-compatible. Until someone presents such evidence, it seems safest to specify the current browser behavior. |
|
@bterlson any thoughts here? Would you merge a PR that made this change, or should I put this on the agenda for a future meeting? |
|
I just wanted to note that SpiderMonkey doesn't parse this syntax in strict mode. |
|
@ajklein We should make a proposal PR. There will be debate about where to put it though (annex B vs. main spec) so it's needs-consensus for sure. Sure would be good to get data about how common this is. We were able to make some changes like this in the past - eg. SM/JSC was able to fix the re-evaluation of LHS references issue. |
|
@bakkot implemented data collection in V8 for the |
|
@littledan, in ES3 the grammar included Edit: although as this issue was originally about, those cases should be marked in Annex E. |
This syntax was formerly legal per ECMAScript, but has been a SyntaxError for some time now. V8 deviates from spec in that it is instead a runtime error; we'd like to know if we can get away with removing it (at least in sloppy mode) or if the spec should be changed. c.f. tc39/ecma262#257 (comment) Also add self to authors file BUG=v8:4480 Review-Url: https://codereview.chromium.org/2599253002 Cr-Commit-Position: refs/heads/master@{#41960}
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
|
@littledan @bakkot what were the results of the v8 data collection? |
|
@ljharb Use is very low in strict mode and less low in sloppy mode. Note that Spidermonkey and (as of recently, thanks @rkirsling) JSC only allow it in sloppy mode, so forbidding it in strict mode is very likely to be web compatible. I can't make the call for what browsers would be comfortable shipping, but eyeballing the above graphs (and given that every browser has found they need to allow this syntax in sloppy mode), I think it makes to spec " |
https://bugs.webkit.org/show_bug.cgi?id=209716 Reviewed by Ross Kirsling. JSTests: * stress/logical-assignment-operator-and.js: Added. * stress/logical-assignment-operator-nullish.js: Added. * stress/logical-assignment-operator-or.js: Added. * test262/config.yaml: * test262/expectations.yaml: Right now, test262 expects an early error to be thrown if the lhs is not simple, which does not match what we do with other read-modify assignment operators. This is likely to change in the future to match existing behavior (throw a `ReferenceError`) [1]. [1]: <tc39/ecma262#257 (comment)> Source/JavaScriptCore: Implement the logical assignment operators proposal, which is now Stage 3. It introduces three new assignment operators which will only store the result of the rhs in the lhs if the lhs meets the given condition: - `??=`, for if the lhs is nullish (`null` or `undefined`) - `||=`, for if the lhs is falsy - `&&=`, for if the lhs is truthy This short circuiting can be beneficial as it can avoid a redundant store when used in the common JavaScript programming pattern of "defaulting" a parameter. ```js function foo(x) { x = x || 42; } ``` If `x` is a truthy value, it would result in the rhs `x` being stored back into the lhs `x`. In some situations, this can have negative unintended side-effects, such as for `innerHTML`. Logical assignment operators, however, are defined such that they only store if the rhs is to actually be needed/used, skipping the redundant store and simply returning lhs otherwise. In the case of readonly references, this means that an error is only thrown when the assignment occurs, meaning that if the lhs already satisfies the condition it will be used and returned with no error. * parser/ParserTokens.h: * parser/Lexer.cpp: (JSC::Lexer<T>::lexWithoutClearingLineTerminator): * parser/Parser.cpp: (JSC::Parser<LexerType>::parseAssignmentExpression): * parser/ASTBuilder.h: (JSC::ASTBuilder::makeAssignNode): * parser/Nodes.h: * parser/NodeConstructors.h: (JSC::ShortCircuitReadModifyResolveNode::ShortCircuitReadModifyResolveNode): Added. (JSC::ShortCircuitReadModifyBracketNode::ShortCircuitReadModifyBracketNode): Added. (JSC::ShortCircuitReadModifyDotNode::ShortCircuitReadModifyDotNode): Added. * bytecompiler/NodesCodegen.cpp: (JSC::emitShortCircuitAssignment): Added. (JSC::ShortCircuitReadModifyResolveNode::emitBytecode): Added. (JSC::ShortCircuitReadModifyDotNode::emitBytecode): Added. (JSC::ShortCircuitReadModifyBracketNode::emitBytecode): Added. * runtime/OptionsList.h: Add a `useLogicalAssignmentOperators` setting for controlling this feature. Tools: * Scripts/run-jsc-stress-tests: Canonical link: https://commits.webkit.org/223412@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@260119 268f45cc-cd09-0410-ab3c-d52691b4dbfc
|
For posterity, I believe the historical use of this syntax was for vbarrays: https://matrixlogs.bakkot.com/TC39_Delegates/2021-10-26#L443-L450 |
In earlier ECMAScript versions, the syntax
++++awould lead to a ReferenceError when executed, but was considered valid code: if it existed in some large script and was never used, it would not lead to a failure. In ECMAScript 2015, this behavior has been changed to require an "early Reference Error" per 12.4.1, 12.5.1, and 12.14.1. As far as I can tell, this has not been marked as being an incompatibility.Worse, while we would pray that people were not relying on this strange syntax, Chrome has judged that in at least one case this behavior is not compatible with the legacy web :/. In researching why V8 was not throwing errors in some circumstances (such as how 12.3.1.5 requires that function calls return false for IsValidSimpleAssignmentTarget), I determined that Chrome backed out this change a while back.
https://codereview.chromium.org/217823003
Mozilla actually has a similar compatibility case in SpiderMonkey, though I can't find any discussion to find out exactly why. (They modify IsValidSimpleAssignmentTarget to gate on a feature flag they call "PermitAssignmentToFunctionCalls", which is in turn controlled by whether a "keyed destructuring" is in place, though I haven't yet taken the time to figure out what is going on there... but the syntax works ;P.)
https://dxr.mozilla.org/mozilla-central/rev/97e537f85183ef31481602ab9e5587a6e7d16b4d/js/src/frontend/Parser.cpp#7272
There is a mention in Annex E that "6.2.3: In ECMAScript 2015, Function calls are not allowed to return a Reference value.", but I do not believe that is a sufficient notice of the incompatibility (and would bet it is not intended to address this case, but I could be wrong?). I also did not see anything in Annex B that "for web compatibility reasons some of these early errors are relaxed", but maybe I'm just being dense.
(The versions of JavaScriptCore I have easy access to currently, which may or may not be terribly up-to-date, simply do not implement any of these early error semantics, so they would not have run into any compatibility concerns that I can try to dig up and report.)
The text was updated successfully, but these errors were encountered: