Sequence expressions not formatted "correctly"

[CharlieEriksen]

Version used

v3.0.2

Describe the bug

The pretty formatter handles a sequence expressions in a way that makes it hard to read. I tried looking at how this is handled, but I've gotta admit that the printing logic is a bit of voodoo for me.

To Reproduce

Consider this code:

Object.defineProperty(e, "__esModule", { value: !0 }),
  (e.STORE_NAME_KEY_VALUE = "keyValue"),
  (e.STORE_NAME_MESSAGE_LOG = "messages"),
  (e.STORE_NAME_MAIN_LOG = "log"),
  (e.STORE_NAME_INBOX_MESSAGES = "inboxMessages"),
  (e.KEY_PATH_BASE_INCREMENT = "id");

When parsed and pretty-formatted with the default options, it returns this instead:

    Object.defineProperty(e, "__esModule", { value: !0 }), e.STORE_NAME_KEY_VALUE = "keyValue", e.STORE_NAME_MESSAGE_LOG = "messages", e.STORE_NAME_MAIN_LOG = "log", e.STORE_NAME_INBOX_MESSAGES = "inboxMessages", e.KEY_PATH_BASE_INCREMENT = "id";

Note that it's stripped the parentheses, and it's now all on a single line, which is much harder to read.

Expected behavior

It should most likely retain the parentheses around each expression, and when the expression list is above a certain length, it should put each on its own line.

[adams85]

It should most likely retain the parentheses around each expression

Esprima has no way to represent parenthesized expressions in the AST, so this is kinda impossible currently.

when the expression list is above a certain length, it should put each on its own line.

This can be achieved relatively easy though. But there are multiple ways to format a long sequence expr, so it's not something that Esprima should support out of the box IMO.

However, the AST to JSON conversion was designed to be customizable (via inheritance), so it shouldn't be too hard to create a custom formatter with the desired formatting for sequence exprs.

The logic of the AST to JSON conversion is not something trivial indeed, however you don't need to familiarize yourself with the whole thing to achieve what you want.

It's enough to know that the conversion logic has two main components:

• A visitor (AstToJavaScriptConverter), which traverses the AST and feeds the nodes to the formatter along with some context that is necessary for the formatter to do its job.
• A formatter (JavaScriptTextFormatter and its subclasses), which produces the actual output, based on the info it receives from the visitor.

So, if you decide to take a stab at it, I suggest you check out the repo and debug through this method, using a simple sequence expression as input:

esprima-dotnet/src/Esprima/Utils/AstToJavaScriptConverter.cs

Lines 1484 to 1492 in 144ed66

	protected internal override object? VisitSequenceExpression(SequenceExpression sequenceExpression)
	{
	_writeContext.SetNodeProperty(nameof(sequenceExpression.Expressions), static node => ref node.As<SequenceExpression>().Expressions);
	VisitExpressionList(in sequenceExpression.Expressions, static (@this, expression, index, _) =>
	s_getCombinedSubExpressionFlags(@this, expression, SubExpressionFlags(@this.ExpressionNeedsBracketsInList(expression), isLeftMost: index == 0)));
	return sequenceExpression;
	}

This will help you understand what formatter methods the visitor calls and what context they pass to them. Then you can move on to subclassing KnRJavaScriptTextFormatter, override the necessary methods and add the extra logic for the custom formatting.

These pieces of code can give you further pointers:

• Subclassing:

  esprima-dotnet/test/Esprima.Tests/AstToJavascriptTests.cs

  Lines 11 to 44 in 144ed66

  	private record class CustomCompactJavaScriptTextWriterOptions : JavaScriptTextWriterOptions
  	{
  	protected internal override JavaScriptTextWriter CreateWriter(TextWriter writer) => new CustomCompactJavaScriptTextWriter(writer, this);
  	}
  	private sealed class CustomCompactJavaScriptTextWriter : JavaScriptTextWriter
  	{
  	public CustomCompactJavaScriptTextWriter(TextWriter writer, CustomCompactJavaScriptTextWriterOptions options) : base(writer, options) { }
  	public override void EndStatement(StatementFlags flags, ref WriteContext context)
  	{
  	if (flags.HasFlagFast(StatementFlags.NeedsSemicolon) || ShouldTerminateStatementAnyway(context.GetNodePropertyValue<Statement>(), flags, ref context))
  	{
  	WritePunctuator(";", TokenFlags.Trailing | TokenFlags.TrailingSpaceRecommended, ref context);
  	}
  	}
  	public override void EndStatementListItem(int index, int count, StatementFlags flags, ref WriteContext context)
  	{
  	if (flags.HasFlagFast(StatementFlags.NeedsSemicolon) || ShouldTerminateStatementAnyway(context.GetNodePropertyListValue<Statement>()[index], flags, ref context))
  	{
  	WritePunctuator(";", TokenFlags.Trailing | TokenFlags.TrailingSpaceRecommended, ref context);
  	}
  	}
  	private bool ShouldTerminateStatementAnyway(Statement statement, StatementFlags flags, ref WriteContext context)
  	{
  	return statement.Type switch
  	{
  	Nodes.DoWhileStatement => true,
  	_ => false
  	};
  	}
  	}

• Using the custom formatter:

  esprima-dotnet/test/Esprima.Tests/AstToJavascriptTests.cs

  Line 73 in 144ed66

  var code = program.ToJavaScriptString(s_customCompactWriterOptions, AstToJavaScriptOptions.Default);

• This is how breaking long array expressions into multiple lines are implemented: https://github.com/sebastienros/esprima-dotnet/blob/main/src/Esprima/Utils/JavaScriptTextFormatter.cs#L482-L500

[adams85]

Just an FYI/FWIW: I released a new JS parser that is capable of preserving parentheses around expressions.

It can solve your issue partially as its feature set and public API is pretty similar to Esprima's, so migration is relatively easy. Still no formatting options for long sequence expressions though.