351515 - "yield" can no longer be used as an argument name (Mercury News page not rendered properly for FF2)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tim Riley [:timr]]

The Mercury News events page does not render events properly.  This is a regression 1.5.0.6 to FF 2.0 Beta2.  I'll try to get a better regression window.  Or can someone else help with this?

STR:
1) Go to the URL above
2) Enter "concert" in the "What" field
3) Select "Search"

A "List" tab appears on page. But no events are listed below it.

Expected result:
A list of 10 concerts in the bay area.

This works fine for Safari and FF1.5.0.6: 
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

Fails for FF2 Beta 2:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Their page uses JavaScript obfuscation, but what it boils down to is that they use a function with a parameter named "yield", and we know throw a "SyntaxError: missing formal parameter" since yield is now a keyword used for something else (generators).

Here's the code that fails:
Z.ready=function(id,yield){
  var interval=setInterval(function(){
    var e=document.getElementById(id);
    if(e) {
      clearInterval(interval);
      yield(e);
    }
  },100);
};

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This should block Fx2, I claim.

Comment 3

[Igor Bukanov]

A possible workaround can be to disable the special meaning of "let" and "yield" when the parser detects them as argument name or in var declarations while printing a warning.   

Comment 4

[Igor Bukanov]

Attachment: Proof-on-concept

This is a proof of concept that allows to use yield or let as argument name. With such usage a warning is used and in the rest of script yield or let is treated as ordinary names. I will extend the patch to cover var declarations and using yield outside of function.

Comment 5

[Igor Bukanov]

Attachment: Implementation v1

The patches switches the parser to ignore JS1.7-only yield and let keywords in the following cases:

1. let or yield are used as argument name:
js> function f(yield, let) { return yield+let; }
typein:2: strict warning: using JavaScript 1.7 yield keyword when name is expected:
typein:2: strict warning: function f(yield, let) { return yield+let; }
typein:2: strict warning: ...........^

2. let or yield are used as var name:
js> var let = 1;
typein:3: strict warning: using JavaScript 1.7 let keyword when name is expected:
typein:3: strict warning: var let = 1;
typein:3: strict warning: ....^

3. yield is used outside function:
js> yield=1
typein:4: strict warning: using JavaScript 1.7 yield keyword when name is expected:
typein:4: strict warning: yield=1
typein:4: strict warning: ^
typein:4: strict warning: assignment to undeclared variable yield

4. When let or yield are used as function name:
js> function yield() {}
typein:2: strict warning: using JavaScript 1.7 yield keyword when name is expected:
typein:2: strict warning: function yield() {}
typein:2: strict warning: .........^

Note that this would be unnecessary if bug 343675 would be addressed.

For simplicity the patch switches to such compatibility mode even if let or yield were previously used correctly. Is this desirable?

Comment 6

[Igor Bukanov]

I will fix the bug 343675 first as the end result is less code.

Comment 7

[Igor Bukanov]

Comment on attachment 236995 [details] [diff] [review]
Implementation v1

The patch requires some work after fixing bug 343675.

Comment 8

[Igor Bukanov]

Attachment: Implementation v2

This a patch that takes into account bug 343675.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 236998 [details] [diff] [review]
Implementation v2


>+static JSBool
>+TurnMisusedKeywordToName(JSContext *cx, JSTokenStream *ts, uintN errorNumber)

s/To/Into/ -- or perhaps UnreserveKeyword for shorter lines below?

