js/src/tests/ecma_5 tests should not run as JavaScript 1.5

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
At the moment, the tests in js/src/tests/ecma_5 run as JavaScript 1.5, because the call 'version(150)' in js/src/tests/shell.js is still in effect.  It seems like 1.8 would be a more reasonable default for the ecma_5 tree.
(Assignee)

Comment 1

9 years ago
Created attachment 406735 [details] [diff] [review]
Make JavaScript 1.8 the default for js/src/tests/ecma_5.
Assignee: general → jim
Attachment #406735 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #406735 - Flags: review? → review?(jwalden+bmo)

Updated

9 years ago
Attachment #406735 - Flags: review?(jwalden+bmo) → review+
Shouldn't we be testing "the default version"? Or make an ES5 version on the number line:

 * 1.0     1.1     1.2     1.3     1.4     ECMAv3  1.5     1.6     1.7     1.8
 *         ^                       ^
 *         |                       |
 *         basis for ECMAv1        close to ECMAv2

(from jsversion.h).

/be
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> Shouldn't we be testing "the default version"? Or make an ES5 version on the
> number line:

Adding JSVERSION_ECMA_5 sounds like a good idea for users.  The whole idea of preventing new features from being present when older language versions are selected seems like a nightmare to me, though.

As far as the tests go --- my original confusion was caused by the fact that js/src/tests/shell.js doesn't go with "the default version".  The behavior that I had expected was for the whole test suite to default to the latest version, with portions of the suite that are specific to a particular version to explicitly request what they need in their own shell.js.  But this is evidently not the status quo.
(In reply to comment #3)
> (In reply to comment #2)
> > Shouldn't we be testing "the default version"? Or make an ES5 version on the
> > number line:
> 
> Adding JSVERSION_ECMA_5 sounds like a good idea for users.  The whole idea of
> preventing new features from being present when older language versions are
> selected seems like a nightmare to me, though.

We don't try to do that, it was a nightmare. Check out the cvs history for jsconfig.h some time:

http://mxr.mozilla.org/mozilla/source/js/src/jsconfig.h

We pruned back to just the post-1.5 stuff, and we should prune again. I doubt anyone uses these compile-time knobs much (JS_HAS_SCRIPT_OBJECT excluded, as you noticed that can be handy) and they are way undertested.

> As far as the tests go --- my original confusion was caused by the fact that
> js/src/tests/shell.js doesn't go with "the default version".  The behavior that
> I had expected was for the whole test suite to default to the latest version,
> with portions of the suite that are specific to a particular version to
> explicitly request what they need in their own shell.js.  But this is evidently
> not the status quo.

If I knew that, I had forgotten it. Bob, why is that?

/be

Comment 5

9 years ago
I didn't set up the ecma_5 suite, which appears to have just been cloned from an existing ecma_* suite.  If the features are available in the default version, then I can think of no reason to specify the version. 

I think the practice of setting the version originated in the past where the language behavior would change from one version to another (e.g. 1.2) and wasn't removed when we dropped that version mutation. My past practice of keeping all the branches/repos in sync may have also contributed.
(Assignee)

Comment 6

9 years ago
Thanks for the explanation, Bob.  What's the motivation for the 'version(150)' in js/src/tests/shell.js?  Could we drop that, and just have the whole test suite default to running as the latest version?

Comment 7

9 years ago
It appears to me that features are implemented identically for version(170) and version(180). Brendan, is that correct?

I am a bit concerned that the shell default version is 1.8 while the browser default version is 1.6. The browser will still require an explicit version > 160 for js1_7, js1_8, js1_8_1, ecma_3_1 and ecma_5. 

If we defaulted to 1.8 in the shell and 1.6 in the browser it would be easy for someone to drop a test in a subsuite (such as js1_5) which would run as 1.8 in the shell but fail in the browser since it would be run as 1.6.

version is called in several of the suite specific shell.js and some of the tests as well. In particular, Rhino calls it to set version(160) in e4x to override the version(150) set in the top level shell.js. Norris, what is Rhino's default version? Would it be ok with running e4x with its default version?

I think a better approach would be to default to 1.6 except for js1_7, js1_8, js1_8_1, ecma_3_1, and ecma_5 suites which should call version(180) in their respective shell.js while the top level browser.js sets version=1.8 in the mimetype for js1_7, js1_8, and js1_8_1, ecma_3_1 and ecma_5.
JSVERSION_DEFAULT is what should be used unless you need 'let' or 'yield' or any such new syntax that's not backward compatible due to (in the case of those two) new keywords (other incompatible syntactic or semantic changes could require opt-in versioning, but we hope to avoid any beyond "use strict").

Bob, where does the browser default to 1.6 while the shell defaults to 1.8? Are you talking about version=1.6 in script tags in the test harness vs. version(180) calls in the shell test framework?

/be
Whittled-down, annotated `grep JSVERSION_ *.cpp`:

jsapi.cpp:    if (version != JSVERSION_DEFAULT && version <= JSVERSION_1_4)

This is in JS_SetVersion:

    /* We no longer support 1.4 or below. */
    if (version != JSVERSION_DEFAULT && version <= JSVERSION_1_4)
        return oldVersion;

jscntxt.cpp:    JS_ASSERT(version == JSVERSION_DEFAULT || version >= JSVERSION_ECMA_3);

Same thing, in js_OnVersionChange -- no going below ES3.

jsparse.cpp:        if (JSVERSION_NUMBER(cx) != JSVERSION_ECMA_3) {

        if (JSVERSION_NUMBER(cx) != JSVERSION_ECMA_3) {
            /*
             * All legacy and extended versions must do automatic semicolon
             * insertion after do-while.  See the testcase and discussion in
             * http://bugzilla.mozilla.org/show_bug.cgi?id=238945.
             */

jsparse.cpp:                   || (JSVERSION_NUMBER(cx) == JSVERSION_1_7 &&
jsparse.cpp:                   ((JSVERSION_NUMBER(cx) == JSVERSION_1_7 &&
jsparse.cpp:                if (JSVERSION_NUMBER(cx) == JSVERSION_1_7) {
jsparse.cpp:            if (JSVERSION_NUMBER(cx) == JSVERSION_1_7) {

Bug-for-bug JS1.7 compatibility which we should break when iteration is sorted out in Harmony and implemented in SpiderMonkey, to do with key-value destructuring as a special case of 'for ([k,v] in o)' instead of just iterating over o's values and destructuring them with a 2-element array pattern.

Probably we should keep this for another JS version.

jsscan.cpp:            } else if (kw->version <= JSVERSION_NUMBER(cx)) {

This points to jskeyword.tbl, where the only non-DEFAULT-version keywords are:

#if JS_HAS_GENERATORS
JS_KEYWORD(yield,       TOK_YIELD,      JSOP_NOP,       JSVERSION_1_7)
#endif

#if JS_HAS_BLOCK_SCOPE
JS_KEYWORD(let,         TOK_LET,        JSOP_NOP,       JSVERSION_1_7)
#endif

That's it! So there is not a lot of point in changing from the default version, apart from let and yield and that key-value destructuring js1.7 bug.

/be

Comment 10

9 years ago
(In reply to comment #8)

> Bob, where does the browser default to 1.6 while the shell defaults to 1.8? Are
> you talking about version=1.6 in script tags in the test harness vs.
> version(180) calls in the shell test framework?

No, just that without any version specification on the script tag's type, the browser will default to 1.6, i.e. no let or yield.

Currently the browser.js maps the suite directory to a version v and loads the test case with a type=text/javascript;version=v
(In reply to comment #10)
> (In reply to comment #8)
> 
> > Bob, where does the browser default to 1.6 while the shell defaults to 1.8? Are
> > you talking about version=1.6 in script tags in the test harness vs.
> > version(180) calls in the shell test framework?
> 
> No, just that without any version specification on the script tag's type, the
> browser will default to 1.6, i.e. no let or yield.

The default version is not 1.6, it's "the default version" -- see

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsParserUtils.cpp#253
http://mxr.mozilla.org/mozilla-central/search?string=JSVERSION_DEFAULT
http://mxr.mozilla.org/mozilla-central/ident?i=SCRIPTVERSION_DEFAULT

JSVERSION_DEFAULT (0) is a distinct version meaning "the web-compatible union of all extensions". It has to leave out let and yield from JS1.7 but no other 1.7 changes are excluded.

> Currently the browser.js maps the suite directory to a version v and loads the
> test case with a type=text/javascript;version=v

I don't want to rock that boat right now, necessarily. Clearly 1.7 and 1.8 will need to opt in to get everything. But ES5 tests should use the default version.

/be
(In reply to comment #11)
> JSVERSION_DEFAULT (0) is a distinct version meaning "the web-compatible union
> of all extensions". It has to leave out let and yield from JS1.7 but no other
> 1.7 changes are excluded.

I forgot the for ([k,v] in o) bug compatibility issue, in addition to let and yield being keywords.

/be

Comment 13

9 years ago
Ok. Thanks. Default everywhere except js1_7, js1_8 and js1_8_1. 1.8 there? 

Nelson, can Rhino deal with the default version in e4x? If not, we can conditionally call version(160) for Rhino in e4x/shell.js.
s/Nelson/Norris/ ;-)

/be
(Assignee)

Comment 15

9 years ago
(In reply to comment #11)
> JSVERSION_DEFAULT (0) is a distinct version meaning "the web-compatible union
> of all extensions". It has to leave out let and yield from JS1.7 but no other
> 1.7 changes are excluded.

This is interesting.  I've updated the wiki.
(Assignee)

Updated

9 years ago
Depends on: 523139

Comment 16

9 years ago
Jim, I'll work up a patch this afternoon for your and brendan's approval if that is ok with you.
(In reply to comment #15)
> (In reply to comment #11)
> > JSVERSION_DEFAULT (0) is a distinct version meaning "the web-compatible union
> > of all extensions". It has to leave out let and yield from JS1.7 but no other
> > 1.7 changes are excluded.
> 
> This is interesting.  I've updated the wiki.

Link? wiki.m.o search found no JSVERSION_DEFAULT hits.

/be
(Assignee)

Comment 18

9 years ago
Created attachment 407125 [details] [diff] [review]
Use the default JavaScript version for most tests, and ECMAScript v5 for js/src/tests/ecma_5 tests.

I'd already written this; I don't know if what you had in mind was the same at all.  Feel free to pitch this if it doesn't have anything valuable.
Attachment #406735 - Attachment is obsolete: true
Attachment #407125 - Flags: review?(bclary)
(Assignee)

Comment 19

9 years ago
(In reply to comment #17)
> Link? wiki.m.o search found no JSVERSION_DEFAULT hits.

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Reference/JSVersion
(Assignee)

Comment 20

9 years ago
That is, the fact that JSVERSION_DEFAULT has that meaning deserves documentation.

Updated

9 years ago
Attachment #407125 - Attachment is obsolete: true
Attachment #407125 - Flags: review?(bclary)

Comment 21

9 years ago
Created attachment 407910 [details] [diff] [review]
patch

Use the default version for everything but ecma_3_1, ecma_5, js1_7, js1_8, js1_8_1 which use js 180. Note that bug 523139 should include the version bump to 185 for ecma_5. Note I removed the whole version thing in startTest(). I don't think it is consistent with the browser usage and the suite shell.js set the version explicitly when needed.
Attachment #407910 - Flags: review?(jim)

Updated

9 years ago
Attachment #407910 - Flags: review?(brendan)
Comment on attachment 407910 [details] [diff] [review]
patch

>+    else if (properties.test.match(/^ecma_3_1/))

ES3.1 was renamed ES5 -- should these tests be moved to match?

>+++ b/js/src/tests/ecma_3_1/shell.js
>@@ -34,14 +34,13 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> gTestsuite = 'ecma_3_1';
> 
>-// explicitly turn on js18
>+// explicitly turn on js180
> if (typeof version != 'undefined')
> {
>   version(180);

Why is 180 needed? Shouldn't this use the default version (i.e. be deleted)? If we have to use version(0) we can but it seems the suite could use the default version by default (heh) and override only once in each js_funky/shell.js for which funky requires it (funky=1_7, e.g.).

>+++ b/js/src/tests/ecma_5/shell.js
>@@ -33,8 +33,14 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> gTestsuite = 'ecma_5';
>+
>+// explicitly turn on js180
>+if (typeof version != 'undefined')
>+{
>+  version(180);

Ditto.

>+++ b/js/src/tests/js1_7/shell.js
>@@ -37,11 +37,11 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> gTestsuite = 'js1_7';
> 
> // explicitly turn on js17
> if (typeof version != 'undefined')
> {
>-  version(170);
>+  version(180);

Not sure about this. We want to test version(170) while we still have bug-for-bug compat over the for ([k,v] in o) thing. We may well want test coverage for version(180) too (both 170 and 180 get us 'let' and 'yield').

Idea: opt into 180 here as you do, but have tests of the for ([k,v] in o) form override to 170.

IIRC we have such tests but I am short on time to find them. If we don't have such tests, we should -- or even better, we should rip out the compatibility on this point (but that requires a newsgroup post and possibly even some hg.mozilla.org-hosted code auditing to make sure we don't break someone abruptly).

>+++ b/js/src/tests/js1_8_1/shell.js
>@@ -34,14 +34,14 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> gTestsuite = 'js1_8_1';
> 
>-// explicitly turn on js181
>+// explicitly turn on js180
> if (typeof version != 'undefined')
> {
>   version(180);

Is 181 not working? Runtime version tests should check order using < or <= etc.

Probably 181 is a fiction, but I wanted to check that version(181) works. We'll have version(185) for the ES5 stuff IIRC.

/be

Comment 23

9 years ago
(In reply to comment #22)
> (From update of attachment 407910 [details] [diff] [review])
> >+    else if (properties.test.match(/^ecma_3_1/))
> 
> ES3.1 was renamed ES5 -- should these tests be moved to match?
> 

can do. I'll do as a part of this bug.

> >+++ b/js/src/tests/ecma_3_1/shell.js
> >@@ -34,14 +34,13 @@
> >  * and other provisions required by the GPL or the LGPL. If you do not delete
> >  * the provisions above, a recipient may use your version of this file under
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > gTestsuite = 'ecma_3_1';
> > 
> >-// explicitly turn on js18
> >+// explicitly turn on js180
> > if (typeof version != 'undefined')
> > {
> >   version(180);
> 
> Why is 180 needed? Shouldn't this use the default version (i.e. be deleted)? If
> we have to use version(0) we can but it seems the suite could use the default
> version by default (heh) and override only once in each js_funky/shell.js for
> which funky requires it (funky=1_7, e.g.).
> 

The way I have been handling the versions relied on mapping each suite to a specific version. The suite/shell.js call to version handled the shell case while the top level browser.js would perform the mapping and create the appropriate versioned mime type for loading the test script.

One possibility is to use ecma_5/js1_7/shell.js etc. and to modify browser.js to handle the mapping for these additional directories. This seems ugly to me. Or we could just bite the bullet and create a browser version of version().

I don't have any skin in this, so rely on what you and the others think is best. But whatever choice you make *must* work for both the shell and the browser.

I had considered the version 180 here and in ecma_5 below, to be placeholders until 185 was activated.


> >+++ b/js/src/tests/js1_7/shell.js
> >@@ -37,11 +37,11 @@
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > gTestsuite = 'js1_7';
> > 
> > // explicitly turn on js17
> > if (typeof version != 'undefined')
> > {
> >-  version(170);
> >+  version(180);
> 
> Not sure about this. We want to test version(170) while we still have
> bug-for-bug compat over the for ([k,v] in o) thing. We may well want test
> coverage for version(180) too (both 170 and 180 get us 'let' and 'yield').
> 
> Idea: opt into 180 here as you do, but have tests of the for ([k,v] in o) form
> override to 170.

Maybe just sticking with 170 for js1_7 is best. We can disallow adding new tests here.


> >+++ b/js/src/tests/js1_8_1/shell.js
> >@@ -34,14 +34,14 @@
> >  * and other provisions required by the GPL or the LGPL. If you do not delete
> >  * the provisions above, a recipient may use your version of this file under
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > gTestsuite = 'js1_8_1';
> > 
> >-// explicitly turn on js181
> >+// explicitly turn on js180
> > if (typeof version != 'undefined')
> > {
> >   version(180);
> 
> Is 181 not working? Runtime version tests should check order using < or <= etc.

version=1.8.1 doesn't work in the browser. I'm embarrassed to ask how would I tell in the shell? I can put anything in a  js -v <ver> or a call to version and it doesn't complain. 

> 
> Probably 181 is a fiction, but I wanted to check that version(181) works. We'll
> have version(185) for the ES5 stuff IIRC.
> 
> /be
(In reply to comment #23)
> The way I have been handling the versions relied on mapping each suite to a
> specific version.

This is ok so long as you can set the default version (for most of the subdirs this is the right thing).

> Maybe just sticking with 170 for js1_7 is best. We can disallow adding new
> tests here.

+1

> version=1.8.1 doesn't work in the browser. I'm embarrassed to ask how would I
> tell in the shell?

version(181)

> I can put anything in a  js -v <ver> or a call to version and it doesn't complain.

That is enough. There are no runtime dependencies, so it is really only a matter of whether the <script type="application/javascript;version=1.8.1"> tag works. If anyone cares we could have a bug, but I think we should just let this sleeping dog lie.

/be

Comment 25

9 years ago
(In reply to comment #24)
> (In reply to comment #23)
> > The way I have been handling the versions relied on mapping each suite to a
> > specific version.
> 
> This is ok so long as you can set the default version (for most of the subdirs
> this is the right thing).
> 
> > Maybe just sticking with 170 for js1_7 is best. We can disallow adding new
> > tests here.
> 
> +1
> 
> > version=1.8.1 doesn't work in the browser. I'm embarrassed to ask how would I
> > tell in the shell?
> 
> version(181)
> 


> > I can put anything in a  js -v <ver> or a call to version and it doesn't complain.

Oh, ok.

js> version(2984240298342)
180
js> version()              
180
js> version(181)  
180
js> version()    
181

> 
> That is enough. There are no runtime dependencies, so it is really only a
> matter of whether the <script type="application/javascript;version=1.8.1"> tag
> works. If anyone cares we could have a bug, but I think we should just let this
> sleeping dog lie.

If there is no difference, I can use version(181) for the shell and<script type="application/javascript;version=1.8"> for the browser in js1_8_1.

I will need <script type="application/javascript;version=1.8.5"> for ecma_5 though, wont' I ?
(In reply to comment #25)
> I will need <script type="application/javascript;version=1.8.5"> for ecma_5
> though, wont' I ?

No, because ES5 is ES3-compatible (no new syntax, for us), and "use strict" is an opt-in we think no real code uses.

The great thing about versioning is not doing it. A browser that actually parses ;version= per RFC 4329 could be targeted with ES5- or JS1.8.5-only content, but IE ignores version (violating the RFC, boo hiss) and anyway what do you do for the downrev browsers? <script> has no alternative-content fallback syntax.

So the less versioning the better, for our tests as for web devs!

/be
(Assignee)

Comment 27

9 years ago
(In reply to comment #23)
> I had considered the version 180 here and in ecma_5 below, to be placeholders
> until 185 was activated.

185 is activated in Tracemonkey now: bug 523139 landed.

Updated

9 years ago
Attachment #407910 - Flags: review?(jim)
Attachment #407910 - Flags: review?(brendan)
Attachment #407910 - Flags: review-

Comment 28

9 years ago
Created attachment 409557 [details] [diff] [review]
patch

add new suite for version 1.8.5

shell
use default version(0) for everyone except the following:
use version(170) for js1_7
use version(180) for js1_8
use version(181) for js1_8_1
use version(185) for js1_8_5

browser
use default version for everyone except the following:
use text/javascript;version=1.8 for js1_7, js1_8, js1_8_1, js1_8_5
Attachment #409557 - Flags: review?(brendan)

Comment 29

9 years ago
PS. I'll wait to move ecma_3_1 until after dmandelin lands bug 509629.
Attachment #409557 - Flags: review?(brendan) → review+

Comment 30

9 years ago
http://hg.mozilla.org/mozilla-central/rev/c4e267407908
http://hg.mozilla.org/tracemonkey/rev/a0b2107abac9
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-tracemonkey]
You need to log in before you can comment on or make changes to this bug.