Closed Bug 497869 Opened 11 years ago Closed 9 years ago

Prohibit all FutureReservedWords (whether restricted to strict mode or not) as identifiers in strict code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b11
Tracking Status
blocking2.0 --- -

People

(Reporter: anon.hui, Assigned: Waldo)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20060421 Firefox/1.0.7 (Ubuntu package 1.0.7)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.12) Gecko/20070508 Firefox/1.5.0.12

The warning should be produce in error console, when future reserved word is used as identifier in javascript.
This is important, since the web developers should get the notice, that they should not use those reserved words.
Without this warning, the web developers may accidentally develop the javascript that cannot run in some other web browser that is strictly conforming to ecmascript standard.


Reproducible: Always

Steps to Reproduce:
1. Open error console
2. Put the text, "var private=50;alert(private)", in the console
3. It produce no warning message
Actual Results:  
No any warning message in the error console.

Expected Results:  
Should produce warning message in the error console.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
The ES5 list of future reserved words evolved at the last face-to-face Ecma TC39 meeting. Waldemar's notes are here:

https://mail.mozilla.org/pipermail/es5-discuss/2009-May/002691.html

/be
Blocks: es5
Status: UNCONFIRMED → NEW
Ever confirmed: true
From that mail and previous messages on es-discuss, it should be clear that most of the ES1-3 future reserved words were not in fact reserved by IE, and since IE allowed most, they were used in web content.

So we should not in any event go back to ES1-3 and try to reserve all the old Java keywords. Only the select ones blessed in the meeting notes. Plus any others we add, but there aren't likely to be any more for ES5.

/be
Assignee: general → jim
"Without this warning, the web developers may accidentally develop the
javascript that cannot run in some other web browser that is strictly
conforming to ecmascript standard."

Just happened to me: I used obj.super in my code for a while until I run it in IE and got the unhelpful IE error message.
blocking2.0: --- → beta6+
ES5 unreserved a bunch of the old Java-infected future reserved words from ES1-3. But super is still reserved. Here's the spec clause, for the record:

7.6.1.2 Future Reserved Words

The following words are used as keywords in proposed extensions and are therefore reserved to allow for the possibility of future adoption of those extensions.

Syntax
  FutureReservedWord :: one of
    class        enum        extends     super
    const        export      import

The following tokens are also considered to be FutureReservedWords when they occur within strict mode code (see 10.1.1). The occurrence of any of these tokens within strict mode code in any context where the occurrence of a FutureReservedWord would produce an error must also produce an equivalent error:

    implements   let         private     public      yield
    interface    package     protected   static

This reminds me: we need to bring our implementation into line with the Harmony proposals for let, const and function in block. We do not want to start throwing in strict mode when someone uses let, since we want Firefox and other XUL app and add-on code that uses let, etc., to be put under "use strict". Need separate bugs on this, pronto.

/be
blocking2.0: beta6+ → betaN+
blocking2.0: betaN+ → -
I don't think we can leave these restrictions on the table for this release.  I would like to land this patch for the release.  But I doubt I can convince everyone of this, so I'll try to create a smaller version that only makes the strict mode future reserved words into syntax errors.
Assignee: jimb → jwalden+bmo
Comment on attachment 506058 [details] [diff] [review]
Patch to implement according to spec (with exceptions for let, const, and yield)

...but I guess it can't hurt to try this now, and cycle back to the less-compliant patch if I get complaints.
Attachment #506058 - Flags: review?(brendan)
Comment on attachment 506058 [details] [diff] [review]
Patch to implement according to spec (with exceptions for let, const, and yield)

>+++ b/js/src/jskeyword.tbl
>@@ -38,87 +38,74 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+/* Miscellaneous tokens implemented via the keyword mechanism */

This should say "Keywords used as primary expressions, sharing the TOK_PRIMARY token kind, distinguished by opcode." Or similar words.

The file is jskeywords.tbl, tokens is too general. Miscellaneous is wrong because these are all one grammatical terminal, TOK_PRIMARY.

>+JS_KEYWORD(false,       TOK_PRIMARY,    JSOP_FALSE,     JSVERSION_DEFAULT)
>+JS_KEYWORD(true,        TOK_PRIMARY,    JSOP_TRUE,      JSVERSION_DEFAULT)
>+JS_KEYWORD(null,        TOK_PRIMARY,    JSOP_NULL,      JSVERSION_DEFAULT)
>+
>+

One blank line is enough.

 . . .
> JS_KEYWORD(with,        TOK_WITH,       JSOP_NOP,       JSVERSION_DEFAULT)
>+
>+

Ditto.

