RFC: New "break this group if you have to, but try to break its parents first" doc command?

[suchipi]

I've seen a lot of issues opened lately where a single item in braces, parens, angle brackets, or square brackets gets left on a line alone, and it looks odd. In each instance, it seems that there is an alternative formatting option that looks better and still fits within the line limit.

An idea popped into my head the other day for a way we could address this- if we could consider breaking of certain groups "undesirable", then we could take places where people try to avoid putting breaks (TypeParameterDeclaration, computed MemberExpression property, arrow function expression parameters, etc) and use a new "undesirable-to-break group" command there.

The idea behind the "undesirable-to-break group" is, when a break occurs, and we're iterating up through the parents to break them, if we find an undesirable-to-break group, we skip over it and try breaking the next parent group, and see if that makes everything fit within the desired print-width. If we get all the way up to <wherever we decide to stop- the root node, or a statement context?>, and the part of the code we were formatting still isn't under the desired print width, then we iterate back down over the undesirable-to-break groups, breaking the outermost ones first, and stopping once everything fits under the desired print width (or we've broken all the groups).

It obviously introduces some time complexity, but I think it might be worth it. Does anyone have thoughts?

[lydell]

It sounds like a good idea. But I'm worried about potential performance problems, too. One way of going forward is to make a proof of concept PR, and add some crazy nested code as a performance test.

[azz]

Maybe the concept of a "strong group"?

Before:

[
  "new ",
  "Foo",
  group(["<", indent([softline, "T"]), softline, ">"]),
  group([
    "(",
    indent([softline, "1", ",", line, "2", ",", line, "3"]),
    softline,
    ")"
  ]),
  ";",
  hardline,
  breakParent
];

After:

[
  "new ",
  "Foo",
  strongGroup(["<", indent([softline, "T"]), softline, ">"]),
  group([
    "(",
    indent([softline, "1", ",", line, "2", ",", line, "3"]),
    softline,
    ")"
  ]),
  ";",
  hardline,
  breakParent
];

[azz]

I think it'll be tricky to get right... But if I'm certainly interested.

[karl]

Really interesting idea, would love to see a proof of concept!

[suchipi]

I've been trying to find a free weekend to work on this for a while; hopefully I'll have some time this weekend.

[bakkot]

This might be a solution for #2482: instead of a = b being a single unbreakable unit, have it be a "strong group" consisting of a = and b.

Edit: though, thinking about it more, probably not quite; see comment there.

[bakkot]

Also, this is kind of in the direction of dartfmt; if you haven't read that essay it's worth reading.

[suchipi]

With the holiday season coming up, it will probably be a while before I'll have time to implement a proof-of-concept for this. If anyone else wants to take a stab at it, they are welcome to.

[duailibe]

@suchipi I have some free time in the following weeks but I'm not exactly sure how to implement this. Do you have any general advice?

[suchipi]

No; I'm not familiar with the doc printer, so I was gonna dig in and just try to figure stuff out. But I think the implementation should involve two new doc nodes: strongGroup and strongGroupBoundary. A strong group shouldn't break unless we have broken every other group or strong group up to the nearest strong group boundary and code within that strong group boundary still doesn't fit under the print width. There should probably be strong group boundaries around each statement, and around the whole program. And we'll probably end up putting them in a few other places eventually, too.

[vjeux]

I'm not sure I understand this issue. "break this group if you have to, but try to break its parents first" is how the prettier algorithm works already. It always breaks the outermost group first.

I believe that the issues you're seeing is that if there are two groups next to each other, prettier algorithm will break the last one which is not always what you want. We sometimes want to control which one to break first.

A technique that we use is to remove a group in those cases. A good example is function arguments vs return types.

With a naive implementation, the following would break like this:

function f(a: int, b: int): React.Component<Props, State> {}

// Would break like this
function f(a: int, b: int): React.Component<
  Props,
  State,
> {}

// But you want to break like this
function f(
  a: int,
  b: int,
): React.Component<Props, State> {}

The doc structure looks something like this:

group([
  "function f(",
  group(comma(["a: int", "b: int"])),
  "): React.Component<",
  group(comma(["Props", "State"])),
  "> {}",
])

If you remove the first group, then it's going to have the behavior that you want:

group([
  "function f(",
  comma(["a: int", "b: int"]), // <-- no more group here
  "): React.Component<",
  group(comma(["Props", "State"])),
  "> {}",
])

I don't know if it applies to this exact situation but this is a handy tool to have on the toolbox to fix those kind of edge cases.

[vjeux]

I looked at the issues you linked and outside of the one around lambdas, they are all about member chains. This is by far the most complicated piece of prettier logic. What I would recommend is to try and make those examples work and see what tests break. We have a really good coverage at this point.

We may or may not need new primitives for it but we need to understand what trade-offs have been made. You’ll likely find examples that look very similar based on the ast that in some cases you would print one way and some other cases you would print another way and it’s hard to automatically chose which to use.

[vjeux]

For the lambda expansion, we may want to introduce a new concept to prettier. I'm implementing a pretty printer for a different language and I am experimenting with a different approach: #3376 (comment)

So far it looks promising but I still find edge cases so it's not yet guaranteed to work.

[suchipi]

@vjeux thanks for the input. It appears you are right, this happens more frequently with sibling groups than parent/child groups.

The algorithm you mentioned in that comment sounds perfect for this kind of stuff- excited about it, hope it pans out.

[octogonz]

This proposal hasn't been updated in a while. Did we (mostly) converge on a design, but nobody has stepped up to implement it? Or is the idea still in the speculative stages? Just curious about the status.

[alexander-akait]

@octogonz it is just idea, anyway you can send a PR with implementation and we review this

[thorn0]

"Break this group if you have to, but try to break its parents first" is indeed how Prettier already works, but it's just an unfortunate wording. This problem:

a single item in braces, parens, angle brackets, or square brackets gets left on a line alone, and it looks odd. In each instance, it seems that there is an alternative formatting option that looks better and still fits within the line limit.

is a real issue and still doesn't have a solution. E.g., that's how it manifested itself in my attempts to fix assignments (playground):

{
  {
    // (1) -- unbalanced
    //   - big visual distance between the fn name and the arg
    //   - the lone short argument on its own line looks bad
    result.typeParameters = this.convertTSTypeParametersToTypeParametearsFoo1(
      node
    );

    // (2) -- much better than (1)
    result.typeParameters =
      this.convertTSTypeParametersToTypeParametearsFoo1(node);

    // (3) -- but with a longer argument, (2) breaks and becomes bad:
    //   takes an extra line, extra indentation looks unjustified
    result.typeParameters =
      this.convertTSTypeParametersToTypeParametearsFoo1(
        convertTSTypeParametersToTypeParametearsFoo1
      );

    // (4) -- better than (3), but how can we algorithmically distinguish this case from (1)?
    result.typeParameters = this.convertTSTypeParametersToTypeParametearsFoo1(
      convertTSTypeParametersToTypeParametearsFoo1
    );
  }
}