Closed Bug 907958 Opened 11 years ago Closed 11 years ago

Restrict function names to non-keywords

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Keywords: dev-doc-complete, relnote, site-compat)

Attachments

(3 files, 1 obsolete file)

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.
Attachment #793730 - Flags: review?(jorendorff) → review+
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?
OK, all clear!
Attached patch disallow.diffSplinter Review
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?
Attachment #794996 - Flags: review?(wingo)
Attachment #794996 - Flags: review?(luke)
Comment on attachment 793730 [details] [diff] [review]
Patch

Backed out for "trivial"-rebase bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5fd513db35
Attachment #793730 - Attachment is obsolete: true
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.
Attachment #794996 - Flags: review?(wingo) → review+
(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
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.)
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.
Attachment #795693 - Flags: review?
Attachment #795693 - Flags: review? → review?(wingo)
(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 on attachment 795693 [details] [diff] [review]
Remove checkYieldNameValidity's optional argument, remove checkFunctionName

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

LGTM.
Attachment #795693 - Flags: review?(wingo) → review+
(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 on attachment 794996 [details] [diff] [review]
disallow.diff

Fair enough, I'll remove this goo in a separate bug.
Attachment #794996 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/2643fd47538b
https://hg.mozilla.org/mozilla-central/rev/43503c7ca48a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: