Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed changes to inferred Function.name #575

Closed
wants to merge 1 commit into from

Conversation

msaboff
Copy link
Contributor

@msaboff msaboff commented May 18, 2016

Currently the spec allows for creating an inferred Function.name that is not a valid Identifier. This causes a problem with the library mathjs (see http://mathjs.org and https://github.com/josdejong/mathjs). It uses an an idiom where it created literal objects with property names with the library's type information encoded in them, e.g.: number | BigNumber | Unit. See WebKit bug ES6 Function.name inferred from property names of literal objects can break some websites for more details.

We propose that we revert to the ES5 behavior of creating a name with an empty string if the inferred name is not a valid Identifier or ReservedWord.

We have confirmed that this is not only a web compatibility issue for WebKit, but also for Chrome.

1. If _hasNameProperty_ is *false*, perform SetFunctionName(_propValue_, _propKey_).
1. If _hasNameProperty_ is *false*, then
1. Let _name_ be _propKey_.
1. If _name_ is not an |Identifier|, let _name_ be `""`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Identifier is defined as "IdentifierName but not ReservedWord", the next step is useless.

Also, I think it is more correct to say: "the StringValue of an Identifier".

@claudepache
Copy link
Contributor

if the inferred name is not a valid Identifier or ReservedWord.

You meant: "... if the inferred name is not a valid IdentifierName or is a ReservedWord"; or just: "... is not a valid Identifier".

But beside ReserveWords there are other IdentifierNames you probably want to blacklist, namely those listed in 12.1.1

@domenic
Copy link
Member

domenic commented May 18, 2016

It seems like a big shame that { catch() {}, foo() {} } would result in one nameless method and one with a name.

@claudepache
Copy link
Contributor

It seems like a big shame that { catch() {}, foo() {} } would result in one nameless method and one with a name.

Indeed, and:

class FunkySet extends Set {
    has(key) { /* ... */ } // this method would have a name...
    delete(key) { /* ... */ } // ... but the name of this method would be deleted?
}

It might be better, for consistency, to allow every IdentifierName, including reserved words.

@msaboff
Copy link
Contributor Author

msaboff commented May 18, 2016

Although the proposed changes don't show enough context, this change is for the PropertyDefinition : PropertyName : AssignmentExpression part of 12.2.6.9 Runtime Semantics: PropertyDefinitionEvaluation.

This proposed change is to handle the constructs like:

    var o = {
        ["abc 123"]: function () { },
        'c | d': function() { }
    }

@domenic
Copy link
Member

domenic commented May 18, 2016

Ah I see; sorry for not investigating further.

In that case, it sounds like we're not, in general, trying to solve the problem of (automatically-generated) func.name properties being unsuitable for eval("function " + func.name + "() { }"). Instead we're going for a targeted web-compatibility fix for mathjs's specific syntactic form. In that case, it makes the most sense to me to allow any IdentifierName, since presumably mathjs doesn't use type information that coincides with ReservedWords.

@msaboff
Copy link
Contributor Author

msaboff commented May 18, 2016

if the inferred name is not a valid Identifier or ReservedWord.

You meant: "... if the inferred name is not a valid IdentifierName or is a ReservedWord"; or just: "... is not a valid Identifier".

But beside ReserveWords there are other IdentifierNames you probably want to blacklist, namely those listed in 12.1.1

I'm fine with adjusting what is considered an acceptable inferred function name.

@littledan
Copy link
Member

Cc @ajklein

@ajklein
Copy link
Contributor

ajklein commented May 20, 2016

I noticed that mathjs has since been updated to avoid this problem (see josdejong/mathjs#601). @msaboff, are you aware of widespread deployments of mathjs that can't easily be updated?

@bterlson bterlson added the needs consensus This needs committee consensus before it can be eligible to be merged. label Jun 3, 2016
@msaboff
Copy link
Contributor Author

msaboff commented Jul 21, 2016

We checked with the submitter of the bug we had and they have updated to mathjs 3.2.1, therefore this is no longer an issue for them. We don't know of other users or JS libraries that have a problem because of this issue.

Closing this out without a change.

@msaboff msaboff closed this Jul 21, 2016
@littledan
Copy link
Member

@msaboff Thanks for following up. I'll be very interested to hear if there are any further compatibility issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants