[PATCH] D116638: [clang-format] Fix ignoring JavaScriptWrapImport when ColumnWidth: 0

[Andrey Mishchenko via Phabricator via cfe-commits]

andmis added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2827
 
-**JavaScriptWrapImports** (``Boolean``) :versionbadge:`clang-format 3.9`
-  Whether to wrap JavaScript import/export statements.
+  * If ``ColumnWidth`` is 0 (no limit on the number of columns), then import
+    statements will keep the number of lines they start with.
----------------
mprobst wrote:
> andmis wrote:
> > HazardyKnusperkeks wrote:
> > > mprobst wrote:
> > > > this seems odd to me: my understanding is that clang-format always reflows the entire document, there's no logic to ever "keep" whitespace.
> > > > 
> > > > Are you sure you are seeing this behaviour? The logic change below sounds more as if the ColumnWidth: 0, import lines might not break (mustBreak returns false), but might still break?
> > > From what I can tell with `ColumnLimit` (and not `ColumnWidth`) 0, then `clang-format` does many things differently. If by design or by accident I'm not sure. I can see that often, since I format my code with a limit, but my tests without.
> > I just double-checked and yes, as-implemented, with `ColumnLimit: 0` and `JavaScriptWrapImports: true`, import statements retain the number of lines they started with.
> > 
> > It is explicit in the code that with `ColumnLimit: 0`, we decide whether to break on a token based on whether there was a preexisting line break. (Check out `NoColumnLimitLineFormatter`.) I think you're right that usually clang-format reflows everything and I was also confused about this point.
> > 
> > There are other options that do similar things, for example `EmptyLineAfterAccessModifier`.
> Can you check with code that would normally cause reformatting? E.g.
> 
> ```
> import {
>   A, B,
>   C,
> } from 'url';
> ```
> 
Sigh, you are right: your snippet gets formatted to 
```
import {
  A, B, C,
} from 'url';
```

I had tried 
```
import {
  A,
  B, C,
} from 'url';
```
which does indeed get left alone.

It seems that the implementation logic of the `NoColumnLimitLineFormatter` just doesn't play well with certain other clang-format features. In this case, the `NoColumnLimitLineFormatter` decides whether to allow line breaks based on whether a given token had a newline preceding it in the original input, but tokens also get reordered by `SortIncludes` so being preceded by a newline doesn't mean what `NoColumnLimitLineFormatter` thinks it does. For example, in my implementation, these two get formatted differently:
```
import {
  A,
  B, C,
} from 'url';

import {
  A,
  C, B,
} from 'url';
```

This example also shows that it is totally unclear how to make sense of both (1) keeping import lines "in the original order" and at the same time (2) sorting the actual imported symbols. So I guess we might need to rethink the intent.

It seems there are two clean options if `ColumnLimit: 0`:
- Make `JavaScriptWrapImports: true` *always* wrap imports to multiple lines. This will be noisy and ugly.
- Make `JavaScriptWrapImports` have no effect if `ColumnLimit: 0`, and *always* unwrap import lines to a single line if `ColumnLimit: 0`.
In either case, a user who wants `ColumnLimit: 0` and "leave my import line wrapping as I have it" (likely to be a common case for people with `ColumnLimit: 0`, IMO) will not be able to achieve what they want. In the much-more-common `ColumnLimit > 0` case, we use the `ColumnLimit` to decide whether to wrap, but we can't do that here.

Note: before this patch, the option `JavaScriptWrapImports` is ignored with `ColumnLimit: 0`, and imports, even short ones, are formatted as:
```
import {aaa,
        bbb,
        ccc} from "def";
```

To summarize, it seems like JavaScript formatting with `ColumnLimit: 0` is broken for JS ATM, because the flag `JavaScriptWrapImports` is basically ignored by the code in this case, and imports are always wrapped to multiple lines, even short ones, which is pretty ugly and noisy. This, by the way, suggests that very few people use `ColumnLimit: 0` for JS. The two simple resolution options I quoted above are straightforward, but completely miss what I expect would be the most-common use case with `ColumnLimit: 0`: a user who wants to manually control their line wrapping. The current patch handles what's likely to be the most-common version of this use case (all imports on one line, or every import on its own line), but is broken for some edge cases in a way that is conceptually difficult to see a solution to (due to interaction with `SortIncludes`).

So I'm a bit unsure how to proceed. This patch seems like an improvement since it fixes the very-common case `ColumnLimit: 0` and `JavaScriptWrapImports: false`. So IMO it would be ok to merge it.

What should we do in the case `ColumnLimit: 0` and `JavaScriptWrapImports: true`? For a given `import` line, we could check whether it started out multi-line, and if so, we break at every imported symbol, otherwise we leave the import line as a single line. But this cascades into further issues: is the first symbol on the same line as the opening curly? Do we try to infer this from the original layout? Or add another flag? Or just impose a policy?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116638/new/

https://reviews.llvm.org/D116638