907958 - Restrict function names to non-keywords

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

This should do it:

https://tbpl.mozilla.org/?tree=Try&rev=30f17e8a6ac4
https://tbpl.mozilla.org/?tree=Try&rev=3235f703970c

Comment 1

[Jason Orendorff [:jorendorff]]

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

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

This is a dup of bug 638667. I don't think it's "too soon" anymore.

::: services/common/modules-testing/storageserver.js
@@ +111,4 @@
>      return obj;
>    },
>  
> +  delete: function delete_() {

Woof, this doesn't exactly bode well. But let's try it.

Comment 2

[Jason Orendorff [:jorendorff]]

I just realized that methods have the curious property of being built-in syntax... that creates named function objects... with keywords as their names. At least, I think this is allowed by the latest draft:

    var obj = { if() {} };
    obj.if.name
        ===> "if"

I dunno, if ES6 is allowing that, perhaps it should allow keywords as function names after `function`, which seems to me *less* likely to be confusing (or user error).

So I posted about it to es-discuss. Want to hold off a day or two landing this, and see how that is received?

Comment 4

[Jason Orendorff [:jorendorff]]

OK, all clear!

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc75fd3095eb

Comment 6

[Jeff Walden [:Waldo]]

Attachment: disallow.diff

The trivial rebase wasn't sufficient to address the havoc bug 666399 wreaked on this.  *obligatory fist-shake at the sky*

Andy, could you review this wrt yield-parsing sadnesses?  I *think* it's okay, but I could easily have made a mistake, yield-parsing being such complex sadtimes these days.  And Luke, it looks like the asm.js code for function name parsing was kind of busted too -- could you review the AsmJS.cpp changes?

Comment 7

[Jeff Walden [:Waldo]]

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

Backed out for "trivial"-rebase bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5fd513db35

Comment 8

[Andy Wingo [:wingo]]

Comment on attachment 794996 [details] [diff] [review]
disallow.diff

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

LGTM overall, with two nits (removal of checkFunctionName and validity of "yield" in asm.js).

::: js/src/frontend/Parser.cpp
@@ +2456,5 @@
>  
>      if (tt == TOK_NAME) {
>          name = tokenStream.currentName();
> +    } else if (tt == TOK_YIELD) {
> +        if (!checkYieldNameValidity())

I believe this check here and the one below in functionExpr() obviate the need for Parser::checkFunctionName().  Methods don't bind a lexical name for their PropertyName, so they appear to not need this check.

You might want to produce a different error, other than "syntax error" -- to do so pass a msg id to checkYieldNameValidity.

::: js/src/jit/AsmJS.cpp
@@ +4594,5 @@
> +    if (tt == TOK_NAME) {
> +        name = tokenStream.currentName();
> +    } else if (tt == TOK_YIELD) {
> +        if (!m.parser().checkYieldNameValidity())
> +            return false;

I guess.  Though, why allow "yield" as a name in asm.js?  You could add it to the static restrictions of the language.

Comment 9

[Luke Wagner [:luke]]

(In reply to Andy Wingo from comment #8)
> I guess.  Though, why allow "yield" as a name in asm.js?  You could add it
> to the static restrictions of the language.

Agreed

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Change a few tests to not do exact error-message checks

It's not clear to me, at second glance, why checkYieldNameValidity even takes a message number.  It's certainly the case that "missing formal parameter", when |yield| is used as an argument name, is not a very helpful message.  We'd be better off just saying "yield is a reserved identifier" everywhere.  This is easily done, just requires fixing up the tests that wrongly do exact-message checks.  This patch does that (at least as far as JS tests are concerned -- probably worth a try-run to be sure mochitests or whatever don't contain any such badnesses.)

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Remove checkYieldNameValidity's optional argument, remove checkFunctionName

I think this covers your comment, plus the extra change I think we should make.  Asking for a once-over mostly for the opportunity to state some non-obvious objection to clarifying the yield-is-a-keyword better-error opportunity, in case I missed something.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #9)
> (In reply to Andy Wingo from comment #8)
> > I guess.  Though, why allow "yield" as a name in asm.js?  You could add it
> > to the static restrictions of the language.
> 
> Agreed

Fair enough.  (But if you're going for strict-mode subset compatibility, just adding in the no-yield restriction isn't enough to do that.  And if you're going to do that, you should put in the extra effort to reject yield as a variable name, as well, which you're not, here.)

But, could we leave blacklisting yield inside asm.js code to a separate bug?  The existing asm.js semantics say nothing about this, so it requires a spec change, and I'd prefer that be a separate, asm.js-scoped bug.  And such changes would have greater scope than just this bug's scope (because of variable names, argument names, and so on) -- trying to shoehorn it all into this patch/bug seems a bad fit.

Comment 13

[Andy Wingo [:wingo]]

Comment on attachment 795693 [details] [diff] [review]
Remove checkYieldNameValidity's optional argument, remove checkFunctionName

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

LGTM.

Comment 14

[Andy Wingo [:wingo]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> (In reply to Luke Wagner [:luke] from comment #9)
> > (In reply to Andy Wingo from comment #8)
> > > I guess.  Though, why allow "yield" as a name in asm.js?  You could add it
> > > to the static restrictions of the language.
> > 
> > Agreed
> 
> Fair enough.  (But if you're going for strict-mode subset compatibility,
> just adding in the no-yield restriction isn't enough to do that.  And if
> you're going to do that, you should put in the extra effort to reject yield
> as a variable name, as well, which you're not, here.)

So, there is nothing currently on http://asmjs.org/spec/latest/ that indicates that asm.js is meant to be compatible with a "strict-mode subset".  (The page does mention that it is to be a "strict subset", but of course that's not the same thing.)

However I think there is ample precedent in http://asmjs.org/spec/latest/#syntax to disallow "yield" as a name for a module to be valid asm.js.

> But, could we leave blacklisting yield inside asm.js code to a separate bug?

Sounds good to me.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 794996 [details] [diff] [review]
disallow.diff

Fair enough, I'll remove this goo in a separate bug.

Comment 16

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2643fd47538b
https://hg.mozilla.org/integration/mozilla-inbound/rev/43503c7ca48a

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2643fd47538b
https://hg.mozilla.org/mozilla-central/rev/43503c7ca48a

Comment 18

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26/Site_Compatibility