Skip to content

Commit

Permalink
Fix a few edge cases in AST to JS conversion (#413)
Browse files Browse the repository at this point in the history 
* Correct formatting of logical expressions which mix nullish coalescing and logical AND/OR operations

* Correct formatting of plus/minus binary operations having a right side that starts with an ambiguous punctuator

* Correct formatting of further ambiguous operator sequences + improve WriteContext for JavaScriptTextWriter
  • Loading branch information
@adams85
adams85 authored Nov 4, 2023
1 parent 7fdad18 commit 74035ad
Show file tree
Hide file tree
Showing 9 changed files with 447 additions and 62 deletions.
7 changes: 2 additions & 5 deletions src/Esprima/Ast/Literal.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ namespace Esprima.Ast;
[VisitableNode(SealOverrideMethods = true)]
public partial class Literal : Expression
{
private static readonly object s_boxedTrue = true;
private static readonly object s_boxedFalse = false;

internal Literal(TokenType tokenType, object? value, string raw) : base(Nodes.Literal)
{
TokenType = tokenType;
Expand All @@ -21,7 +18,7 @@ public Literal(string value, string raw) : this(TokenType.StringLiteral, value,
{
}

public Literal(bool value, string raw) : this(TokenType.BooleanLiteral, value ? s_boxedTrue : s_boxedFalse, raw)
public Literal(bool value, string raw) : this(TokenType.BooleanLiteral, value.AsCachedObject(), raw)
{
}

Expand All @@ -42,7 +39,7 @@ public Literal(string raw) : this(TokenType.NullLiteral, null, raw)
public string Raw { [MethodImpl(MethodImplOptions.AggressiveInlining)] get; }

public string? StringValue { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => TokenType == TokenType.StringLiteral ? (string) Value! : null; }
public bool? BooleanValue { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => TokenType == TokenType.BooleanLiteral ? ReferenceEquals(Value, s_boxedTrue) : null; }
public bool? BooleanValue { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => TokenType == TokenType.BooleanLiteral ? ReferenceEquals(Value, ParserExtensions.s_boxedTrue) : null; }
public double? NumericValue { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => TokenType == TokenType.NumericLiteral ? (double) Value! : null; }
public Regex? RegexValue { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => TokenType == TokenType.RegularExpression ? (Regex?) Value : null; }
public BigInteger? BigIntValue { [MethodImpl(MethodImplOptions.AggressiveInlining)] get => TokenType == TokenType.BigIntLiteral ? (BigInteger) Value! : null; }
Expand Down
5 changes: 5 additions & 0 deletions src/Esprima/ParserExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ internal static partial class ParserExtensions
// old framework doesn't know this flag
private const int MethodImplOptionsAggressiveOptimization = 512;

internal static readonly object s_boxedTrue = true;
internal static readonly object s_boxedFalse = false;

public static object AsCachedObject(this bool value) => value ? s_boxedTrue : s_boxedFalse;

private static readonly string[] s_charToString = new string[256];

static ParserExtensions()
Expand Down
5 changes: 4 additions & 1 deletion src/Esprima/Utils/AstToJavaScriptConverter.Helpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ protected void VisitStatementListItem(Statement statement, int index, int count,
var originalStatementFlags = _currentStatementFlags;
_currentStatementFlags = getCombinedFlags(this, statement, index, count);

_writeContext.SetNodePropertyItemIndex(index);
Writer.StartStatementListItem(index, count, (JavaScriptTextWriter.StatementFlags) _currentStatementFlags, ref _writeContext);
Visit(statement);
Writer.EndStatementListItem(index, count, (JavaScriptTextWriter.StatementFlags) _currentStatementFlags, ref _writeContext);
Expand Down Expand Up @@ -209,6 +210,7 @@ protected void VisitExpressionListItem(Expression expression, int index, int cou
var originalExpressionFlags = _currentExpressionFlags;
_currentExpressionFlags = getCombinedFlags(this, expression, index, count);

_writeContext.SetNodePropertyItemIndex(index);
Writer.StartExpressionListItem(index, count, (JavaScriptTextWriter.ExpressionFlags) _currentExpressionFlags, ref _writeContext);
Visit(expression);
Writer.EndExpressionListItem(index, count, (JavaScriptTextWriter.ExpressionFlags) _currentExpressionFlags, ref _writeContext);
Expand All @@ -231,7 +233,7 @@ private void VisitImportAttributes(in NodeList<ImportAttribute> attributes)

private void VisitExportOrImportSpecifierIdentifier(Expression identifierExpression)
{
if (identifierExpression is Identifier { Name: "default" } identifier)
if (identifierExpression is Identifier { Name: "default" })
{
Writer.WriteKeyword("default", ref _writeContext);
}
Expand Down Expand Up @@ -429,6 +431,7 @@ protected void VisitAuxiliaryNodeListItem<TNode>(TNode node, int index, int coun
var originalAuxiliaryNodeContext = _currentAuxiliaryNodeContext;
_currentAuxiliaryNodeContext = getNodeContext(this, node, index, count);

_writeContext.SetNodePropertyItemIndex(index);
Writer.StartAuxiliaryNodeListItem<TNode>(index, count, separator, _currentAuxiliaryNodeContext, ref _writeContext);
Visit(node);
Writer.EndAuxiliaryNodeListItem<TNode>(index, count, separator, _currentAuxiliaryNodeContext, ref _writeContext);
Expand Down
71 changes: 38 additions & 33 deletions src/Esprima/Utils/AstToJavaScriptConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Esprima.Utils;
public partial class AstToJavaScriptConverter : AstVisitor
{
// Notes for maintainers:
// Don't visit nodes directly (by calling Visit) unless it's necessary for some special reason (but in that case you'll need to setup the context of the visitation manually!)
// Don't visit nodes by directly calling Visit unless it's necessary for some special reason (but in that case you'll need to setup the context of the visitation manually!)
// For examples of special reason, see VisitArrayExpression, VisitObjectExpression, VisitImport, etc. In usual cases just use the following predefined visitation helper methods:
// * Visit statements using VisitStatement / VisitStatementList.
// * Visit expressions using VisitRootExpression and sub-expressions (expressions inside another expression) using VisitSubExpression / VisitSubExpressionList.
Expand Down Expand Up @@ -124,14 +124,15 @@ public void Convert(Node node)

if (element is not null)
{
VisitExpressionListItem(element, i, arrayExpression.Elements.Count, static (@this, expression, index, _) =>
VisitExpressionListItem(element, i, arrayExpression.Elements.Count, static (@this, expression, _, _) =>
s_getCombinedSubExpressionFlags(@this, expression, SubExpressionFlags(@this.ExpressionNeedsBracketsInList(expression), isLeftMost: false)));
}
else
{
var originalExpressionFlags = _currentExpressionFlags;
_currentExpressionFlags = PropagateExpressionFlags(SubExpressionFlags(needsBrackets: false, isLeftMost: false));

_writeContext.SetNodePropertyItemIndex(i);
Writer.StartExpressionListItem(i, arrayExpression.Elements.Count, (JavaScriptTextWriter.ExpressionFlags) _currentExpressionFlags, ref _writeContext);
Writer.EndExpressionListItem(i, arrayExpression.Elements.Count, (JavaScriptTextWriter.ExpressionFlags) _currentExpressionFlags, ref _writeContext);

Expand Down Expand Up @@ -175,6 +176,7 @@ public void Convert(Node node)
var originalAuxiliaryNodeContext = _currentAuxiliaryNodeContext;
_currentAuxiliaryNodeContext = null;

_writeContext.SetNodePropertyItemIndex(i);
Writer.StartAuxiliaryNodeListItem<Node?>(i, arrayPattern.Elements.Count, separator: ",", _currentAuxiliaryNodeContext, ref _writeContext);
VisitRootExpression(expression, RootExpressionFlags(needsBrackets: ExpressionNeedsBracketsInList(expression)));
Writer.EndAuxiliaryNodeListItem<Node?>(i, arrayPattern.Elements.Count, separator: ",", _currentAuxiliaryNodeContext, ref _writeContext);
Expand All @@ -187,6 +189,7 @@ public void Convert(Node node)
var originalAuxiliaryNodeContext = _currentAuxiliaryNodeContext;
_currentAuxiliaryNodeContext = null;

_writeContext.SetNodePropertyItemIndex(i);
Writer.StartAuxiliaryNodeListItem<Node?>(i, arrayPattern.Elements.Count, separator: ",", _currentAuxiliaryNodeContext, ref _writeContext);
Writer.EndAuxiliaryNodeListItem<Node?>(i, arrayPattern.Elements.Count, separator: ",", _currentAuxiliaryNodeContext, ref _writeContext);

Expand Down Expand Up @@ -256,7 +259,7 @@ public void Convert(Node node)
var op = AssignmentExpression.GetAssignmentOperatorToken(assignmentExpression.Operator);

_writeContext.SetNodeProperty(nameof(assignmentExpression.Operator), static node => node.As<AssignmentExpression>().Operator);
Writer.WritePunctuator(op, TokenFlags.InBetween | TokenFlags.SurroundingSpaceRecommended, ref _writeContext);
Writer.WritePunctuator(op, TokenFlags.InBetween | TokenFlags.SurroundingSpaceRecommended | TokenFlags.IsAssignmentOperator, ref _writeContext);

// AssignmentExpression is not a real binary operation because its left side is not an expression.
var rightNeedsBrackets = GetOperatorPrecedence(assignmentExpression, out _) > GetOperatorPrecedence(assignmentExpression.Right, out _);
Expand Down Expand Up @@ -307,14 +310,19 @@ public void Convert(Node node)
protected internal override object? VisitBinaryExpression(BinaryExpression binaryExpression)
{
var operationFlags = BinaryOperandsNeedBrackets(binaryExpression, binaryExpression.Left, binaryExpression.Right);

// The operand of unary operators cannot be an exponentiation without grouping.
// E.g. -1 ** 2 is syntactically unambiguous but the language requires (-1) ** 2 instead.
if (!operationFlags.HasFlagFast(BinaryOperationFlags.LeftOperandNeedsBrackets) &&
binaryExpression.Operator == BinaryOperator.Exponentiation &&
binaryExpression.Left is UnaryExpression leftUnaryExpression)
{
operationFlags |= BinaryOperationFlags.LeftOperandNeedsBrackets;
if (!operationFlags.HasFlagFast(BinaryOperationFlags.LeftOperandNeedsBrackets))
{
if (
// The operand of unary operators cannot be an exponentiation without grouping.
// E.g. -1 ** 2 is syntactically unambiguous but the language requires (-1) ** 2 instead.
binaryExpression.Operator == BinaryOperator.Exponentiation && binaryExpression.Left.Type == Nodes.UnaryExpression ||
// Logical expressions which mix nullish coalescing and logical AND/OR operators (e.g. (a ?? b) || c or (a && b) ?? c)
// needs to be parenthesized despite the operator of the parenthesized sub-expression having the same or higher precedence.
binaryExpression.Operator == BinaryOperator.NullishCoalescing && binaryExpression.Left is LogicalExpression { Operator: BinaryOperator.LogicalAnd or BinaryOperator.LogicalOr } ||
binaryExpression.Operator is BinaryOperator.LogicalAnd or BinaryOperator.LogicalOr && binaryExpression.Left is LogicalExpression { Operator: BinaryOperator.NullishCoalescing })
{
operationFlags |= BinaryOperationFlags.LeftOperandNeedsBrackets;
}
}

_writeContext.SetNodeProperty(nameof(binaryExpression.Left), static node => node.As<BinaryExpression>().Left);
Expand All @@ -329,16 +337,17 @@ public void Convert(Node node)
}
else
{
Writer.WritePunctuator(op, TokenFlags.InBetween | TokenFlags.SurroundingSpaceRecommended, ref _writeContext);
Writer.WritePunctuator(op, TokenFlags.InBetween | TokenFlags.SurroundingSpaceRecommended | TokenFlags.IsBinaryOperator, ref _writeContext);

// Cases like 1 + (+x) must be disambiguated with brackets.
if (!operationFlags.HasFlagFast(BinaryOperationFlags.RightOperandNeedsBrackets) &&
binaryExpression.Right is UnaryExpression rightUnaryExpression &&
rightUnaryExpression.Prefix &&
op[op.Length - 1] is '+' or '-' &&
op[op.Length - 1] == UnaryExpression.GetUnaryOperatorToken(rightUnaryExpression.Operator)[0])
if (!operationFlags.HasFlagFast(BinaryOperationFlags.RightOperandNeedsBrackets))
{
operationFlags |= BinaryOperationFlags.RightOperandNeedsBrackets;
if (
// Logical expressions which mix nullish coalescing and logical AND operators (e.g. a ?? (b && c))
// needs to be parenthesized despite the operator of the parenthesized sub-expression having higher precedence.
binaryExpression.Operator == BinaryOperator.NullishCoalescing && binaryExpression.Right is LogicalExpression { Operator: BinaryOperator.LogicalAnd })
{
operationFlags |= BinaryOperationFlags.RightOperandNeedsBrackets;
}
}
}

Expand Down Expand Up @@ -992,7 +1001,6 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
Writer.WritePunctuator(":", TokenFlags.Trailing | TokenFlags.TrailingSpaceRecommended, ref _writeContext);

_writeContext.SetNodeProperty(nameof(importAttribute.Value), static node => node.As<ImportAttribute>().Value);

VisitRootExpression(importAttribute.Value, RootExpressionFlags(needsBrackets: false));

return importAttribute;
Expand Down Expand Up @@ -1287,6 +1295,7 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
var originalAuxiliaryNodeContext = _currentAuxiliaryNodeContext;
_currentAuxiliaryNodeContext = null;

_writeContext.SetNodePropertyItemIndex(i);
Writer.StartAuxiliaryNodeListItem<Node>(i, objectExpression.Properties.Count, separator: ",", _currentAuxiliaryNodeContext, ref _writeContext);
VisitRootExpression(spreadElement, RootExpressionFlags(needsBrackets: ExpressionNeedsBracketsInList(spreadElement)));
Writer.EndAuxiliaryNodeListItem<Node>(i, objectExpression.Properties.Count, separator: ",", _currentAuxiliaryNodeContext, ref _writeContext);
Expand Down Expand Up @@ -1593,18 +1602,22 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
Writer.WritePunctuator("`", TokenFlags.Leading, ref _writeContext);

TemplateElement quasi;
for (var i = 0; !(quasi = templateLiteral.Quasis[i]).Tail; i++)
int i;
for (i = 0; !(quasi = templateLiteral.Quasis[i]).Tail; i++)
{
_writeContext.SetNodeProperty(nameof(templateLiteral.Quasis), static node => ref node.As<TemplateLiteral>().Quasis);
_writeContext.SetNodePropertyItemIndex(i);
VisitAuxiliaryNode(quasi);

_writeContext.SetNodeProperty(nameof(templateLiteral.Expressions), static node => ref node.As<TemplateLiteral>().Expressions);
_writeContext.SetNodePropertyItemIndex(i);
Writer.WritePunctuator("${", TokenFlags.Leading, ref _writeContext);
VisitRootExpression(templateLiteral.Expressions[i], RootExpressionFlags(needsBrackets: false));
Writer.WritePunctuator("}", TokenFlags.Trailing, ref _writeContext);
}

_writeContext.SetNodeProperty(nameof(templateLiteral.Quasis), static node => ref node.As<TemplateLiteral>().Quasis);
_writeContext.SetNodePropertyItemIndex(i);
VisitAuxiliaryNode(quasi);

Writer.WritePunctuator("`", TokenFlags.Trailing, ref _writeContext);
Expand Down Expand Up @@ -1675,17 +1688,9 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
}
else
{
Writer.WritePunctuator(op, TokenFlags.Leading, ref _writeContext);

// Cases like +(+x) or +(++x) must be disambiguated with brackets.
if (!argumentNeedsBrackets &&
unaryExpression.Argument is UnaryExpression argumentUnaryExpression &&
argumentUnaryExpression.Prefix &&
op[op.Length - 1] is '+' or '-' &&
op[op.Length - 1] == UnaryExpression.GetUnaryOperatorToken(argumentUnaryExpression.Operator)[0])
{
argumentNeedsBrackets = true;
}
// Cases like +(+x) or +(++x) must be disambiguated. However, this can be done in multiple ways: e.g. + +x or +(+x).
// It depends on the formatting which way to choose, so disambiguation must be implemented by JavaScriptTextWriter.
Writer.WritePunctuator(op, TokenFlags.Leading | TokenFlags.IsUnaryOperator, ref _writeContext);
}

_writeContext.SetNodeProperty(nameof(unaryExpression.Argument), static node => node.As<UnaryExpression>().Argument);
Expand All @@ -1697,7 +1702,7 @@ unaryExpression.Argument is UnaryExpression argumentUnaryExpression &&
VisitSubExpression(unaryExpression.Argument, SubExpressionFlags(argumentNeedsBrackets, isLeftMost: true));

_writeContext.SetNodeProperty(nameof(unaryExpression.Operator), static node => node.As<UnaryExpression>().Operator);
Writer.WritePunctuator(op, TokenFlags.Trailing, ref _writeContext);
Writer.WritePunctuator(op, TokenFlags.Trailing | TokenFlags.IsUnaryOperator, ref _writeContext);
}

return unaryExpression;
Expand Down
45 changes: 45 additions & 0 deletions src/Esprima/Utils/JavaScriptTextFormatter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ protected virtual void WriteWhiteSpaceBetweenTokenAndPunctuator(string value, To
{
WriteSpace();
}
else
{
WriteRequiredSpaceBetweenTokenAndPunctuator(value, flags, ref context);
}
}

protected override void StartPunctuator(string value, TokenFlags flags, ref WriteContext context)
Expand Down Expand Up @@ -439,6 +443,38 @@ protected virtual bool ShouldTerminateStatementAnyway(Statement statement, State
};
}

public override void StartExpression(ExpressionFlags flags, ref WriteContext context)
{
if (!flags.HasFlagFast(ExpressionFlags.NeedsBrackets))
{
var expression = !context.NodePropertyHasListValue
? context.GetNodePropertyValue<Expression>()
: context.GetNodePropertyListItem<Expression>();
var forceBrackets = ShouldWrapExpressionInBracketsAnyway(expression, flags, ref context);
context._additionalDataSlot.PrimaryData = forceBrackets.AsCachedObject();
if (forceBrackets)
{
flags |= ExpressionFlags.NeedsBrackets;
}
}

base.StartExpression(flags, ref context);
}

public override void EndExpression(ExpressionFlags flags, ref WriteContext context)
{
if (!flags.HasFlagFast(ExpressionFlags.NeedsBrackets))
{
var forceBrackets = (bool) context._additionalDataSlot.PrimaryData!;
if (forceBrackets)
{
flags |= ExpressionFlags.NeedsBrackets;
}
}

base.EndExpression(flags, ref context);
}

public override void StartExpressionListItem(int index, int count, ExpressionFlags flags, ref WriteContext context)
{
if (context.Node.Type == Nodes.ArrayExpression && count >= MultiLineArrayLiteralThreshold)
Expand All @@ -459,6 +495,15 @@ public override void EndExpressionListItem(int index, int count, ExpressionFlags
}
}

protected virtual bool ShouldWrapExpressionInBracketsAnyway(Expression expression, ExpressionFlags flags, ref WriteContext context)
{
return
LastTokenType == TokenType.Punctuator &&
LastTokenFlags.HasFlagFast(TokenFlags.IsUnaryOperator) &&
expression is UnaryExpression { Prefix: true } unaryExpression &&
!char.IsLetter(UnaryExpression.GetUnaryOperatorToken(unaryExpression.Operator)[0]);
}

public override void StartAuxiliaryNodeListItem<T>(int index, int count, string separator, object? nodeContext, ref WriteContext context)
{
if (typeof(T) == typeof(SwitchCase) ||
Expand Down
Loading

0 comments on commit 74035ad

Please sign in to comment.