665835 - Remove JSOPTION_ANONFUNFIX

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

It seems JSOPTION_ANONFUNFIX, which makes |function(){}| at top level of a program or function a syntax error, and which was enabled for the browser in bug 377433, broke sometime.  This causes several test262 tests to fail, and we're the only browser to fail them for that reason (everyone else treats the syntax as an error).  Obviously we should fix that.

(I'm curious why this regression wasn't caught by any tests; we definitely have some for anonfunfix, but did they not break because those tests are shell-only?  I don't much care who/what/why this regressed.  I do much want to know why it wasn't immediately caught by our tests.)

Given that this is non-standard, that nobody else supports this, and that maintaining it (...or not) is making us buggy, I think it's time to remove JSOPTION_ANONFUNFIX entirely.  The only behavior and code to maintain (...or not) should be that dictated by the ECMAScript standard.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

19 files changed, 42 insertions(+), 128 deletions(-)

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 540782 [details] [diff] [review]
Patch

Review of attachment 540782 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/js.msg
@@ +352,4 @@
>  MSG_DEF(JSMSG_MALFORMED_ESCAPE,       270, 1, JSEXN_SYNTAXERR, "malformed {0} character escape sequence")
>  MSG_DEF(JSMSG_BAD_GENEXP_BODY,        271, 1, JSEXN_SYNTAXERR, "illegal use of {0} in generator expression")
>  MSG_DEF(JSMSG_XML_PROTO_FORBIDDEN,    272, 0, JSEXN_TYPEERR, "can't set prototype of an object to an XML value")
> +MSG_DEF(JSMSG_UNNAMED_FUNCTION_STMT,  273, 0, JSEXN_SYNTAXERR, "function expressions in statement context must be named")

I think this should read, "a function in a statement context must be named". I'm not sure whether we would prefer something more friendly (not mentioning parser context) and actionable like, "function statement requires a name"? Up to you.

::: js/src/jscntxt.h
@@ +1000,4 @@
>  static inline uintN
>  VersionFlagsToOptions(JSVersion version)
>  {
> +    uintN copts = (VersionHasXML(version) ? JSOPTION_XML : 0);

Can lose the parens.

::: js/src/jsparse.cpp
@@ +3364,1 @@
>      JSParseNode *result = pn;

|result| isn't a necessary var anymore.

@@ +3364,3 @@
>      JSParseNode *result = pn;
>      JSOp op = JSOP_NOP;
>      if (lambda != 0) {

Can you make this the more canonical |if (lambda)| while you're in here?

@@ -3372,5 @@
> -        /*
> -         * If this anonymous function definition is *not* embedded within a
> -         * larger expression, we treat it as an expression statement, not as
> -         * a function declaration -- and not as a syntax error (as ECMA-262
> -         * Edition 3 would have it).  Backward compatibility must trump all,

Flabbergast!

@@ +3368,4 @@
>          op = JSOP_LAMBDA;
>      } else if (!bodyLevel) {
>          /*
> +         * Extension: in non-strict mode code, a function expression statement

I think this should just be "a function statement" -- we're removing the "expression statement" case.

@@ +3375,2 @@
>           */
> +        JS_ASSERT(funAtom);

Is it possible to hoist this to the top of this long-n-ooglay function, as |JS_ASSERT_IF(!lambda, funAtom)|? Assertions that look like preconditions are nice.

::: js/src/tests/js1_5/extensions/jstests.list
@@ -137,4 @@
>  script regress-375183.js
>  script regress-375344.js
>  script regress-375801.js
> -script regress-376052.js

Did we lose this without rm-ing the file? Looked like you had updated it and this line still wants to be there.

Comment 3

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/57f6c41c8431

(In reply to comment #2)
> I think this should read, "a function in a statement context must be named".

WFM.

> @@ +3364,3 @@
> >      JSParseNode *result = pn;
> >      JSOp op = JSOP_NOP;
> >      if (lambda != 0) {
> 
> Can you make this the more canonical |if (lambda)| while you're in here?

I did one better -- I replaced the numeric argument with an enum argument.  This propagated to several other methods, but it's definitely nicer to read now.

> @@ +3375,2 @@
> >           */
> > +        JS_ASSERT(funAtom);
> 
> Is it possible to hoist this to the top of this long-n-ooglay function, as
> |JS_ASSERT_IF(!lambda, funAtom)|? Assertions that look like preconditions
> are nice.

Indeed -- done.

> ::: js/src/tests/js1_5/extensions/jstests.list
> @@ -137,4 @@
> >  script regress-375183.js
> >  script regress-375344.js
> >  script regress-375801.js
> > -script regress-376052.js
> 
> Did we lose this without rm-ing the file? Looked like you had updated it and
> this line still wants to be there.

The patch shows that the file was moved to a new location, with some changes made to it.  Splinter doesn't show that in UI -- not entirely surprising, I guess.

Comment 4

[Brendan Eich [:brendan]]

$ grep 'enum Fun' jsparse.h
enum FunctionSyntaxKind { Expression, Statement };
    enum FunctionType { GETTER, SETTER, GENERAL };

How about consistency, both in enumerator naming convention and location of enum (in the class or before it)?

/be

Comment 5

[Jeff Walden [:Waldo]]

I was thinking it was somewhat better to minimize patch size, maybe circle back to it later.  Anyway:

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

The enum location remains unchanged, because it's briefly used in a function outside of the Parser class, and it seemed slightly nicer to have it outside than to gunk up that function's declaration with |Parser::FunctionSyntaxKind kind|.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/57f6c41c8431
http://hg.mozilla.org/mozilla-central/rev/bf147b22f72c