Skip to content

Class AST: bound/computed properties, executable body #5208

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

Merged
merged 4 commits into from
Apr 29, 2019

Conversation

helixbass
Copy link
Collaborator

@GeoffreyBooth this covers the remaining class AST:

  • bound methods
  • computed properties/methods
  • executable bodies

@@ -2668,6 +2668,7 @@ exports.Class = class Class extends Base
else if method.bound
@boundMethods.push method

return unless o.compiling
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the natural representation for executable class body statements is the equivalent AST as if they were just statements in a regular block

So here, we don't have to deal with ExecutableClassBody/HoistTarget at all, we just leave the class body Block as a mix of "class-specific" nodes (ClassMethod/ClassProperty/ClassPrototypeProperty) and other "executable body" nodes, and then the class body AST ends up looking like eg:

# this could correspond to eg:
# class A
#   b: ->
#   x = 3
#   @c: 2
#   if y
#     z()
type: 'ClassBody'
body: [
  {type: 'ClassMethod', ...}
  {type: 'ExpressionStatement', ...}
  {type: 'ClassProperty', ...}
  {type: 'IfStatement', ...}
]

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ones that match class fields should probably output as those Babel node types, I'd think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you give an example of what you mean by "class fields"? See my comment as far as my understanding of how Coffeescript constructs relate to the JS class fields proposal

So basically we are matching Babel for everything that corresponds semantically/structurally to a JS/Babel construct

Eg a static property corresponds to a JS "static class field", so for this:

class A
  @b: 2

we generate an AST node with:

type: 'ClassProperty'
static: true

like you'd see in the Babel AST for

class A {
  static b = 2
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just meant the class fields proposal. Though I guess it doesn’t apply here because these are all on the prototype?

See https://astexplorer.net/#/gist/152f7080b8be9dde369647933ab7747f/c7aea4d9cae9df2a4b6fbf61bbac604f6d8dac2e

class Foo {
  a = 1
}

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup I don't think we have a structure that corresponds to non-static JS class fields

I read over #4552 for context. My understanding is that the answer to his first question there ("does the b property have different semantics?") is "yes", that since b: 3 is set (once) on the prototype, it's not semantically the same as a JS class field eg b = 3

So that's why I introduced ClassPrototypeProperty AST nodes to represent eg b: 3, since they're a different construct that doesn't seem to have an equivalent in JS/Babel classes

@GeoffreyBooth GeoffreyBooth merged commit 391fcc4 into jashkenas:ast Apr 29, 2019
@helixbass helixbass deleted the class-body-ast branch April 30, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants