Skip to content
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

Adds a simplified pretty printer for tree transformations #6987

Merged
merged 88 commits into from
Mar 18, 2016

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Feb 9, 2016

Unfortunately, we cannot easily leverage our existing emitter to emit transformed nodes. The current emitter often assumes nodes are original source tree nodes, rather than synthesized nodes created during a transformation phase. Transformers are not required to set parent pointers on nodes, as they will often reuse existing source tree nodes and should not change the parent pointers on these nodes.

Instead we will add a new, simpler node printer whose responsibility is to emit the exact node tree provided.

The printFiles function will eventually supplant the existing emitFiles function. When called, this function will get a list of Transformer callbacks based on the provided compiler options, adding the node printer as the last transformer in the list. This gives the node printer access to the TransformationContext, allowing it to process substitutions and track any hoisted declarations added during just-in-time substitution.

The node printer in printFiles is designed for performance, and minimizes the number of conditional branches within the emit for any node. The expectation is that all transformers have finished every non-substitution transformation before the printer is called, and therefore the node printer will print nodes verbatim.

In addition, similarly to how source maps were extracted from emitter.ts and into a SourceMapWriter to support the node printer, comment emit is now also extracted into a CommentWriter.

Related Pull Requests:

Review on Reviewable

@rbuckton
Copy link
Member Author

rbuckton commented Feb 9, 2016

Paging for review: @mhegazy, @ahejlsberg, @yuit, @DanielRosenwasser, @RyanCavanaugh, @vladima

@DanielRosenwasser
Copy link
Member

image

image

@DanielRosenwasser
Copy link
Member

You can get around this by using Reviewable.

@DanielRosenwasser
Copy link
Member

Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions.


src/compiler/comments.ts, line 143 [r1] (raw file):
emitted as

Put some sort of quote demarcation around the emit.


src/compiler/comments.ts, line 148 [r1] (raw file):
same here regarding demarcation and "as"


src/compiler/comments.ts, line 185 [r1] (raw file):
Just use a numeric comparison


src/compiler/utilities.ts, line 115 [r1] (raw file):
I thought it was CommonJS by default but this works too.


src/compiler/factory.ts, line 148 [r1] (raw file):
What's the point of this exactly? Why can't you say this just returns a PrimaryExpression?


Comments from the review on Reviewable.io

@rbuckton
Copy link
Member Author

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.


src/compiler/comments.ts, line 143 [r1] (raw file):
Copy/paste from emitter, but I'll take a look.


src/compiler/comments.ts, line 148 [r1] (raw file):
Copy/paste from emitter.


src/compiler/comments.ts, line 185 [r1] (raw file):
Copy/paste from emitter.


src/compiler/utilities.ts, line 115 [r1] (raw file):
Not when the target is ES6. It looks like someone else had the same idea though, so I will drop these in favor of getEmitScriptTarget and getEmitModuleKind.


Comments from the review on Reviewable.io

@DanielRosenwasser
Copy link
Member

Review status: 0 of 7 files reviewed at latest revision, 13 unresolved discussions.


src/compiler/printer.ts, line 184 [r1] (raw file):
Make it explicit withTempFlags.Auto


src/compiler/printer.ts, line 742 [r1] (raw file):
Could we not emit a semicolon as part of emitting a signature list?


src/compiler/printer.ts, line 995 [r1] (raw file):
Does the pretty-printer make no assumptions about the structure? Is it the responsibility of the prior transformers to ensure that an object literal is not the immediate concise body?


src/compiler/printer.ts, line 1106 [r1] (raw file):
Why even do it with a prefix? Or if you do, why not just

emitExpressionWithPrefix(node.asteriskToken ? "yield* " : "yield ", node.expression);·

src/compiler/printer.ts, line 1788 [r1] (raw file):
Cache length


src/compiler/printer.ts, line 1798 [r1] (raw file):
I don't like how the function doesn't document its return value. Can you make the function name more explicit and add a JSDoc comment?


src/compiler/printer.ts, line 1870 [r1] (raw file):
Cache length


src/compiler/printer.ts, line 1984 [r1] (raw file):
What is this condition for?

What about default initializers and optional parameters? Do we have tests for these cases?


Comments from the review on Reviewable.io

@rbuckton
Copy link
Member Author

Review status: 0 of 12 files reviewed at latest revision, 13 unresolved discussions.


src/compiler/printer.ts, line 742 [r1] (raw file):
This code path won't be used until such a time as we migrate the declaration emitter to use the pretty printer, but in the declaration emitter we always emit a semicolon.


src/compiler/printer.ts, line 995 [r1] (raw file):
That is correct, although I am adding some support to automatically parenthesize expressions to ensure the correct output. One reason for this is to make sure we have the right parentheses around nodes without having to know about the parent node during an individual transformation.


src/compiler/printer.ts, line 1106 [r1] (raw file):
Because yield; is legal in JavaScript, and we don't want to emit yield ;.


src/compiler/printer.ts, line 1788 [r1] (raw file):
I've discussed this with @vladima in the past. V8 still does an array length lookup on every iteration to perform a bounds check, so caching the length doesn't necessarily have a real benefit.


src/compiler/printer.ts, line 1798 [r1] (raw file):
I'll fix this, though the equivalent function in emitter.ts is also undocumented.


src/compiler/printer.ts, line 1870 [r1] (raw file):
See above.


src/compiler/printer.ts, line 1984 [r1] (raw file):
These are effectively the same conditions as in emitSignatureParametersForArrow in emitter.ts, with some additional restrictions.


Comments from the review on Reviewable.io

rbuckton and others added 25 commits March 2, 2016 12:32
rbuckton added a commit that referenced this pull request Mar 18, 2016
Adds a simplified pretty printer for tree transformations
@rbuckton rbuckton merged commit 85ae53d into transforms-transformer Mar 18, 2016
@rbuckton rbuckton deleted the transforms-printer branch March 18, 2016 23:39
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants