Fix several issues with class properties (#157, #116)

[motiz88]

EDIT: This rolls up fixes and tests for several interrelated bugs detailed in issues #157 and #116 as well as in the comments below, namely:

1. Incorrect semicolon insertion after get / set in a class (resulting in a property + regular method);
2. Incorrect semicolon insertion after a method name in a class (resulting in a property + parse error);
3. Regressed edge cases from the initial fixes for 1 and 2;
4. Class properties not being parsed if they appear on one line separated by semicolons;
5. ClassProperty nodes being present in the output without classProperties being set (except when flow is set, which should enable uninitialized properties only)

---

To repeat the note I made in #157: All parsers on ast-explorer.net, except for TypeScript, disagree with Babylon's current parse and agree with this PR (EDIT: this was true as of ca66f6b and refers only to the initial get/set issue from #157 - with the later fixes we've diverged from e.g. Flow)

I'm not saying it's necessarily correct, though. Definitely could use input from a higher authority on spec matters.

[codecov-io]

Current coverage is 94.59% (diff: 100%)

Merging #158 into master will increase coverage by 0.07%

@@             master       #158   diff @@
==========================================
  Files            19         19          
  Lines          3135       3144     +9   
  Methods         328        331     +3   
  Messages          0          0          
  Branches        828        833     +5   
==========================================
+ Hits           2963       2974    +11   
  Misses           94         94          
+ Partials         78         76     -2   

Powered by Codecov. Last update 44f77bd...c51fb67

[bakkot]

Not sure if I count as a higher authority, but it is absolutely the case that no ASI can occur after get: ASI can only occur if the following token is "not allowed by any production of the grammar". See spec.

See also other committee members on the same topic.

Also, the issue is larger than I initially reported and is not covered by this fix. All of the following are incorrectly rejected:

class C {
  x
  (){}
}

class C {
  get
  (){}
}

class C {
  set
  (){}
}

class C {
  static
  (){}
}

class C {
  async
  (){}
}

etc., which define methods named 'x', 'get', 'set', 'static', or 'async'.

EDIT: same issue when the name is 'x' or ['x'].

[loganfsmyth]

Haven't looked into the PR implementation, but I'll verify it seems like the newline should be allowed there since the spec explicitly marks things with [no LineTerminator here] in the grammar where newlines aren't allowed and http://www.ecma-international.org/ecma-262/7.0/#sec-method-definitions doesn't have that in the grammar.

[danez]

@motiz88 Should we do the issues that @bakkot reported in a new PR or also within this one? Maybe you already have an idea if it will be easy or not to fix. I can create an issue otherwise.

Besides that looks good to me.

[motiz88]

I will try to fix them here and report back.

On Oct 12, 2016 11:53, "Daniel Tschinder" [email protected] wrote:

@motiz88 https://github.com/motiz88 Should we do the issues that @bakkot
https://github.com/bakkot reported in a new PR or also within this one?
Maybe you already have an idea if it will be easy or not to fix. I can
create an issue otherwise.

Besides that looks good to me.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#158 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACJHpVR6Ux7FhgeXp7aKq4IwJ4ujxhvYks5qzKAdgaJpZM4KOtEO
.

[motiz88]

Looking into this now. By the way, async is out - the spec does say [no LineTerminator here] between it and its PropertyName,

[bakkot]

Yeah, the fun edge case for async is

class C {
  async
  x(){}
}

which should have ASI performed, such that it parses the same as

class C {
  async;
  x(){}
}

i.e., a syntax error if class properties are not enabled, and otherwise an uninitialized class property named async followed by a method named x.

Contrast

class C {
  static
  x(){}
}

which is a static method named x.

IIRC, Babylon gets these three cases right.

[motiz88]

OK, this was as simple as bailing out from a ClassProperty parse if the name is followed by a left paren. I've also added tests to cover the various cases raised here.

[bakkot]

I don't think that's going to be quite sufficient - e.g. this is a class with just a property named 'get': class C { get }. Similarly you can have properties just named 'static' or 'async' or 'set' in the same position.

This patch as it stands regresses 'get' and 'set' there and handles 'async' correctly; it does not correctly handle 'static', but neither does master.

[bakkot]

While I'm at it...

Class properties can appear on the same line if separated by semicolons. So class C { a; b; } should be legal, as should class C { a = 0; b; }.

Sorry for continuing to come back with more issues; I'm just mentioning them as I think of them and/or recall them.

As long as this patch doesn't regress any such cases, I would expect it to be fine to handle this issue in another patch.

[motiz88]

@bakkot Personally I'm very grateful for your input. If I'm working on this part of the code, might as well get it right and not just fix one edge case 😃

[motiz88]

Hang on, I'm not comfortable with us emitting ClassProperty nodes without the classProperties plugin. Gonna fix that too.

[motiz88]

Well! I'm quite possibly done. Come on, hit this with all you've got 💪

[bakkot]

I am not involved in the project, so take this with a grain of salt, but nested ifs like this look strange to me - why not combine the two conditions?

[bakkot]

Issues: rejects class C { static; } and class C { static = 0 }, and parses

class C {
  get
  [Symbol.foo](){}
}

and

class C {
  static
  get
  x(){}
}

as a property and a method instead of as a getter.

Might also be good to add a test for

class C {
  get
  *x(){}
}

(a property followed by a generator method), which is handled correctly but which is kind of a funny case.

Otherwise looks good to me.

[motiz88]

@bakkot That first one mayyyy be out of scope here but I'd like to fix it too, with the same rationale as above.

This is getting nightmarish - what about this one?

class C {
  static
  get
  static
  (){}
}

This is a static getter named static, right?

[bakkot]

@motiz88, indeed it is.

I think you pretty much have to end up repeatedly inspecting the next token to determine the type of the property. Something like this (forgive the psuedocode / difference from Babylon; I'm not quite awake and haven't read the project source):

function parsePropertyName() {...} // should include static and computed names


let static = false;

if (eat('static')) {
  switch (lookahead) { // assuming 'lookahead' magically holds the next token
    case '(':
      return parseClassMethod({name: 'static', static, async: false});
    case '=':
      return parseInitializedClassProperty({name: 'static', static});
    case ';':
    case '}':
      return parseUninitializedClassProperty({name: 'static', static});
  }
  static = true;
}

if (eat('*')) {
  let name = parsePropertyName();
  return parseGeneratorMethod({name, static});
}

if (eat('async')) {
  switch (lookahead) {
    case '(':
      return parseClassMethod({name: 'async', static, async: false});
    case '=':
      return parseInitializedClassProperty({name: 'async', static});
    case ';':
    case '}':
      return parseUninitializedClassProperty({name: 'async', static});
  }
  if (hasLineTerminatorBeforeNext) {
    return parseUninitializedClassProperty({name: 'async', static});
  }
  let name = parsePropertyName();
  return parseClassMethod({name, static, async: true});
}

if (eat('get')) {
  switch (lookahead) {
    case '(':
      return parseClassMethod({name: 'get', static, async: false});
    case '=':
      return parseInitializedClassProperty({name: 'get', static});
    case ';':
    case '}':
      return parseUninitializedClassProperty({name: 'get', static});
    case '*':
      if (hasLineTerminatorBeforeNext) {
        return parseUninitializedClassProperty({name: 'get', static});
      } else {
        throw new SyntaxError('unexpected token');
      }
  }
  let name = parsePropertyName();
  return parseGetter({name, static});
}

if (eat('set')) {
  switch (lookahead) {
    case '(':
      return parseClassMethod({name: 'set', static, async: false});
    case '=':
      return parseInitializedClassProperty({name: 'set', static});
    case ';':
    case '}':
      return parseUninitializedClassProperty({name: 'set', static});
    case '*':
      if (hasLineTerminatorBeforeNext) {
        return parseUninitializedClassProperty({name: 'set', static});
      } else {
        throw new SyntaxError('unexpected token');
      }
  }
  let name = parsePropertyName();
  return parseSetter({name, static});
}

let name = parsePropertyName();
switch (lookahead) {
  case '(':
    return parseClassMethod({name, static, async: false});
  case '=':
    return parseInitializedClassProperty({name, static});
}
return parseUninitializedClassProperty({name, static});

[motiz88]

@bakkot Yeah, looks like a rewrite for sanity is in order and your pseudocode will be a lifesaver. Thanks! Will get to it in a day or two.

[danez]

Hey @motiz88 What is the status on this? can this be reviewed and merged or is there still stuff open?

[motiz88]

@danez - this is still in need of a major rewrite. I unexpectedly ran out of time to devote to this issue, and this will go on for a few weeks at least.

As is, the PR solves some edge cases but opens up others, and I've come to realize that larger changes would be required for the code to be both correct and maintainable. Hence a rewrite.

[bakkot]

@motiz88, still planning on getting to this? I expect I could take it if not.

[hzoo]

I believe @motiz88 has been pretty busy over the last few months so dono when ready to get back into oss stuff

[motiz88]

Yeah @bakkot @hzoo. Sorry for the sharp drop in my availability. I will most definitely be back though. In the mean time I'm not working on this issue.

[danez]

obsolete as of #351