-
Notifications
You must be signed in to change notification settings - Fork 715
Port TypeScript PR #60304: More rigorous ASI prevention when emitting return/yield #1850
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
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 |
---|---|---|
|
@@ -2947,8 +2947,38 @@ | |
} | ||
} | ||
|
||
func (p *Printer) commentWillEmitNewLine(comment ast.CommentRange) bool { | ||
return comment.Kind == ast.KindSingleLineCommentTrivia || comment.HasTrailingNewLine | ||
} | ||
|
||
func (p *Printer) willEmitLeadingNewLine(node *ast.Expression) bool { | ||
return false // !!! check if node will emit a leading comment that contains a trailing newline | ||
if p.currentSourceFile == nil { | ||
return false | ||
} | ||
|
||
leadingCommentRanges := scanner.GetLeadingCommentRanges(p.emitContext.Factory.AsNodeFactory(), p.currentSourceFile.Text(), node.Pos()) | ||
for comment := range leadingCommentRanges { | ||
// Check if the comment will emit a newline | ||
if p.commentWillEmitNewLine(comment) { | ||
return true | ||
} | ||
} | ||
|
||
// For PartiallyEmittedExpression, recursively check the inner expression | ||
if node.Kind == ast.KindPartiallyEmittedExpression { | ||
pee := node.AsPartiallyEmittedExpression() | ||
if node.Pos() != pee.Expression.Pos() { | ||
trailingCommentRanges := scanner.GetTrailingCommentRanges(p.emitContext.Factory.AsNodeFactory(), p.currentSourceFile.Text(), pee.Expression.Pos()) | ||
for comment := range trailingCommentRanges { | ||
if p.commentWillEmitNewLine(comment) { | ||
return true | ||
} | ||
} | ||
} | ||
return p.willEmitLeadingNewLine(pee.Expression) | ||
} | ||
|
||
return false | ||
} | ||
|
||
func (p *Printer) emitExpressionNoASI(node *ast.Expression, precedence ast.OperatorPrecedence) { | ||
|
@@ -2964,12 +2994,155 @@ | |
// a; | ||
// } | ||
// Due to ASI, this would result in a `return` with no value followed by an unreachable expression statement. | ||
if !p.commentsDisabled && node.Kind == ast.KindPartiallyEmittedExpression && p.willEmitLeadingNewLine(node) { | ||
// !!! if there is an original parse tree node, restore it with location to preserve comments and source maps. | ||
p.emitExpression(node, ast.OperatorPrecedenceParentheses) | ||
} else { | ||
p.emitExpression(node, precedence) | ||
p.emitExpression(p.parenthesizeExpressionForNoAsi(node), precedence) | ||
} | ||
|
||
// parenthesizeExpressionForNoAsi walks down the left side of expressions to check if any | ||
// PartiallyEmittedExpression might introduce ASI, and returns an expression that ensures | ||
// proper parenthesization to prevent ASI issues. | ||
func (p *Printer) parenthesizeExpressionForNoAsi(node *ast.Expression) *ast.Expression { | ||
if p.commentsDisabled { | ||
return node | ||
} | ||
|
||
switch node.Kind { | ||
case ast.KindPartiallyEmittedExpression: | ||
pee := node.AsPartiallyEmittedExpression() | ||
if p.willEmitLeadingNewLine(node) { | ||
// !!! if there is an original parse tree node, restore it with location to preserve comments and source maps. | ||
// Emit with parentheses precedence to force wrapping | ||
return p.createParenthesizedExpressionForNoAsi(node) | ||
} | ||
// Recursively check the inner expression | ||
innerParenthesized := p.parenthesizeExpressionForNoAsi(pee.Expression) | ||
if innerParenthesized != pee.Expression { | ||
// Need to create a new PartiallyEmittedExpression with the parenthesized inner expression | ||
return p.updatePartiallyEmittedExpression(pee, innerParenthesized) | ||
} | ||
|
||
case ast.KindPropertyAccessExpression: | ||
pae := node.AsPropertyAccessExpression() | ||
exprParenthesized := p.parenthesizeExpressionForNoAsi(pae.Expression) | ||
if exprParenthesized != pae.Expression { | ||
return p.updatePropertyAccessExpression(pae, exprParenthesized) | ||
} | ||
|
||
case ast.KindElementAccessExpression: | ||
eae := node.AsElementAccessExpression() | ||
exprParenthesized := p.parenthesizeExpressionForNoAsi(eae.Expression) | ||
if exprParenthesized != eae.Expression { | ||
return p.updateElementAccessExpression(eae, exprParenthesized) | ||
} | ||
|
||
case ast.KindCallExpression: | ||
ce := node.AsCallExpression() | ||
exprParenthesized := p.parenthesizeExpressionForNoAsi(ce.Expression) | ||
if exprParenthesized != ce.Expression { | ||
return p.updateCallExpression(ce, exprParenthesized) | ||
} | ||
|
||
case ast.KindTaggedTemplateExpression: | ||
tte := node.AsTaggedTemplateExpression() | ||
tagParenthesized := p.parenthesizeExpressionForNoAsi(tte.Tag) | ||
if tagParenthesized != tte.Tag { | ||
return p.updateTaggedTemplateExpression(tte, tagParenthesized) | ||
} | ||
|
||
case ast.KindPostfixUnaryExpression: | ||
pue := node.AsPostfixUnaryExpression() | ||
operandParenthesized := p.parenthesizeExpressionForNoAsi(pue.Operand) | ||
if operandParenthesized != pue.Operand { | ||
return p.updatePostfixUnaryExpression(pue, operandParenthesized) | ||
} | ||
|
||
case ast.KindBinaryExpression: | ||
be := node.AsBinaryExpression() | ||
leftParenthesized := p.parenthesizeExpressionForNoAsi(be.Left) | ||
if leftParenthesized != be.Left { | ||
return p.updateBinaryExpression(be, leftParenthesized) | ||
} | ||
|
||
case ast.KindConditionalExpression: | ||
ce := node.AsConditionalExpression() | ||
conditionParenthesized := p.parenthesizeExpressionForNoAsi(ce.Condition) | ||
if conditionParenthesized != ce.Condition { | ||
return p.updateConditionalExpression(ce, conditionParenthesized) | ||
} | ||
|
||
case ast.KindAsExpression: | ||
ae := node.AsAsExpression() | ||
exprParenthesized := p.parenthesizeExpressionForNoAsi(ae.Expression) | ||
if exprParenthesized != ae.Expression { | ||
return p.updateAsExpression(ae, exprParenthesized) | ||
} | ||
|
||
case ast.KindSatisfiesExpression: | ||
se := node.AsSatisfiesExpression() | ||
exprParenthesized := p.parenthesizeExpressionForNoAsi(se.Expression) | ||
if exprParenthesized != se.Expression { | ||
return p.updateSatisfiesExpression(se, exprParenthesized) | ||
} | ||
|
||
case ast.KindNonNullExpression: | ||
nne := node.AsNonNullExpression() | ||
exprParenthesized := p.parenthesizeExpressionForNoAsi(nne.Expression) | ||
if exprParenthesized != nne.Expression { | ||
return p.updateNonNullExpression(nne, exprParenthesized) | ||
} | ||
} | ||
|
||
return node | ||
} | ||
|
||
// Helper functions to create/update nodes with parenthesized sub-expressions | ||
|
||
func (p *Printer) createParenthesizedExpressionForNoAsi(node *ast.Expression) *ast.Expression { | ||
// Create a parenthesized expression to force wrapping | ||
return p.emitContext.Factory.NewParenthesizedExpression(node) | ||
Check failure on line 3101 in internal/printer/printer.go
|
||
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, this isn't how parenthesizing works in the corsa printer. The printer doesn't build nodes - it just emits the parens into the output where needed. See the 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. You're right that this creates new AST nodes during emission, which differs from the typical corsa approach. However, I'm having trouble understanding how to implement this without node creation while still achieving the correct output (where parens wrap only the specific sub-expression that has the comment, not the entire expression). For example, with Could you provide guidance on the corsa approach for this? Would it involve:
The current implementation does produce the correct output and all tests pass, but I want to make sure it's architecturally sound. |
||
} | ||
|
||
func (p *Printer) updatePartiallyEmittedExpression(node *ast.PartiallyEmittedExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdatePartiallyEmittedExpression(node, expression) | ||
} | ||
|
||
func (p *Printer) updatePropertyAccessExpression(node *ast.PropertyAccessExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdatePropertyAccessExpression(node, expression, node.QuestionDotToken, node.Name()) | ||
} | ||
|
||
func (p *Printer) updateElementAccessExpression(node *ast.ElementAccessExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateElementAccessExpression(node, expression, node.QuestionDotToken, node.ArgumentExpression) | ||
} | ||
|
||
func (p *Printer) updateCallExpression(node *ast.CallExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateCallExpression(node, expression, node.QuestionDotToken, node.TypeArguments, node.Arguments) | ||
} | ||
|
||
func (p *Printer) updateTaggedTemplateExpression(node *ast.TaggedTemplateExpression, tag *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateTaggedTemplateExpression(node, tag, node.QuestionDotToken, node.TypeArguments, node.Template) | ||
} | ||
|
||
func (p *Printer) updatePostfixUnaryExpression(node *ast.PostfixUnaryExpression, operand *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdatePostfixUnaryExpression(node, operand) | ||
} | ||
|
||
func (p *Printer) updateBinaryExpression(node *ast.BinaryExpression, left *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateBinaryExpression(node, node.Modifiers(), left, node.Type, node.OperatorToken, node.Right) | ||
} | ||
|
||
func (p *Printer) updateConditionalExpression(node *ast.ConditionalExpression, condition *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateConditionalExpression(node, condition, node.QuestionToken, node.WhenTrue, node.ColonToken, node.WhenFalse) | ||
} | ||
|
||
func (p *Printer) updateAsExpression(node *ast.AsExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateAsExpression(node, expression, node.Type) | ||
} | ||
|
||
func (p *Printer) updateSatisfiesExpression(node *ast.SatisfiesExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateSatisfiesExpression(node, expression, node.Type) | ||
} | ||
|
||
func (p *Printer) updateNonNullExpression(node *ast.NonNullExpression, expression *ast.Expression) *ast.Expression { | ||
return p.emitContext.Factory.UpdateNonNullExpression(node, expression) | ||
} | ||
|
||
func (p *Printer) emitExpression(node *ast.Expression, precedence ast.OperatorPrecedence) { | ||
|
@@ -3006,9 +3179,9 @@ | |
// Expressions | ||
case ast.KindArrayLiteralExpression: | ||
p.emitArrayLiteralExpression(node.AsArrayLiteralExpression()) | ||
case ast.KindObjectLiteralExpression: | ||
p.emitObjectLiteralExpression(node.AsObjectLiteralExpression()) | ||
case ast.KindPropertyAccessExpression: | ||
p.emitPropertyAccessExpression(node.AsPropertyAccessExpression()) | ||
case ast.KindElementAccessExpression: | ||
p.emitElementAccessExpression(node.AsElementAccessExpression()) | ||
|
@@ -3020,9 +3193,9 @@ | |
p.emitTaggedTemplateExpression(node.AsTaggedTemplateExpression()) | ||
case ast.KindTypeAssertionExpression: | ||
p.emitTypeAssertionExpression(node.AsTypeAssertion()) | ||
case ast.KindParenthesizedExpression: | ||
p.emitParenthesizedExpression(node.AsParenthesizedExpression()) | ||
case ast.KindFunctionExpression: | ||
p.emitFunctionExpression(node.AsFunctionExpression()) | ||
case ast.KindArrowFunction: | ||
p.emitArrowFunction(node.AsArrowFunction()) | ||
|
@@ -3033,7 +3206,7 @@ | |
case ast.KindVoidExpression: | ||
p.emitVoidExpression(node.AsVoidExpression()) | ||
case ast.KindAwaitExpression: | ||
p.emitAwaitExpression(node.AsAwaitExpression()) | ||
case ast.KindPrefixUnaryExpression: | ||
p.emitPrefixUnaryExpression(node.AsPrefixUnaryExpression()) | ||
case ast.KindPostfixUnaryExpression: | ||
|
This file was deleted.
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.
These temp names....
pee
,pae
,pue
? Legibility out the window with those for sure.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.
Fixed the variable names to be more descriptive (e.g.,
partiallyEmitted
,propertyAccess
,binaryExpr
, etc.) in commit [SHORT_HASH].