Restrict function names to non-keywords

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete, relnote, site-compat})

unspecified
mozilla26
dev-doc-complete, relnote, site-compat
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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?
Duplicate of this bug: 908133
OK, all clear!
(Assignee)

Comment 6

5 years ago
Created attachment 794996 [details] [diff] [review]
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?
Attachment #794996 - Flags: review?(wingo)
Attachment #794996 - Flags: review?(luke)
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Comment 10

5 years ago
Created attachment 795691 [details] [diff] [review]
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.)
(Assignee)

Comment 11

5 years ago
Created attachment 795693 [details] [diff] [review]
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.
Attachment #795693 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #795693 - Flags: review? → review?(wingo)
(Assignee)

Comment 12

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Keywords: site-compat
Duplicate of this bug: 638667
You need to log in before you can comment on or make changes to this bug.