Propagate parental rule breakage in chunk groups at range-formatting boundaries
commite768e52b238ba56454b3afe7d25e52424794a1bb
authorJake Bailey (Hacklang) <jakebailey@fb.com>
Wed, 25 Jul 2018 01:30:20 +0000 (24 18:30 -0700)
committerHhvm Bot <hhvm-bot@users.noreply.github.com>
Wed, 25 Jul 2018 01:42:39 +0000 (24 18:42 -0700)
tree6aad199c320e2ea6cc17c6b62e7bf27de7b82606
parentfe5da68825a9720daeb991e14a2c897b203f6327
Propagate parental rule breakage in chunk groups at range-formatting boundaries

Summary:
In D5853970, we changed the behavior of range-formatting at boundaries. If a split outside of the range we were formatting was broken in the original source, we would break all splits associated with the same rule within the formatting range. Additionally (and more critically), we would not propagate breakage from such a rule's dependencies to break that rule.

Here's an example. Suppose we are formatting line 3 of this file, with a maximum line length of 10 characters:

```
<?hh
f($a, g(
  h($b),
  i($c),
));
```

If we run `hackfmt --line-width 10 file.php`, we get this result:

```
<?hh
f(
  $a,
  g(
    h($b),
    i($c),
  ),
);
```

This is because the rule governing the splits in `f`'s argument list depends upon the rule governing the splits in `g`'s argument list. If `g`'s rule breaks, causing all its splits to be rendered as newlines, `f`'s rule will break too.

However, if we run `hackfmt --line-width 10 --line-range 3 3 file.php`, we get a different result for the line containing the invocation of `h`:

```
  h($b),
```

It's now indented only two spaces instead of four. This is because indentation is calculated based on which rules we decided to break--`h`'s nesting is associated both with the rule governing the splits in `g`'s argument list and the rule governing the splits in `f`'s argument list. When we render this line, we note that we have decided to break `g`'s rule, and indent `h` two spaces. We also note that we have //not// decided to break the rule governing the splits in `f`'s argument list, and don't indent it further.

This is the behavior that D5853970 introduced. Under normal circumstances, we would always break `f`'s rule because we broke `g`'s rule. But since `f`'s rule governs splits outside of the formatting range, we are not free to do so in this case, and do not propagate `g`'s breakage to `f` (in fact, this implementation does not propagate breakage at all).

Unfortunately, this led to undesirable outcomes in other (rarer) situations. Consider this file (reported in T31728499):

```
<?hh

f(
  self::getBuilder()
    ->setSomeProperty(self::$thing)
    ->setSomeOtherProperty(self::$ids['stuff'])
    ->genSave(),
  self::getBuilder()
    ->setAnotherProperty(self::$item)
    ->setYetAnotherProperty(self::$value)
    ->setOneMoreProperty(vec[self::$junk])
    ->genSave(),
);
```

Let's say we are formatting the range of the entire invocation of `f` (lines 3-13). If we run `hackfmt --line-range 3 13 file.php`, we get this result:

```
f(self::getBuilder()
  ->setSomeProperty(self::$thing)
  ->setSomeOtherProperty(self::$ids['stuff'])
  ->genSave(), self::getBuilder()
  ->setAnotherProperty(self::$item)
  ->setYetAnotherProperty(self::$value)
  ->setOneMoreProperty(vec[self::$junk])
  ->genSave());
```

The rule governing the splits in `f`'s argument list depends on the two rules governing the splitting of the builder chains, but we did not propagate the breakage of the builder chain rules to `f`'s rule. Since this solution fits within 80 characters, hackfmt does not break `f`'s rule and accepts this solution.

I think the critical distinction between this situation, where we **do** want to propagate breakage, and the previous situation, where we don't, is that the splits governed by `f`'s rule occur //entirely within the formatting range//. After this change, we do propagate breakage, but only to the rules entirely within the formatting range, producing the right solution in both cases.

Reviewed By: pittsw

Differential Revision: D8957816

fbshipit-source-id: 8d3692437d87f429ea969a813a1b4ddc9846aad7
hphp/hack/src/hackfmt/chunk_group.ml
hphp/hack/src/hackfmt/line_splitter.ml
hphp/hack/test/hackfmt/tests/range_function_call.flags [new file with mode: 0644]
hphp/hack/test/hackfmt/tests/range_function_call.php [new file with mode: 0644]
hphp/hack/test/hackfmt/tests/range_function_call.php.exp [new file with mode: 0644]