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

ES2017: allowing async method named "function" is future hostile #832

Closed
allenwb opened this Issue Mar 2, 2017 · 14 comments

Comments

Projects
None yet
@allenwb
Member

allenwb commented Mar 2, 2017

Summary:

The ES2017 draft currently allows this:

class {
   async function() {}; //this creates an property named "function" whose value is an async function object;
}

This is future hostile because it creates issues if we every want to add lexically scoped declarations to class bodies. It would be more future proof to disallow (via a look-ahead restriction) function in the PropertyName position of AsyncMethod . It would still be possible to define an async method named "function" by quoting the property name.

We should add the look-ahead restriction in the final version of the ES2017 spec.

TL;DR

The possibility of adding lexically scoped declarations to class bodies have long been discussed and is under active discussion, particularly as a complement to private fields. However, consider the following:

class {
  async function() {}   //ES2017 draft says this is an async method named "function"
  async function helper() {}  //in the future we might want this to be class-scoped async function declaration
}

The problem is that in parsing a ClassElement whose first token is async we can't distinguish AsyncMethod and AsyncFunctionDeclaration using single token look-ahead.

We should future-proof ES2017 to allow for the future possibility of such declarations. This can be accomplished by changing the definition of AsyncMethod to:

AsyncMethod[Yield, Await] :
   async [no LineTerminator here] [lookahead ∉ {function}] PropertyName[?Yield, ?Await] ( UniqueFormalFarameters[~Yield, +Await] ) { AsyncFunctionBody }

Web compatibility considerations.

Most browser are already shipping async methods. Can we get away with making this change at this late data? Probably. It's seems likely that very few (if any) websites have yet defined async methods named "function". But we will never know if we don't try to make this change. If we do make this change in the final spec. one or more browser will test it in preview release. If no compatibility problems are found, we can expect that the change will be widely adopted. If compatibility problems are found the change wouldn't be adopted by browsers and we can remove it in ES2018. Changing it now in the ES2017 spec. is probably the only opportunity we will have to do this future proofing.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Mar 2, 2017

Member

This makes sense, but I don't see any reason to rush it into the ES2017 spec. We should wait for an implementation to try this out in the field and report back, commit test262 tests etc. per the usual process for normative changes, before changing anything.

Member

domenic commented Mar 2, 2017

This makes sense, but I don't see any reason to rush it into the ES2017 spec. We should wait for an implementation to try this out in the field and report back, commit test262 tests etc. per the usual process for normative changes, before changing anything.

littledan added a commit to littledan/ecma262 that referenced this issue Apr 11, 2017

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Apr 11, 2017

Member

In the interest of helping out any implementation that might be interested, I wrote up spec text and test262 tests for this change, which you can find linked above.

(Parenthetical: I like to think that annual version cutoffs are a red herring, but I'm sympathetic to Allen's 'ASAP' feeling for compat reasons)