>+/* ECMAScript 5 FutureReservedWord tokens */
>+JS_KEYWORD(class,       TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
>+JS_KEYWORD(enum,        TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
>+JS_KEYWORD(export,      TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
>+JS_KEYWORD(extends,     TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
>+JS_KEYWORD(import,      TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
>+JS_KEYWORD(super,       TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
>+
>+/* FutureReservedWord tokens which we might not treat as such */

"that" not "which" for defining clause linkage, but this comment should not say "might". We are not going to ban const and let in strict mode in Firefox 4, per TC39 discussions and my call as module owner at this late date in the release cycle.

So something like "ES5 future reserved words that we already implement, allowed in our strict mode implementation to ease code migration."

> #if JS_HAS_CONST
> JS_KEYWORD(const,       TOK_VAR,        JSOP_DEFCONST,  JSVERSION_DEFAULT)
> #else
> JS_KEYWORD(const,       TOK_RESERVED,   JSOP_NOP,       JSVERSION_DEFAULT)
> #endif
> 
> 

One blank line is enough.

>+/* ECMAScript 5 tokens which are FutureReservedWord in strict mode code */

These are not "ECMAScript 5 tokens" in any sense other than being FutureReservedWords, and "that" not "which", so:

"Other keywords reserved for future use in strict mode."

and move LET up to group with CONST, before the "Other keywords reserved for future use in strict mode".

r=me with these changes.

/be
Attachment #506058 - Flags: review?(brendan) → review+
... and move YIELD up too, so CONST, LET, YIELD in a group, then the strict mode future reserved words we do ban.

I didn't get comment 6's ref to a "less-complaint patch". What would that patch allow? I don't think we want to unreserve "private" or other ES5 future reserved words that we don't yet implement.

/be
(In reply to comment #7)
> >+/* FutureReservedWord tokens which we might not treat as such */
> 
> "that" not "which" for defining clause linkage

A grammar "rule" made to be broken if ever there was one.  But I digress.  ;-)

> but this comment should not say "might". We are not going to ban const and let
> in strict mode in Firefox 4, per TC39 discussions and my call as module owner
> at this late date in the release cycle.

"might" didn't refer to implementation "probability".  It referred to let and yield not being treated as reserved *if* the script had a version which didn't treat them as keywords.  The point, then, is that *if* you're using the right script version, we don't treat them as reserved -- hence the "might".

But in hindsight "might" is definitely less clear than it should have been (I started reading your comment and thought I'd typo'd!), so changing it makes sense anyway.

(In reply to comment #8)
> I didn't get comment 6's ref to a "less-complaint patch". What would that
> patch allow? I don't think we want to unreserve "private" or other ES5
> future reserved words that we don't yet implement.

Right now we warn if we see class/enum/export/extends/import/super.  With this patch we return them to the status they had in older Firefox releases, still have in IE, and should have according to ES5.
Re: comment 9 in three parts: good English usage pls. / yeah, that was "might"-y unclear / yay!

/be
I added a bit more to the test.  In the process I discovered these changes, while good, aren't complete.  In versions which don't support let/yield (const too, I suppose), we should be treating them as though they were reserved words, not letting them slide as identifiers and whatnot.  This would seem the holy grail here -- web code would stop using yield/let as identifiers in strict mode, extension code could keep using them still as keywords, and the way would be cleared -- in web script that matters most -- to ES6 giving these keyword semantics.

So I tried hacking in the extra few lines to make this happen.  But it quickly became clear that while it wasn't going to be much complexity, it was probably enough to get another review -- so I punted it to a followup patch and landed what's here, which is desirable for the cleanups it does in any case even if it's not a full patch.

http://hg.mozilla.org/tracemonkey/rev/e0ce1fb5e566

Followup for let/yield in code not supporting them as keywords shortly...
I looked into what to do about const, but it's meaningless to add it to the list there, because const's version is JSVERSION_DEFAULT, which is 0, which is satisfied by every version out there as far as ordering goes.  So const is always valid, and the condition could never be hit, so there's no reason to clutter up the condition for it.

While I'm thinking about it, I might as well mention that I tested WebKit, and they're forbidding const in strict mode code.  So it looks like we'll be going it alone permitting const, in web code, in strict mode.
Comment on attachment 506181 [details] [diff] [review]
Make let/yield strict mode errors if the script version doesn't support let/yield

Thanks, this looks good.

Our const extension is quite old. Anyone writing portable strict code and using extensions may get burned, or may be future-proof for Harmony if they use const well. We're going to "optimize", in particular for XUL and other Mozilla-specific JS using strict.

let and yield are more important and closer to harmony than our const impl, so if this blows back we can restrict const, even in a .x release. I doubt it'll come up except among the spec-testing purity crowd.

/be
Attachment #506181 - Flags: review?(brendan) → review+
More likely and this matters in my thinking: we'll be prototyping Harmonious let and const soon enough. Forward!

/be
Landed the first part, backed out in a horrible mess of orange.  When I realized I'd forgotten browser code might use enum/class/import/etc. I should have backed out, but I decided to persevere through, stupid idea.  Will test browser heavily before relanding.  :-(
http://hg.mozilla.org/tracemonkey/rev/b21d0f75e50a
http://hg.mozilla.org/tracemonkey/rev/3d6533055424

Landed without problems this time around, after a try trial -- I must have gotten to a good point with the pushes, but for an unrelated-and-bad change.
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b11
I updated docs for this change:

https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words

...but it should be listed in "What's New" documentation as appropriate, docs whose locations and overall organization I don't know fully.
Severity: trivial → normal
Keywords: dev-doc-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
FYI:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110125 SeaMonkey/2.1b2pre

Error: enum is a reserved identifier
Source file: resource://gre/components/smileApplication.js
Line: 365, Column: 8
Source code:
    var enum = Utilities.windowMediator.getEnumerator("navigator:browser");
Blocks: 628893
I suspect this breaks automatic updates on http://www.facebook.com/?sk=lf with 
Fehler: can't redefine non-configurable property 'Rect'
Quelldatei: http://www.facebook.com/?sk=lf
Zeile: 11

and Fehler: class is a reserved identifier
Quelldatei: http://l10n.mozilla.org/~axel/nightlies/?date=2011-01-26
Zeile: 86, Spalte: 17
Quelltext:
      var _c="", class=""
Comment 21 first problem is bug 628612.

Axel: the class as var name is a bad sign. Does that page work in IE, or are you testing that only in Firefox? How about other browsers? IE has historically reserved {class, enum, extends, super}.

Waldo, I just tested Firefox 3.6 and javascript:var class=42; alert(class) works without exception. Firefox 3 era JS shell does not allow var class. Are we making a breaking change here? I had thought not.

/be
class is a reserved word in ES5, so it is rightly a syntax error.  We could change back temporarily for the release, but if so we should fix this for Firefox 5.
Also, the summary says "warning" but the patch throws errors.

/be
The summary's out of date, then -- not the first time it's happened.

So make the reserved words into strict mode reserved words and move on, perhaps?
I had only tested in fx 3.6 and 4.

I updated the code to use _class now, but I wouldn't be surprised if more niche pages would be affected.
ES5 is new and undertested, it does not trump browser-interop reality.

The point of reserving words more or differently in ES5, particularly let and yield in strict mode, is to future-proof strict code for Harmony. But non-strict code is not going to run in Harmony without opt-in and migration effort.

So we should fall back on an error in strict mode only, warning at most in non-strict. Jeff, can you file and fix fast?

/be
Attachment #507260 - Flags: review?(brendan)
Comment on attachment 507260 [details] [diff] [review]
Unreserve somewhat

Fast followup action, I like it. Leaves no TOK_RESERVED uses, I guess.

Does the test need updating?

/be
Attachment #507260 - Flags: review?(brendan) → review+
This won't need any test updates.
I inexplicably mis-thought about the test needing updating, of course it did.  Stupid is as stupid does:

http://hg.mozilla.org/tracemonkey/rev/53dea513311e
(In reply to comment #25)
> The summary's out of date, then -- not the first time it's happened.

Shouldn't it be updated then?
(In reply to comment #33)
> (In reply to comment #25)
> > The summary's out of date, then -- not the first time it's happened.
> 
> Shouldn't it be updated then?

Summary's right enough now (could mention strict mode), so no need.

/be
Doesn't hurt to update, I guess.
Summary: should produce warning when future reserved word is used as identifier in javascript → Prohibit all FutureReservedWords (whether restricted to strict mode or not) as identifiers in strict code
I'm sorry to spam the thread, but I need clarification as to which keywords have been reserved and which are reserved only under strict mode.

The patch in comment 28 seems to indicate that words like 'enum' are only reserved in strict mode, but I just noticed an add-on that was affected by this, and it doesn't appear to be running in strict mode. I also tested in the Error Console on Firefox 5.0a2, and got an error when declaring a variable called enum.

This appears to be a documentation gap for Firefox 5 compatibility and we might need to revert our automatic compatibility bump because of this.
Firefox 4 comported with the comments in this bug.  Firefox 5 goes further and makes enum and a few others keywords even in non-strict code -- see bug 637204:

http://whereswalden.com/2011/03/16/javascript-change-for-firefox-5-not-4-class-enum-export-extends-import-and-super-are-once-again-reserved-words-per-ecmascript-5/

That should give you all the detail you need, I think.
You need to log in before you can comment on or make changes to this bug.