Octal in directive not caught in ES5 strict-mode code

RESOLVED FIXED in mozilla2.0

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: brendan, Assigned: Waldo)

Tracking

Other Branch
mozilla2.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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
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.
Created attachment 482824 [details] [diff] [review]
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
(Reporter)

Comment 3

7 years ago
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
Attachment #482824 - Flags: review+
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.
(It's on top of a couple patches awaiting review now, so expect this to land in a couple days, not just immediately.)
http://hg.mozilla.org/tracemonkey/rev/3fe1860a15b3
Whiteboard: fixed-in-tracemonkey

Comment 7

7 years ago
http://hg.mozilla.org/mozilla-central/rev/3fe1860a15b3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.