ref. #813 reject old octal in property name #814
Conversation
| @@ -3812,6 +3793,25 @@ private StringLiteral createStringLiteral() { | |||
| return s; | |||
| } | |||
|
|
|||
| private NumberLiteral createNumberLiteral() { | |||
| String s = ts.getString(); | |||
| if (this.inUseStrictDirective && ts.isNumberOldOctal()) { | |||
There was a problem hiding this comment.
Thank you for fixing this. I'm not sure what version of the language this changed in (probably many many years ago) but since Rhino supports it today I am quite sure that there is code out there that depends on the old behavior. The way that we are addressing things like that that cause Rhino to diverge from the current spec is to check "Context.getLanguageVersion()'" and only throw the error if the language version is >= Context.VERSION_ES6. If you add this check here, then I think this change is safe.
I'm not sure where in the context of this function you can find the current context, unfortunately -- if you can't, you can call Context.getCurrentContext() and fall back if it returns null.
There was a problem hiding this comment.
This pull request is a change in strict mode.
It is a specification from ES5 to prohibit old octal numbers in strict mode.
https://www.ecma-international.org/ecma-262/5.1/#sec-C
A conforming implementation, when processing strict mode code, may not extend the syntax of NumericLiteral (7.8.3) to include OctalIntegerLiteral as described in B.1.1.
If strict mode is specified, I think that they expect to find problem that doesn't follow the ecma262 strict mode spec.
Not only language version but also strict mode can be specified from the outside.
rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java
Lines 325 to 326 in cee2661
I think you should have done this if compatibility was taken into consideration.
} else if (directive.equals("use strict")) {
inUseStrictDirective = compilerEnv.getLanguageVersion() >= Context.VERSION_ES5;There was a problem hiding this comment.
I'm not sure I understand. The goal is to maintain compatibility with the very large amount of legacy code that uses Rhino, and we have no idea how much of that code depends on "old octals." Ensuring that this change only affects versions >= VERSION_ES6 is the only way that we have right now to allow people interested in spec compliance to get it without breaking Rhino for everyone else. We've done this for a number of changes in the last few releases.
Even when strict mode is enabled, there is still code out there that will break if we make older versions incompatible.
There was a problem hiding this comment.
Do you want to revert it only when it is property name? It was an error from before when it is a expression.
'use strict';
01; // SyntaxError from before'use strict';
({ 01: 1 }); // SyntaxError, if this pull request mergedThere was a problem hiding this comment.
In general, I would like to avoid breaking that worked with the language version < VERSION_ES6 even if the older-version stuff was inconsistent like you described above. So if we can change this so that the change only affects code where the language version is >= VERSION_ES6 then I'm happy to merge it.
There was a problem hiding this comment.
Added compilerEnv.getLanguageVersion() >= Context.VERSION_ES6.
|
This seems like it does it. Thanks! |
ref. #813