-
Notifications
You must be signed in to change notification settings - Fork 2k
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
If AST #5160
If AST #5160
Conversation
@@ -817,8 +817,8 @@ grammar = | |||
If: [ | |||
o 'IfBlock' | |||
o 'IfBlock ELSE Block', -> $1.addElse $3 | |||
o 'Statement POST_IF Expression', -> new If $3, LOC(1)(Block.wrap [$1]), type: $2, statement: true | |||
o 'Expression POST_IF Expression', -> new If $3, LOC(1)(Block.wrap [$1]), type: $2, statement: true | |||
o 'Statement POST_IF Expression', -> new If $3, LOC(1)(Block.wrap [$1]), type: $2, postfix: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statement
option to the If
constructor was ignored and corresponded to it being a postfix if (which we now need to track), so renamed it to postfix
@@ -270,7 +270,9 @@ exports.Base = class Base | |||
# as JSON. This is what the `ast` option in the Node API returns. | |||
# We try to follow the [Babel AST spec](https://github.com/babel/babel/blob/master/packages/babel-parser/ast/spec.md) | |||
# as closely as possible, for improved interoperability with other tools. | |||
ast: (o) -> | |||
ast: (o, level) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to how we just added support for o.scope
, we now need to support passing level
on o
So allow passing it as a second argument to ast()
(like the signature of compileToFragments()
)
@@ -1121,7 +1124,7 @@ exports.Return = class Return extends Base | |||
astType: -> 'ReturnStatement' | |||
|
|||
astProperties: (o) -> | |||
argument: @expression?.ast(o) ? null | |||
argument: @expression?.ast(o, LEVEL_PAREN) ? null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So went back through existing calls to .ast()
for child node AST generation and populated the appropriate level
arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someday I'd like to understand how you know which level to use. That would be good to add in a big comment somewhere if it's not written out already (yeah, I'm too lazy to look 😀 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The levels used for child nodes in AST generation here should basically match the corresponding levels used for those children when compiling to JS
But as far as how those work, I haven't seen a thorough explanation - there's a little general comment and examples where the LEVEL_*
s are defined towards the bottom of nodes.coffee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Try compiling and see if it requires more parentheses or not" in general is how it's used. That's why we've had wrong parsing precedence with little to no issue for a long while:
#2199 (comment)
super() | ||
@condition = if options.type is 'unless' then condition.invert() else condition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to defer inverting @condition
so that the original condition is still intact at AST generation time
@processedConditionCache ?= if @type is 'unless' then @condition.invert() else @condition | ||
|
||
isStatementAst: (o) -> | ||
o.level is LEVEL_TOP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what needed o.level
to be set - in order to more closely mimic Babel AST, If
ASTs should be of type IfStatement
(ie a JS if
statement) if we're at LEVEL_TOP
and a ConditionalExpression
(ie a JS ternary) otherwise
else | ||
@elseBody?.ast(o, LEVEL_TOP) ? null | ||
postfix: !!@postfix | ||
inverted: @type is 'unless' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postfix
and inverted
are extensions to the Babel AST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just some nits.
src/nodes.coffee
Outdated
@@ -270,7 +270,9 @@ exports.Base = class Base | |||
# as JSON. This is what the `ast` option in the Node API returns. | |||
# We try to follow the [Babel AST spec](https://github.com/babel/babel/blob/master/packages/babel-parser/ast/spec.md) | |||
# as closely as possible, for improved interoperability with other tools. | |||
ast: (o) -> | |||
ast: (o, level) -> | |||
o = extend {}, o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been trying to phase out these helpers when built-ins can do, like Object.assign
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, updated to use Object.assign()
@@ -1121,7 +1124,7 @@ exports.Return = class Return extends Base | |||
astType: -> 'ReturnStatement' | |||
|
|||
astProperties: (o) -> | |||
argument: @expression?.ast(o) ? null | |||
argument: @expression?.ast(o, LEVEL_PAREN) ? null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someday I'd like to understand how you know which level to use. That would be good to add in a big comment somewhere if it's not written out already (yeah, I'm too lazy to look 😀 )
You've knocked out some big ones lately, and quickly. How much more do you think there is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeoffreyBooth updated per your comments
You've knocked out some big ones lately, and quickly. How much more do you think there is?
As far as the remaining node classes, these are some of the big categories:
- classes
- interpolated strings/regexes and JSX text
- for/while/loop
And there are probably a couple stragglers, eg I think you mentioned that PassthroughLiteral
wasn't covered yet
src/nodes.coffee
Outdated
@@ -270,7 +270,9 @@ exports.Base = class Base | |||
# as JSON. This is what the `ast` option in the Node API returns. | |||
# We try to follow the [Babel AST spec](https://github.com/babel/babel/blob/master/packages/babel-parser/ast/spec.md) | |||
# as closely as possible, for improved interoperability with other tools. | |||
ast: (o) -> | |||
ast: (o, level) -> | |||
o = extend {}, o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, updated to use Object.assign()
@@ -1121,7 +1124,7 @@ exports.Return = class Return extends Base | |||
astType: -> 'ReturnStatement' | |||
|
|||
astProperties: (o) -> | |||
argument: @expression?.ast(o) ? null | |||
argument: @expression?.ast(o, LEVEL_PAREN) ? null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The levels used for child nodes in AST generation here should basically match the corresponding levels used for those children when compiling to JS
But as far as how those work, I haven't seen a thorough explanation - there's a little general comment and examples where the LEVEL_*
s are defined towards the bottom of nodes.coffee
big thanks @helixbass & @GeoffreyBooth for this huge step ahead - ASTexplorer shows much more detail now. I'm working on low hanging fruit alike CS.TaggedTemplateCall a.k.a. Babel.TaggedTemplateExpressions and friends. JSX-related-structures after that. |
@Asc2011 sure, just for some context about these AST PRs: I've already got a complete version of the target AST implemented on my prettier branch, so these PRs are basically me pulling chunks of code off of that branch and then @GeoffreyBooth and I have been cleaning them up and reviewing them for eg consistency So working on AST implementations for remaining classes like But if you're looking to participate, I'd suggest this:
Yes, so far we've been adding tests validating AST structure and location data to
Yes, documentation is something we haven't discussed yet. I'd think that documenting the diff against Babel AST (and linking to its documentation) would be a minimal version I think it might be nice to have a guide to the codebase too, so that someone exploring/hacking on the Coffeescript codebase could understand at least the AST-related stuff |
There’s this: https://github.com/babel/babel/blob/master/packages/babel-parser/ast/spec.md. I’ve generally been using astexplorer.net with
FYI we have this: https://github.com/jashkenas/coffeescript/wiki/%5BHowTo%5D-How-parsing-works though obviously improvements are welcome 😄 and/or a new wiki page about the AST stuff. |
wups, that went quick. How are the coffee-resque AST-Types e.g.
great, as Prettier is already part of ASTExplorer, that would be my starting-point/ -setup.
@GeoffreyBooth agreed to write smth. for the wiki. A JSON-schema that describes the overlap and differences between (Babel)-AST and a CS-AST will help interested contributors in the future. As a bonus, one can use it to validate stuff during dev. Looking at the CS-codebase I wondered if the current approach - staying with the cs-naming-conventions and then transform/rename nodes on export will hold. But let me first see and hopefully understand how things were done... |
We’ve definitely been thinking about streamlining the compiler once AST generation is done. Not sure why it’s slow for you. The slow part of recompiling the compiler is the parser, which doesn’t need to happen unless you change |
ic, so let's do this :-) i'm glad to fill a new wiki-page about the AST/Babel-tooling stuff.
I did not touch the grammar. I guess it's Atom or smth. misconfigured on my end or mojave, I dunno. anyway, the AST-output from Julians CS-branch looks good, but seems to break the local ASTExplorer :-( |
Personally I haven’t been piping the AST output into anything. I’ve just been having the compiler print it to the console and maybe format it there, e.g. |
@Asc2011 ya I'm not clear on how you're using ASTExplorer? That |
@helixbass i did a local install. It's a react-web-app, served by webpack. |
@Asc2011 ya last I checked it didn't look like Babel had updated their docs to reflect Here's a Babel issue that seemed to have a fair amount of information: babel/babel#7256 I haven't fully wrapped my head around it but one interesting aspect is that the usage of For example, look at the AST (with |
Also here's where we discussed it briefly in a previous AST PR: #5117 (comment) |
@helixbass hi again. I scanned the TCO39-proposals that will use soaking and
The whole process is a bit a 'catch-22'-thing. As it's only required if somebody uses the |
Another unused node-type is the |
We were planning on creating new node types for nodes that don’t exist in the Babel AST. This corresponds with how on some existing Babel AST node types, we’re adding custom properties (like whether an |
@GeoffreyBooth well, fair enough - so then lets forget all my comments from above :-) |
I’m not sure I understand. Currently the CoffeeScript compiler doesn’t rely on Babel unless you use the The CoffeeScript AST is just an output like the JavaScript source output. The obligation is on downstream tools receiving this AST to understand what to do with it. @helixbass has been working on plugins for Prettier and eslint to enable those tools to understand how to process the CoffeeScript AST. Since those tools already accept the Babel AST as input, the closer we are to that the simpler the plugin can be. |
Ya I haven't tried to process a CS AST with "Babel itself", just (as @GeoffreyBooth mentioned) with tools that know how to process a "Babel-style" AST So that's interesting if in fact Babel can't handle ASTs with node types that are unknown to it, but I don't think it's within scope of our AST generation (or something that would have a big impact on our approach) for us to try and create a "compatibility layer" there. I wonder if eg Babel syntax plugins are able to introduce new node types (it seems like they'd have to be able to)? @Asc2011 as far as the |
Also it's been on our to do list for a long time to create a Babel plugin for CoffeeScript like how there's one for TypeScript. That might be a bit tricky since TypeScript is a superset of JavaScript and we're not, but I hope it's still possible. If the plugin can pass onto Babel JS source rather than an AST, creating such a plugin can happen now and not need to wait on this AST work. |
@GeoffreyBooth I think CS is technically a superset of JS via the backtick operator. 😄 But I'm no expert on what's needed for a Babel plugin, and whether this perspective helps. |
Thank you, you got me right. And now I've learned that it should stay that way. I was asking because @helixbass |
Before we petition Babel, I think we should see how far we can get without any changes on their side. If a plugin can be written that outputs JavaScript source, and Babel can accept that as input, that’s all we need to do. |
@helixbass regarding the mentioned tc39-proposal(s): @GeoffreyBooth making a babel-syntax-plugin as a starting point. My current believe is that syntax-plugins are not open for public-usage. AFAIK same for the
Surely babel accepts any JS-code that CoffeeScript has generated. From here on - after babel has produced a ast - one can apply one..many |
No, because Babel won’t know how to handle our more exotic AST node types. It won’t know what to convert There’s a chance we could get Babel to include a CoffeeScript converter along the lines of their TypeScript one. According to this the TypeScript converter simply removes all the type annotations before piping the revised source into Babel; a parallel CoffeeScript one could simply run the source through the CoffeeScript compiler before piping the output JS source into Babel. I would start there. If you could get that to work, we could submit it as a PR to Babel (assuming that converters aren’t supported via configuration the way plugins are). |
@GeoffreyBooth thx for the good read. I remain a bit sceptic about redoing what typescript/MS achieved. As you suggested, i started trying stuff we can do without help. So i defined a
hopeing babel would allow to add a new ast-type temporarily to the (b) one could straight-compile the JS contents of the PassThru-node in the
In case it does not throw, then babel returns a ast-fragment to be merged later into the CoffeeScript-ast. That works nicely. Surely the location-info needed to be adjusted. I tried backticked-JS with transformations from 'pipeline-operator' and 'class getter'-stuff. Works fine, but the location-info lost some accuracy due to the transforms and thus may require a bit more attention. (c) temporarily masquerade the |
@Asc2011 I would start a lot less ambitious if I were you. As I understand things, there’s a Babel TypeScript plugin/converter that intercepts the source code before anything else in Babel sees it, and it strips out all the type annotations. Then this revised source gets passed to Babel for Babel to do its thing with like any other JS source. That’s it. There’s no TypeScript AST getting generated, there’s no AST getting passed as input to Babel. Babel gets JS source, the same as if you had run the TypeScript file through the TypeScript compiler to generate a So see if you can achieve the same but for CoffeeScript. It intercepts the source before any part of Babel sees it, runs it through the CoffeeScript compiler (loaded as a Node dependency) and then the resulting JavaScript source code (not AST) is sent onward into Babel. That’s it. Or looked at another way, see if you can achieve this “do something to the source before Babel ever sees it” with Illiterate. That’s even more generic. If you can make that work, Babel would be able to process Literate JavaScript and Literate TypeScript and Literate JSX 😉 And you’d only have to replace the call to Illiterate with a call to CoffeeScript and then you have the CoffeeScript equivalent. |
@GeoffreyBooth PR for If AST