>@@ -3675,18 +3719,24 @@ AssignExpr(JSContext *cx, JSTokenStream 
>     JSTokenType tt;
>     JSOp op;
> 
>     CHECK_RECURSION();
> 
> #if JS_HAS_GENERATORS
>     ts->flags |= TSF_OPERAND;
>     if (js_MatchToken(cx, ts, TOK_YIELD)) {
>-        ts->flags &= ~TSF_OPERAND;
>-        return ReturnOrYield(cx, ts, tc, AssignExpr);
>+        if (tc->flags & TCF_IN_FUNCTION) {
>+            ts->flags &= ~TSF_OPERAND;
>+            return ReturnOrYield(cx, ts, tc, AssignExpr);
>+        } else if (!TurnMisusedKeywordToName(cx, ts,

Lose the else-after-return non-sequitur.

>+/* Treat let and yield JS1.7 keywords as TOK_NAME */
>+#define TSF_KEYWORD17_IS_NAME 0x8000
>+

Let's call this something future-proof, since we will need to use it all the way to JS2/ES4.

How about TSF_ES4_KEYWORD_IS_NAME?

This patch reminds me: I wish I'd avoided all the ts->flags |= TSF_FOO;...; ts->flags &= ~TSF_FOO in the old days and made the js_*Token functions take a flags param and do it themselves.  Worth a followup bug?

About your earlier question: if a single compilation unit uses let or yield correctly per JS1.7 but later loses its mind, we should complain.  Alternatively, we should find a way not to disable yield and let even when used as formal parameter or local variable names.  In today's ECMA TG1 conference call, people worried about the complexity of keeping per function or narrower lexical scope state on whether let or yield had been overridden.  I say we should smack any code that mixes meaning of let or yield within a compilation unit with a big error.  Is this easy to implement?

/be

Comment 10

[Igor Bukanov]

Attachment: Implementation v3

This version either allow to use ES4 features or disallow them globally per compilation unit.

In particular, using destructing assignments or correct usage of yield and let first time means that yield and let are always interpreted as ES4 keywords. On the other hand, if the first occurrence of yield or let was in name context, then no yield, name or destruction assignment is allowed. 

The patch uses a new state variable in JSTokenStream as ts->flags are not global and can be restored to some older value after parsing subtree.

Comment 11

[Igor Bukanov]

Here is a behavior with the patch:

js> var let; [a,b] = "a,b".split(",");
typein:1: strict warning: using EcmaScript 4.0 let keyword in a name context:
typein:1: strict warning: var let; [a,b] = "a,b".split(",");
typein:1: strict warning: ....^
typein:1: SyntaxError: using EcmaScript 4.0 feature after misusing ES4 keyword:
typein:1: var let; [a,b] = "a,b".split(",");
typein:1: ...............^

js> var  [a,b] = "a,b".split(","), let;
typein:2: SyntaxError: missing variable name:
typein:2: var  [a,b] = "a,b".split(","), let;
typein:2: ...............................^

Comment 12

[Igor Bukanov]

(In reply to comment #9)
> In today's ECMA TG1 conference
> call, people worried about the complexity of keeping per function or narrower
> lexical scope state on whether let or yield had been overridden.  I say we
> should smack any code that mixes meaning of let or yield within a compilation
> unit with a big error.  Is this easy to implement?

But why is the compatibility started to worry ECMA TG1 members now, not when let and yield keywords with the current syntax were added? I.e. either one accepts that adding new keywords leads to incompatibility with the current scripts that use the keywords as names. Or one designs new features with backward compatibility in mind.

For example, let and yield can still be keywords, but it should be always possible to tell if a particular usage can happen only with pre-JS1.7 scripts and switch to the compatibility mode for the whole compilation unit. But of cause, it is too late to fix the damage, hence a hack like proposal in this bug.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #9)
> > In today's ECMA TG1 conference
> > call, people worried about the complexity of keeping per function or narrower
> > lexical scope state on whether let or yield had been overridden.  I say we
> > should smack any code that mixes meaning of let or yield within a compilation
> > unit with a big error.  Is this easy to implement?
> 
> But why is the compatibility started to worry ECMA TG1 members now, not when
> let and yield keywords with the current syntax were added?

We worry about compatibility all the time.

Our intention is to evolve the language, which means adding new identifiers that have meaning in certain contexts.  Whether that requires brute force versioning by authors (type="application/ecmascript; version=4" or older ways, none of which currently work in all browsers), user-agent sniffing by servers, or ideally better have-your-cake-and-eat-it approaches such as the ones you've implemented in js1.7, we did not want to prejudge.

But we are not just going to leave things alone.  So in adding contextual or reserved keywords, we have considered this exact problem.  Doug Crockford, representing Yahoo! in TG1, urged that all keywords be unreserved.  But this allows for nonsense sentences.  The compromise you've helped prove allows keywords in property name contexts, and now in function name contexts, and with this bug's fix, in variable and argument names (but at the price of not using the new keywords).

> I.e. either one
> accepts that adding new keywords leads to incompatibility with the current
> scripts that use the keywords as names.

This won't work.

> Or one designs new features with
> backward compatibility in mind.

This is what we are doing.

> For example, let and yield can still be keywords, but it should be always
> possible to tell if a particular usage can happen only with pre-JS1.7 scripts
> and switch to the compatibility mode for the whole compilation unit.

How would you do this?  It's not just that we didn't do it (whatever it is) for JS1.7.  Wouldn't this require a time machine?  I.e., if we had a from __future__ import foo or similar way of announcing new syntax.  I should have done that 11 years ago.

> But of
> cause, it is too late to fix the damage, hence a hack like proposal in this
> bug.

It's not too late for JS1.7, but I am not sure if you are just lamenting the lack of foresight in original JS design.  It's ok, I lament too -- but only a bit, and then move on ;-).  The current situation has the advantage of being (a) reality; (b) incrementally better than doing nothing.

/be

Comment 14

[Brendan Eich [:brendan]]

One suggestion from Dave Herman that we've discussed in TG1: requiring 'use let' or 'use yield' pragma before use of new keyword.  But this (a) chokes all existing implementations; (b) is verbose when we want the default to be concise.

Something more auto-magic is needed, a la Doug Crockford's suggestion, but with reservation of new identifiers when used in syntactic contexts where they must be keywords, not random user-defined identifiers.  For JS1.7, with this patch, I think we are done.  Our results feed directly back into the standard body, which is helpful.

/be

Comment 15

[Igor Bukanov]

(In reply to comment #13)
> > For example, let and yield can still be keywords, but it should be always
> > possible to tell if a particular usage can happen only with pre-JS1.7 scripts
> > and switch to the compatibility mode for the whole compilation unit.
> 
> How would you do this?  It's not just that we didn't do it (whatever it is) for
> JS1.7.  

Simple alternative syntax suggestions after 10 minutes of considerations:

For generators require to use [], not () around argument list. Then any use of yield outside the generator triggers the compatibility mode. The "let" case is harder due to destructing assignment, still using something like var* instead of let could work.

I.e. if the compatibility is really taken seriously, then the new keyword-like features would be introduced on case/by case basis or at least scripts would be  required to use a special statement like use in Perl. Since this is apparently not the case, my conclusion is that compatibility was not that hard requirement. 

Comment 16

[Brendan Eich [:brendan]]

Compatibility is not the only good to consider.  If we have to use ugly new syntax or require verbose opt-in pragmas, users won't use the new features.  Taste in how to name things aside, I hope you will agree that compatibility can't trump all other considerations.

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 237113 [details] [diff] [review]
Implementation v3

Quick comments, have to head into work:

s/EcmaScript 4.0/EcmaScript 4/ (there's a long story here about Ecma changing its name from ECMA, but not changing the spelling of ECMAScript.  But ECMAScript is too loud and ugly, so the only nit here is 4 for the fourth edition, no .0).

CheckCanDestruct is more consistent, but any content using let or yield as a name that wanted to run in ES3 implementations will suffer syntax or runtime errors using destructuring assignment.  This session uses a MOZILLA_1_8_0_BRANCH shell:

js> function f([a, b]){}
typein:1: SyntaxError: missing formal parameter:
typein:1: function f([a, b]){}
typein:1: ...........^
js> function f(){let [a,b] = c()}
js> function f(){var [a,b] = c()}
typein:3: SyntaxError: missing variable name:
typein:3: function f(){var [a,b] = c()}
typein:3: .................^

etc.  The let "array" used in the second function could even be created, no problem, of course, in old code.

So wouldn't it be better to leave destructuring syntax out of this keyword unreserving patch?

/be

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> CheckCanDestruct is more consistent, but any content using let or yield as a
> name that wanted to run in ES3 implementations will suffer syntax or runtime
> errors using destructuring assignment.

I meant to write "will *probably* suffer ... errors".

> js> function f(){let [a,b] = c()}
...
> etc.  The let "array" used in the second function could even be created, no
> problem, of course, in old code.

So disabling let's reserved status should suffice, no need to disable destructuring assignment.

/be

Comment 19

[Brendan Eich [:brendan]]

It probably was not clear, but TG1 is not done with ES4 spec, and the feedback from implementing new features is considered a crucial part of standardization (unlike with certain notorious other standards from other bodies).

Since backward compatibility is not absolute (it can't be given lack of bug-for-bug compatibility among existing ES3 implementations), the new syntax fits the style of the language (let instead of var, as a "better var"; yield follows return and Python precedent; destructuring builds on array destructuring from Opera 8).

In the worst case, this means new code will have to opt in using a version declaration outside of the language (via MIME type version= param, e.g.), but we hope to avoid that, because people forget to use it when they should, and use it when they shouldn't.  This has been a problem since Netscape 3.

If we have erred in not making a harder requirement for compatibility, I will take blame first.  We are pretty committed at this point to "compatible-looking syntax extensions" that need to be "compatibly implemented" by unreserving keywords.  This result will feed back usefully into ECMA TG1.  Igor, are you ok with this?

/be

Comment 20

[Igor Bukanov]

(In reply to comment #19)
> If we have erred in not making a harder requirement for compatibility, I will
> take blame first.  We are pretty committed at this point to "compatible-looking
> syntax extensions" that need to be "compatibly implemented" by unreserving
> keywords. 

I am not arguing for or against let and yield keywords. I just want to know where compatibility requirements stops. Until yesterday I though that incompatibilities added by new keywords are OK and it is the fact of life that old scripts would not be supported. Which is a clear position. But marking this bug as FF2.0 blocker reveals that this is not the case. So I am curious where this stops? I.e if somebody reported that a particular big/important site uses:

function a() {
    yield(10);
}

function yield(n) {
    setTimeout(a, n);
}

will this also be FF2 blocker? Or what about this:

function a() {
    return let(a = 10)[a];
}

function let(n) {
    return someArrayInstance;
} 

> Igor, are you ok with this?

I would be OK with any clear decision. Without it spending time on this bug is just waste of time. In fact I realized today after considering the whole issue that committing 343675 was a mistake as it made both the above examples to compile while drastically changing the semantic.

Comment 21

[Igor Bukanov]

Here is in short why I do not like the way let and yield are added. It is possible that the current code that use them as names can compile with radically changed semantics. And this is the worts case of most "compatible extension" IMO.

Comment 22

[Brendan Eich [:brendan]]

That's a good point (I hadn't seen it for let until the let [a, b] = ... example; it's obvious with yield).

We have an old and safe way of dealing with this: make these keywords be JSVERSION_1_7 in jskeyword.tbl.  Require all scripts wanting to use them to opt in via <script type="application/javascript;version=1.7"> or similar.

Bob, would that work?  We ignore the version suffix for language="JavaScript1.7" to match other browsers, but type with version param should work for us.

/be

Comment 23

[Igor Bukanov]

(In reply to comment #22)
> We have an old and safe way of dealing with this: make these keywords be
> JSVERSION_1_7 in jskeyword.tbl.  Require all scripts wanting to use them to opt
> in via <script type="application/javascript;version=1.7"> or similar.

But it would be really nice to be able to declare that from the script itself so it still can be included like before  <script src="sss"></script>...

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> (In reply to comment #22)
> > We have an old and safe way of dealing with this: make these keywords be
> > JSVERSION_1_7 in jskeyword.tbl.  Require all scripts wanting to use them to opt
> > in via <script type="application/javascript;version=1.7"> or similar.
> 
> But it would be really nice to be able to declare that from the script itself
> so it still can be included like before  <script src="sss"></script>...

In such a case, you would want downrev browsers to choke.  So Dave's 'use yield' idea comes to mind.  We do not have the 'use' pragma syntax in JS1.7, though, and it is late to be adding it.

/be

Comment 25

[Bob Clary [:bc] (inactive)]

(In reply to comment #22)

> in via <script type="application/javascript;version=1.7"> or similar.
> 
> Bob, would that work?  We ignore the version suffix for
> language="JavaScript1.7" to match other browsers, but type with version param
> should work for us.

I use this already in the browser based tests with type="text/javascript;version=1.7" to prevent 1.8.0 and earlier from executing the scripts.

default language/version: (executes in 1.8.0 and fails, and executes and passes in 1.8, 1.9)
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/block/regress-341939.js;language=language;javascript>

javascript/1.7:(does not execute in 1.8.0, but does in 1.8, 1.9)
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/block/regress-341939.js;language=type;text/javascript;version=1.7>

I haven't done a keyword scan for yield, let etc in scripts. Would that be useful?

Comment 26

[Igor Bukanov]

Attachment: Implementation v4

This just declares yield and let to bet JS17-only and patches the keyword check in jsscan.c to really depend on the version instead of returning true for all supported versions. 

Still it is a pity that yield has to be out. If generators would be marked explicitly like in:

for function gen(a, b) {
...
}

or 
function gen[a, b] {
...
}

then the yield could stay with JSVERSION_DEFAULT as the engine can simply warn about its usage outside the generator. Plus this would solve that stupidity about writing empty generator as:

function empty() {
    if (false) yield;
}

where the semantic depends on the dead code :(

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 237309 [details] [diff] [review]
Implementation v4

Yes, this is a set-back.  ECMA TG1 will hear about it and do something, if I can help it.

Nevertheless, for js1.7/firefox2 this looks like the best way to go.

/be

Comment 28

[Igor Bukanov]

I committed the patch from comment 26 to the trunk:

Checking in jskeyword.tbl;
/cvsroot/mozilla/js/src/jskeyword.tbl,v  <--  jskeyword.tbl
new revision: 3.8; previous revision: 3.7
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.107; previous revision: 3.106
done

Comment 29

[Mike Schroepfer]

Comment on attachment 237309 [details] [diff] [review]
Implementation v4

a=schrep for drivers.

Comment 30

[Bob Clary [:bc] (inactive)]

explicitly turn js17 on in js1_7/shell.js

Checking in shell.js;
/cvsroot/mozilla/js/tests/js1_7/shell.js,v  <--  shell.js
new revision: 1.2; previous revision: 1.1

Comment 31

[Bob Clary [:bc] (inactive)]

Checking in js1_7/shell.js;
/cvsroot/mozilla/js/tests/js1_7/shell.js,v  <--  shell.js
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-351515.js,v
done
Checking in js1_5/Regress/regress-351515.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-351515.js,v  <--  regress-351515.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/lexical/regress-351515.js,v
done
Checking in js1_7/lexical/regress-351515.js;
/cvsroot/mozilla/js/tests/js1_7/lexical/regress-351515.js,v  <--  regress-351515.js
initial revision: 1.1

the js1_7/shell.js checkin included a test to prevent version() being called in the browser. the function yield() in js1_7/lexical/regress-351515.js passes due to bug 343675 I presume.

Comment 32

[Igor Bukanov]

I committed the patch from comment 26 to MOZILLA_1_8_BRANCH:

Checking in jskeyword.tbl;
/cvsroot/mozilla/js/src/jskeyword.tbl,v  <--  jskeyword.tbl
new revision: 3.7.2.2; previous revision: 3.7.2.1
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.81.2.19; previous revision: 3.81.2.18
done

Comment 33

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060909 windows/mac*/linux

Comment 34

[Bob Clary [:bc] (inactive)]

1.8 2006091022 windows/mac*/linux
./js1_7/lexical/regress-351515.js:118: SyntaxError: missing ( before formal parameters: ./js1_7/lexical/regress-351515.js:118

bug 343675 needs 1.8 checkin.

Comment 35

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux

Comment 36

[Bob Clary [:bc] (inactive)]

inside of test(), the  function yield() {} is causing a syntax error. Is that correct?

Comment 37

[Bob Clary [:bc] (inactive)]

regression range <http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fjs%2Fsrc&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-17+18%3A00%3A00&maxdate=2007-05-17+21%3A00%3A00&cvsroot=%2Fcvsroot>

Looks like bug 380237