Router: disallow breaks in args in binpack=oneline

[kitbellew]

Fix bug in binpack=oneline implementation:
- handling of the first arg is different from subsequent ones (which
uses a single-line block no-split policy)
- the single-line block policy must also exclude multiline strings.

[kitbellew]

@sjrd @ekrich please verify that this addresses: scala-js/scala-js#4522 (comment)

[sjrd]

Yes, that looks perfect. Thank you!

[tgodzik]

Just one question, but otherwise LGTM

[kitbellew]

@sjrd let me do another iteration, so that we don't have to come back to this later.

first, can you describe the binpack rule for definitions? so far, we focused on method calls, but what about parameters of a def?

second, coming back to one-argument case:

// ok or not?
val foo = foo(bar &&
   bar)
val foo = foo(foo(bar &&
   bar))
val foo = foo(foo("""
   bar"""))
val foo = foo(foo(bar,
   bar))

@tgodzik made a really good observation about the test, and while i explained it away, i don't think that that was the intention, since the code forces a single line block only until a comma, which means multiple arguments. so, just want to be very sure about this.

[kitbellew]

@sjrd for the examples above, please imagine they are taking 3 or more lines, since we can't implement the two-line custom rule you mentioned once before.

[sjrd]

For definition site:

1. First, if necessary, break after the ( of a 2nd, 3rd etc. parameter list (not the first one at this point).
2. If that is not enough (or there is only one param list), fit as many params as possible on the line, and break after a , that separates two parameters.
3. If one parameter does not fit on a line, well first you probably had it coming ^^. If it has a default value, break after = and indent 2 spaces. If that's not enough, do whatever. In any case, a multi-line parameter must be alone on its line(s), and if it's the first after ( it must start a new line as well (this is the same rule as for call site).

---

second, coming back to one-argument case:

// ok or not?
val foo = foo(bar &&
   bar)
val foo = foo(foo(bar &&
   bar))
val foo = foo(foo("""
   bar"""))
val foo = foo(foo(bar,
   bar))

@tgodzik made a really good observation about the test, and while i explained it away, i don't think that that was the intention, since the code forces a single line block only until a comma, which means multiple arguments. so, just want to be very sure about this.

Normally, none of those would be OK. But there is another rule, which I'm sure I explained somewhere, and I think you implemented it, because in the Scala.js PR and its diff I see this for example emitted by scalafmt:

                Some(genApplyStatic(implMethodSym,
                    js.This()(currentClassType) :: jsParams.map(_.ref)))

which is good.

The rule is that, if there is one spot, after a , or (, for which breaking there allows the entire logical line to fit on two lines (but not more), then do that, and be happy. This can be done at any nesting level of the , or (. If several such spots exist that would each be enough on its own, ideally we pick the outer-most, right-most one, but that's just because we need a tie-break somewhere.

That rule is what keeps most binpacking code fairly tight-fitted. One often needs two physical lines for a logical one. But it's comparatively rare that one needs 3 lines or more, so the more explosive (in terms of line count but also in terms of formatter search space) situations do not come up so often.

I think it is that rule that was applied in the formatting of

  when(audienceService.publish(any[Tenant], any[Int])).thenReturn(Success[NonEmptyList[String],
    ApiAudience](apiAudience.copy(status = AudienceStatus.Pending.name)))

(although it was not meant to apply within type parameters 🤷‍♂️ )

With that rule, the following of your examples is OK:

val foo = foo(foo(bar,
   bar))

but the others are not OK because it's not after a , or (. So they must be treated as multi-line params, and therefore start on a separate line. They would have to be formatted as

val foo = foo(
    bar && bar)
val foo = foo(foo( // this is a break allowed by the above rule
    bar && bar))
val foo = foo(
    foo(
        """
        bar"""))

That said, it's not really an issue if scalafmt doesn't follow that by default. In my experience, it has been possible to nudge it into not splitting in bad places like that: if we write by hand correctly, then scalafmt tends to preserve it.

It's OK if scalafmt lets me write the correct thing, even if I have to nudge it into doing it. What is annoying is when it forces a bad formatting and prevents from writing the right thing (like the case with the """ strings that this PR is fixing).

[kitbellew]

Normally, none of those would be OK. But there is another rule, which I'm sure I explained somewhere, and I think you implemented it, because in the Scala.js PR and its diff I see this for example emitted by scalafmt:

                Some(genApplyStatic(implMethodSym,
                    js.This()(currentClassType) :: jsParams.map(_.ref)))

which is good.

i didn't implement the rule as you mention, with two lines and a comma. i simply didn't force anything with one argument, even if multiline. should i change it now, or are you ok with forced break only if there are multiple arguments? as you said, you don't expect three lines in most cases.

[sjrd]

i didn't implement the rule as you mention, with two lines and a comma. i simply didn't force anything with one argument, even if multiline.

Ah, that's interesting! Perhaps it's good enough as is.

[kitbellew]

great, then we'll keep it, and force only with multiple arga. I'll do another look over the code before merging, then.