601262 - Octal in directive not caught in ES5 strict-mode code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

First function below should throw:

$ ./Darwin_DBG.OBJ/js
js> function f(){"\007";"use strict";}
js> function f(){"\007";"use strict";return "\007";}
typein:2: SyntaxError: octal literals and octal escape sequences are deprecated:
typein:2: function f(){"\007";"use strict";return "\007";}
typein:2: ........................................^
Ollie pointed this out, it was found by the codeplex (Microsoft) ES5 suite, he said.

/be

Comment 1

[Jeff Walden [:Waldo]]

I think this requires some sort of awful interaction between strict mode in the parser (fed by the tokenizer) feeding *back* into the tokenizer.

I saw this in the test suite but figured there was other fruit of better value that was much more worth fixing first.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch and test

The things I do when it's late at night and I can't fall asleep, I tell you...

But, working on this, and seeing the patch (which is a bit gross and isn't immediately obviously correct), it occurs to me that it would be much better to just get the spec changed, perhaps like so: require that all StringLiterals that make up the Directive Prologue not contain a LineContinuation or an EscapeSequence (as is already the case for Use Strict Directives).  It's simple and straightforward, it's much easier to implement (half a dozen lines of change or so plus tests), and it doesn't seem like a restriction anyone would care about.  I'll bring this up on es5-discuss in the morning (late morning, that is, I expect); if that plea fails I'll request review then.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 482824 [details] [diff] [review]
Patch and test

Patch looks fine. Hope it's ok to r+.

>+            if (tokenStream.hasOctalCharacterEscape()) {
>+                reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_DEPRECATED_OCTAL);

This will finger the "use strict"; directive, which is ok (considering how crazy this error is). Bonus if you can squirrel away the TokenPos of the offending directive and arrange for it to get blame.

I like the idea of amending the spec to forbid escape sequences of any kind. Bit late for ES5, alas. Still worth floating to es-discuss.

/be

Comment 4

[Jeff Walden [:Waldo]]

I floated it:

https://mail.mozilla.org/pipermail/es5-discuss/2010-October/003745.html

There didn't seem to be a ton of sentiment in favor of changing the spec, I think generally because other engines don't throw away source as we do.  So let's just land this and be done with it; if ES6 changes it, nobody outside test suites will care if we change then rather than now.

Comment 5

[Jeff Walden [:Waldo]]

(It's on top of a couple patches awaiting review now, so expect this to land in a couple days, not just immediately.)

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/3fe1860a15b3

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3fe1860a15b3