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

[Andrey Mishchenko via Phabricator via cfe-commits]

andmis added a comment.

Thanks all for the reviews. I've updated the patch and added responses to your comments.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2820
 
+**JavaScriptWrapImports** (``Boolean``) :versionbadge:`clang-format 3.9`
+  Whether to wrap JavaScript import/export statements. If ``false``, then
----------------
MyDeveloperDay wrote:
> This file is generated from Format.h you need to make the changes there and regenerate using clang/docs/tools/dump_format_style.py
Alright. I've added the source to `Format.h` and run `dump_format_style.py`.


================
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.
----------------
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`.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2836
 
-     true:
+     // Original
+     import {VeryLongImport, AnotherLongImport, LongImportsAreAnnoying} from 'some/module.js'
----------------
curdeius wrote:
> What does "Original" mean here? It's unclear whether the option is true or false nor what the ColumnWidth is.
> 
> I think you want to have 3 examples:
> 1) JavaScriptWrapImports: false, independent of ColumnWidth
> 2) JavaScriptWrapImports: true, ColumnWidth: 0
> 3) JavaScriptWrapImports: true, ColumnWidth: non-zero
> In each case you want to have short and long import lines.
"Original" means "Before running clang-format", I've changed the comment to the latter.

The reason this is here is that with `ColumnLimit: 0`, the output format depends on the input, and I don't see how to make that clear without having a before and after view.


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1948
+  Style.JavaScriptWrapImports = false;
   verifyFormat("import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,"
                " VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying"
----------------
MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > you are no longer testing the LLVM Case, please don't remove that, but feel free to ensure they are doing the same for google!
> Its kind of our golden rule, don't change existing tests, subtle changes can have huge implications on large code bases
Thanks, this was an oversight on my part.


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1975
+               Style);
+  // Using the input_code/expected version of verifyFormat because we can't
+  // indiscriminately test::messUp on these tests.
----------------
mprobst wrote:
> I'm not sure this test case really repros the doc changes you made (keeps any line wraps with ColW: 0).
> 
> I think if you wanted to demonstrate that, you'd need to add a test case where clang-format would normally make a change, and show that it does not with ColW: 0.
> 
> However I think that's not feasible: by design, clang-format cannot not make a change, it always reformats all code. You might need to rethink the intent here.
I think the tests are doing what's claimed. I've rearranged and rewritten them, PTAL.


================
Comment at: clang/unittests/Format/FormatTestJS.cpp:1990
   Style.ColumnLimit = 40;
-  // Using this version of verifyFormat because test::messUp hides the issue.
+  Style.JavaScriptWrapImports = true;
   verifyFormat("import {\n"
----------------
mprobst wrote:
> curdeius wrote:
> > It's already true, cf. line 1977. Remove this line.
> FWIW, I think the tests might be more readable if each configuration ({true, false} x {col: 0, col: 40}) explicitly set all the options, even if it's a bit redundant.
I've reformatted them in two big blocks: one where `ColumnLimit > 0` and one where `ColumnLimit: 0`. So there are no more redundant option set statements.

This is the most-readable way to format the tests IMO because when `ColumnLimit > 0`, we can use the simple `verifyFormat` (where you just pass the expected result and it messes it up, formats, and checks) because clang-format will do a full reflow. But for the `ColumnLimit: 0` tests, we need to use the other `verifyFormat` (where we have input and expected output explicitly), and it's convenient to put a comment explaining what's happening at the top of the `ColumnLimit: 0` block.


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

https://reviews.llvm.org/D116638