-
Notifications
You must be signed in to change notification settings - Fork 694
Const enum inlining and synthetic comment emit support #1599
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
Changes from all commits
3497c59
eb5048e
0467926
a4eb181
0f447ac
48b1b76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -511,6 +511,14 @@ const ( | |
hasSourceMapRange | ||
) | ||
|
||
type SynthesizedComment struct { | ||
Kind ast.Kind | ||
Loc core.TextRange | ||
HasLeadingNewLine bool | ||
HasTrailingNewLine bool | ||
Text string | ||
} | ||
|
||
type emitNode struct { | ||
flags emitNodeFlags | ||
emitFlags EmitFlags | ||
|
@@ -519,6 +527,8 @@ type emitNode struct { | |
tokenSourceMapRanges map[ast.Kind]core.TextRange | ||
helpers []*EmitHelper | ||
externalHelpersModuleName *ast.IdentifierNode | ||
leadingComments []SynthesizedComment | ||
trailingComments []SynthesizedComment | ||
} | ||
|
||
// NOTE: This method is not guaranteed to be thread-safe | ||
|
@@ -917,3 +927,37 @@ func (c *EmitContext) VisitIterationBody(body *ast.Statement, visitor *ast.NodeV | |
|
||
return updated | ||
} | ||
|
||
func (c *EmitContext) SetSyntheticLeadingComments(node *ast.Node, comments []SynthesizedComment) *ast.Node { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not this PR, but there are some callers of this I left commented out in the node builder (probably more than that though, just opining) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sent #1607 before this task slipped out of my brain. |
||
c.emitNodes.Get(node).leadingComments = comments | ||
return node | ||
} | ||
|
||
func (c *EmitContext) AddSyntheticLeadingComment(node *ast.Node, kind ast.Kind, text string, hasTrailingNewLine bool) *ast.Node { | ||
c.emitNodes.Get(node).leadingComments = append(c.emitNodes.Get(node).leadingComments, SynthesizedComment{Kind: kind, Loc: core.NewTextRange(-1, -1), HasTrailingNewLine: hasTrailingNewLine, Text: text}) | ||
return node | ||
} | ||
|
||
func (c *EmitContext) GetSyntheticLeadingComments(node *ast.Node) []SynthesizedComment { | ||
if c.emitNodes.Has(node) { | ||
return c.emitNodes.Get(node).leadingComments | ||
} | ||
return nil | ||
} | ||
|
||
func (c *EmitContext) SetSyntheticTrailingComments(node *ast.Node, comments []SynthesizedComment) *ast.Node { | ||
c.emitNodes.Get(node).trailingComments = comments | ||
return node | ||
} | ||
|
||
func (c *EmitContext) AddSyntheticTrailingComment(node *ast.Node, kind ast.Kind, text string, hasTrailingNewLine bool) *ast.Node { | ||
c.emitNodes.Get(node).trailingComments = append(c.emitNodes.Get(node).trailingComments, SynthesizedComment{Kind: kind, Loc: core.NewTextRange(-1, -1), HasTrailingNewLine: hasTrailingNewLine, Text: text}) | ||
return node | ||
} | ||
|
||
func (c *EmitContext) GetSyntheticTrailingComments(node *ast.Node) []SynthesizedComment { | ||
if c.emitNodes.Has(node) { | ||
return c.emitNodes.Get(node).trailingComments | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there more inliners than just constenum? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As of right now, no, but we've toyed with adding minifer-like emit passes in the past, so it's a logical place to group them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's going to happen, but 😄 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
package inliners | ||
|
||
import ( | ||
"strings" | ||
|
||
"github.com/microsoft/typescript-go/internal/ast" | ||
"github.com/microsoft/typescript-go/internal/core" | ||
"github.com/microsoft/typescript-go/internal/debug" | ||
"github.com/microsoft/typescript-go/internal/jsnum" | ||
"github.com/microsoft/typescript-go/internal/printer" | ||
"github.com/microsoft/typescript-go/internal/scanner" | ||
"github.com/microsoft/typescript-go/internal/transformers" | ||
) | ||
|
||
type ConstEnumInliningTransformer struct { | ||
transformers.Transformer | ||
compilerOptions *core.CompilerOptions | ||
currentSourceFile *ast.SourceFile | ||
emitResolver printer.EmitResolver | ||
} | ||
|
||
func NewConstEnumInliningTransformer(opt *transformers.TransformOptions) *transformers.Transformer { | ||
compilerOptions := opt.CompilerOptions | ||
emitContext := opt.Context | ||
if compilerOptions.GetIsolatedModules() { | ||
debug.Fail("const enums are not inlined under isolated modules") | ||
} | ||
tx := &ConstEnumInliningTransformer{compilerOptions: compilerOptions, emitResolver: opt.EmitResolver} | ||
return tx.NewTransformer(tx.visit, emitContext) | ||
} | ||
|
||
func (tx *ConstEnumInliningTransformer) visit(node *ast.Node) *ast.Node { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, I love how short this is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I inlined some indirections from strada. |
||
switch node.Kind { | ||
case ast.KindPropertyAccessExpression, ast.KindElementAccessExpression: | ||
{ | ||
parse := tx.EmitContext().ParseNode(node) | ||
if parse == nil { | ||
return tx.Visitor().VisitEachChild(node) | ||
} | ||
value := tx.emitResolver.GetConstantValue(parse) | ||
if value != nil { | ||
var replacement *ast.Node | ||
switch v := value.(type) { | ||
case jsnum.Number: | ||
if v.IsInf() { | ||
if v.Abs() == v { | ||
replacement = tx.Factory().NewIdentifier("Infinity") | ||
} else { | ||
replacement = tx.Factory().NewPrefixUnaryExpression(ast.KindMinusToken, tx.Factory().NewIdentifier("Infinity")) | ||
} | ||
} else if v.IsNaN() { | ||
replacement = tx.Factory().NewIdentifier("NaN") | ||
} else if v.Abs() == v { | ||
replacement = tx.Factory().NewNumericLiteral(v.String()) | ||
} else { | ||
replacement = tx.Factory().NewPrefixUnaryExpression(ast.KindMinusToken, tx.Factory().NewNumericLiteral(v.Abs().String())) | ||
} | ||
case string: | ||
replacement = tx.Factory().NewStringLiteral(v) | ||
case jsnum.PseudoBigInt: // technically not supported by strada, and issues a checker error, handled here for completeness | ||
jakebailey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if v == (jsnum.PseudoBigInt{}) { | ||
replacement = tx.Factory().NewBigIntLiteral("0") | ||
} else if !v.Negative { | ||
replacement = tx.Factory().NewBigIntLiteral(v.Base10Value) | ||
} else { | ||
replacement = tx.Factory().NewPrefixUnaryExpression(ast.KindMinusToken, tx.Factory().NewBigIntLiteral(v.Base10Value)) | ||
} | ||
} | ||
|
||
if tx.compilerOptions.RemoveComments.IsFalseOrUnknown() { | ||
original := tx.EmitContext().MostOriginal(node) | ||
if original != nil && !ast.NodeIsSynthesized(original) { | ||
originalText := scanner.GetTextOfNode(original) | ||
escapedText := " " + safeMultiLineComment(originalText) + " " | ||
tx.EmitContext().AddSyntheticTrailingComment(replacement, ast.KindMultiLineCommentTrivia, escapedText, false) | ||
} | ||
} | ||
return replacement | ||
} | ||
return tx.Visitor().VisitEachChild(node) | ||
} | ||
} | ||
return tx.Visitor().VisitEachChild(node) | ||
} | ||
|
||
jakebailey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func safeMultiLineComment(text string) string { | ||
return strings.ReplaceAll(text, "*/", "*_/") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ function foo(x) { | |
var y = x; // Ok | ||
} | ||
foo(5); | ||
foo(E.A); | ||
foo(0 /* E.A */); | ||
class A { | ||
a; | ||
} | ||
|
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.
In strada,
checkExpressionCached
ona.b.c.d
caches resolved symbols ona.b.c
anda.b
anda
.getConstantValue
uses these cached symbols to shortcut its' logic. In corsa, it does not, and, in so doing, reveals that here we try to callresolveAlias
on non-alias (module) symbols when we consider if we want to inlinemodule.exports
. This fixes that.