Merged
Conversation
Adds a `ts-ignore` comment to thew instantiation expressions in factories, because in some compilation modes we can't guarantee that they'll compile.
Cleans up the `escapeDollarInStrings` parameter in various places since it isn't passed anywhere.
Allows for consumers of the `AbstractEmitterVisitor` to determine whether comments should be printed.
691b1a0 to
b6c57a7
Compare
mattrbeck
approved these changes
Mar 18, 2026
|
|
||
| switch (type.name) { | ||
| case o.BuiltinTypeName.Bool: | ||
| ctx.print(null, ': boolean'); |
Member
There was a problem hiding this comment.
It feels a little funny to me that we're printing the : with the type rather than in the parent. Ditto for other places like visitExpressionType. Does it need to be printed here?
Member
Author
There was a problem hiding this comment.
There are a couple of reasons I did it this way:
- We have the
BuiltinTypeName.Inferredwhich basically outputs an empty type annotation. By baking in the:we can avoid having to check for it everywhere. - This is a bit more convenient because we can just visit types without having to insert
:conditionally.
| this.printLeadingComments(stmt, ctx); | ||
| ctx.print(stmt, `function ${stmt.name}(`); | ||
| this.visitParams(stmt.params, ctx); | ||
| ctx.println(stmt, `)`); |
Member
There was a problem hiding this comment.
Do we want newlines between the closing paren and the return type? Ditto for visitFunctionExpr
Member
Author
There was a problem hiding this comment.
This visitor doesn't really focus on making the output readable, e.g. object literals are all on one line.
Moves the logic for constructing more AST nodes into the `AbstractEmitterVisitor` so it's easier to implement. Also cleans up the visitor a bit.
Adds an opt-in flag to emit type information through the base abstract emitter.
b6c57a7 to
156d751
Compare
atscott
approved these changes
Mar 18, 2026
Member
Author
|
Caretaker note: the presubmit failures are pre-existing and unrelated. |
Member
|
This PR was merged into the repository. The changes were merged into the following branches:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes an issue where the factories we generate might not be able to compile, based on the level of analysis.
Also expands and reworks the base abstract emitter so we can reuse it in other contexts.