((Double parenthetical: For engines which want to optimize parsing performance, it seems like looking ahead as opposed to backtracking has worse performance typically. This may affect the applicability of the grammar invariants. Nevertheless, JavaScript VMs aren't the only parsers, and we all have a strong interest in keeping the grammar manageable.))

Member

littledan commented Apr 11, 2017

In the interest of helping out any implementation that might be interested, I wrote up spec text and test262 tests for this change, which you can find linked above.

(Parenthetical: I like to think that annual version cutoffs are a red herring, but I'm sympathetic to Allen's 'ASAP' feeling for compat reasons)

((Double parenthetical: For engines which want to optimize parsing performance, it seems like looking ahead as opposed to backtracking has worse performance typically. This may affect the applicability of the grammar invariants. Nevertheless, JavaScript VMs aren't the only parsers, and we all have a strong interest in keeping the grammar manageable.))

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 11, 2017

Member

we need consensus for this, right?

Member

leobalter commented Apr 11, 2017

we need consensus for this, right?

@Jamesernator

This comment has been minimized.

Show comment
Hide comment
@Jamesernator

Jamesernator Apr 21, 2017

I'd like to see evidence that this would actually make parsing better and not worse before I'd be willing to support it.

Perhaps someone can prove me wrong but I'd definitely expect parsing to be more expensive not less. Essentially all I see this doing in the short term is meaning every single async method will need to be inspected to see if its name is function. And even if async function declarations were possible in the class body surely async function() {} methods are going to be exceedingly rare that backtracking will basically be negligible?

Jamesernator commented Apr 21, 2017

I'd like to see evidence that this would actually make parsing better and not worse before I'd be willing to support it.

Perhaps someone can prove me wrong but I'd definitely expect parsing to be more expensive not less. Essentially all I see this doing in the short term is meaning every single async method will need to be inspected to see if its name is function. And even if async function declarations were possible in the class body surely async function() {} methods are going to be exceedingly rare that backtracking will basically be negligible?

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 21, 2017

Contributor

@Jamesernator, I don't think the real-life time to parse the names of methods is really a concern; it's going to be totally negligible either way. This is more to do with the grammar being unambiguous with only a single token of lookahead, which is a constraint which makes it easier to reason about and build parsers for.

Contributor

bakkot commented Apr 21, 2017

@Jamesernator, I don't think the real-life time to parse the names of methods is really a concern; it's going to be totally negligible either way. This is more to do with the grammar being unambiguous with only a single token of lookahead, which is a constraint which makes it easier to reason about and build parsers for.

@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Apr 21, 2017

@Jamesernator @bakkot

@bakkot is correct that the concern has nothing to do with parsing speed. But it has nothing to do with lookahead either, or parsing difficulty in any sense. The concern is that the current spec allows the syntax @allenwb showed in the first message in this thread

class {
   async function() {}; //this creates an property named "function" whose value is an async function object;
}

but gives it a different meaning than we'd like. If implementations allow this, with this "wrong" meaning, and if code on the web makes use of that, this may preclude the future change Allen explains because it would break that previous-conforming code.

erights commented Apr 21, 2017

@Jamesernator @bakkot

@bakkot is correct that the concern has nothing to do with parsing speed. But it has nothing to do with lookahead either, or parsing difficulty in any sense. The concern is that the current spec allows the syntax @allenwb showed in the first message in this thread

class {
   async function() {}; //this creates an property named "function" whose value is an async function object;
}

but gives it a different meaning than we'd like. If implementations allow this, with this "wrong" meaning, and if code on the web makes use of that, this may preclude the future change Allen explains because it would break that previous-conforming code.

@Jamesernator

This comment has been minimized.

Show comment
Hide comment
@Jamesernator

Jamesernator Apr 22, 2017

@erights Are you sure it's not to do with lookahead? Because Allen specifically mentions

The problem is that in parsing a ClassElement whose first token is async we can't distinguish AsyncMethod and AsyncFunctionDeclaration using single token look-ahead.

If class level declarations are added then it's easy to distinguish async function <somename>() (declaration) from async function() (method called function), with absolutely no ambiguity, just by inspecting the first token after the function token but this is naturally a two token lookahead.

Jamesernator commented Apr 22, 2017

@erights Are you sure it's not to do with lookahead? Because Allen specifically mentions

The problem is that in parsing a ClassElement whose first token is async we can't distinguish AsyncMethod and AsyncFunctionDeclaration using single token look-ahead.

If class level declarations are added then it's easy to distinguish async function <somename>() (declaration) from async function() (method called function), with absolutely no ambiguity, just by inspecting the first token after the function token but this is naturally a two token lookahead.

@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Apr 22, 2017

@Jamesernator My apologies. Rereading @allenwb's message, I think you are correct. @allenwb , in that case, I don't get it. Is the issue only avoiding the burden of the extra lookahead?

Attn @waldemarhorwat

erights commented Apr 22, 2017

@Jamesernator My apologies. Rereading @allenwb's message, I think you are correct. @allenwb , in that case, I don't get it. Is the issue only avoiding the burden of the extra lookahead?

Attn @waldemarhorwat

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows May 5, 2017

Just a thought: isn't the idea of in-class lexical declarations practically equivalent to private static methods/properties?

isiahmeadows commented May 5, 2017

Just a thought: isn't the idea of in-class lexical declarations practically equivalent to private static methods/properties?

@lukescott

This comment has been minimized.

Show comment
Hide comment
@lukescott

lukescott May 5, 2017

This is valid ES2016 syntax (currently works in Chrome):

class Foo {
  function() {} // <-- named "function"
}

If async function() {} becomes invalid, does function() {} become invalid as well?

lukescott commented May 5, 2017

This is valid ES2016 syntax (currently works in Chrome):

class Foo {
  function() {} // <-- named "function"
}

If async function() {} becomes invalid, does function() {} become invalid as well?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan May 6, 2017

Member

@lukescott In the PR I wrote, no. function() {} doesn't lead to the same two-token lookahead issue, so it doesn't need the prohibition as strongly.

Member

littledan commented May 6, 2017

@lukescott In the PR I wrote, no. function() {} doesn't lead to the same two-token lookahead issue, so it doesn't need the prohibition as strongly.

@gskachkov

This comment has been minimized.

Show comment
Hide comment
@gskachkov

gskachkov May 29, 2017

@littledan
Are following cases invalid?

//1
class Foo {
    async 'function'() {}   
}
//2
class Boo {
    async ['function']() {}   
}
//3
var O = {async ['function']() {} }

gskachkov commented May 29, 2017

@littledan
Are following cases invalid?

//1
class Foo {
    async 'function'() {}   
}
//2
class Boo {
    async ['function']() {}   
}
//3
var O = {async ['function']() {} }
@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jun 13, 2017

Member

Wouldn't the first one be invalid regardless?

Member

ljharb commented Jun 13, 2017

Wouldn't the first one be invalid regardless?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jun 13, 2017

Member

At the May 2017 TC39 meeting, we decided that the grammar isn't too bad inside classes even if this lookahead is required.

@ljharb so, all of those should be valid.

Member

littledan commented Jun 13, 2017

At the May 2017 TC39 meeting, we decided that the grammar isn't too bad inside classes even if this lookahead is required.

@ljharb so, all of those should be valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment