Skip to content

Conversation

@helixbass
Copy link
Collaborator

@GeoffreyBooth this covers the remaining class AST:

  • bound methods
  • computed properties/methods
  • executable bodies

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