[clang-format] Add a option for the position of Java static import

[bc-lee]

Summary

Some Java style guides and IDEs group Java static imports after
non-static imports. This patch allows clang-format to control
the location of static imports.

[bc-lee]

Add missing initializer.

[MyDeveloperDay]

Thanks for the patch, You need to generate a fill context diff (see Contributing to LLVM)

ensure the diff is clang-formatted itself (can't quite tell if it is or not)

clang/docs/ClangFormatStyleOptions.rst

2027

The ClangFormatStyleOptions.rst is generated using doc/tools/dump_format_style.py which reads Format.h and generates this,

If this code block in not in the Format.h it will get removed the next time the script is run, please don't change ClangFormatStyleOption.rst by hand use the script, so add the code block to the Format.h file (see others options for now to do this)

[bc-lee]

Modify the comment of Format.h to sync ClangFormatStyleOptions.rst

[JakeMerdichAMD]

clang/include/clang/Format/Format.h

1705

3 things here:

1. Did you mix up the true and false cases?
2. (nit) You should also note that if this option is false, all static imports are before all non-static imports (as opposed to mixed together alphabetically). I was confused on first glance.
3. Please add this config option to FormatTests.cpp:ParsesConfigurationBools.

[bc-lee]

clang/docs/ClangFormatStyleOptions.rst

2027

Done.

clang/include/clang/Format/Format.h

1705

I understand. The description is somewhat misleading.

[bc-lee]

Some comments have been corrected and a unittest has been added in FormatTest.ParsesConfigurationBools

[JakeMerdichAMD]

clang/include/clang/Format/Format.h

1705

New phrasing makes it clear, but true and false are still inverted.

[bc-lee]

Fix the example to match the description.

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

1973

Can you add a test that shows if the sorting is still in the groups, i.e. I can't tell if

import com.test.a;
import static org.a;

import com.test.b;
import static org.b;

becomes

import static org.a;
import static org.b;
import com.test.a;

import com.test.b;

or

import static org.a;
import static org.b;

import com.test.a;

import com.test.b;

or

import static org.a;
import com.test.a;

import static org.b;
import com.test.b;

clang/include/clang/Format/Format.h

1706

Can we consider changing the name or the option to make it clearer what its for?

SortJavaStaticImport

and make it an enumeration rather than true/false

Before,After,Never

There need to be a "don't touch it option (.e.g. Never)" that does what it does today (and that should be the default, otherwise we end up causing clang-format to change ALL java code" which could be 100's of millions of lines+

clang/lib/Format/Format.cpp

906

We can't have a default that does will cause a change

clang/unittests/Format/SortImportsTestJava.cpp

253

please test all options

You are also missing a test for checking the PARSING of the options

[JakeMerdichAMD]

clang/include/clang/Format/Format.h

1706

I'm confused, there is not currently a never option... Right now the behavior is always 'before', there is no 'after' or 'never'.

Making it an enum would probably be more ergonomic though, even there is no never option.

clang/lib/Format/Format.cpp

906

Not a default change, see previous comment for discussion.

clang/unittests/Format/SortImportsTestJava.cpp

253

Parsing test was already noted and done (unless this changes to an enum of course). But testing the 'false' case here makes sense.

[bc-lee]

Add more tests and rename options.

[bc-lee]

clang/docs/ClangFormatStyleOptions.rst

1973

Done.

clang/include/clang/Format/Format.h

1706

If SortIncludes is true, it doesn't make sense to not touch Java static include.
Making it an enum is also good for me.

clang/unittests/Format/SortImportsTestJava.cpp

253

Done

[MyDeveloperDay]

LGTM, I'm happy if @JakeMerdichAMD is

clang/include/clang/Format/Format.h

1706

true.. it will sort it I assume one way or the other

[JakeMerdichAMD]

Sorry on the delay, LGTM too.

It looks like you're a first time contributor and probably don't have write access to the repo, do you want one of us to push this on your behalf? If you intend to have more commits in the future, write access to github isn't hard to get (just request it once you have a few changes in).

[bc-lee]

It's okay for some reviewers to make this change on my behalf. Thanks for reviewing this!

[bc-lee]

Can someone commit my changes on behalf of it?

[bc-lee]

This comment was removed by bc-lee.

clang/docs/ClangFormatStyleOptions.rst

**JavaImportGroups** (``std::vector<std::string>``)
   Each group is separated by a newline. Static imports will also follow the
   same grouping convention above all non-static imports. One group's prefix
   can be a subset of another - the longest prefix is always matched. Within
   a group, the imports are ordered lexicographically.
 
   In the .clang-format configuration file, this can be configured like
   in the following yaml example. This will result in imports being
   formatted as in the Java example below.
 
   .. code-block:: yaml
 
     JavaImportGroups: ['com.example', 'com', 'org']
 
 
   .. code-block:: java
 
      import static com.example.function1;
* ``JSQS_Double`` (in configuration: ``Double``)
     .. code-block:: js
 
        string1 = "foo";
        string2 = "bar";
 
 
 
 **JavaScriptWrapImports** (``bool``)
   Whether to wrap JavaScript import/export statements.
 
   .. code-block:: js
 
      true:
      import {
          VeryLongImportsAreAnnoying,
          VeryLongImportsAreAnnoying,
          VeryLongImportsAreAnnoying,
      } from 'some/module.js'
 
      false:
      import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
 
+**JavaStaticImportAfterImport** (``bool``)
+  If true, clang-format will put Java static imports after all non-static
+  imports.
+
+  .. code-block:: java
+
+    true:
+    import static org.example.function1;
+
+    import org.example.ClassA;
+
+    false:
+    import org.example.ClassA;
+
+    import static org.example.function1;
+
 **KeepEmptyLinesAtTheStartOfBlocks** (``bool``)
   If true, the empty line at the start of blocks is kept.
 
   .. code-block:: c++
 
      true:                                  false:
      if (foo) {                     vs.     if (foo) {
                                               bar();

clang/include/clang/Format/Format.h

   ///    } from 'some/module.js'
   ///
   ///    false:
   ///    import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
   /// \endcode
   bool JavaScriptWrapImports;
   // clang-format on
 
+  /// If true, clang-format will put Java static imports after all non-static
+  /// imports.
+  /// \code{.java}
+  ///   true:
+  ///   import static org.example.function1;
+  ///
+  ///   import org.example.ClassA;
+  ///
+  ///   false:
+  ///   import org.example.ClassA;
+  ///
+  ///   import static org.example.function1;
+  /// \endcode
+  bool JavaStaticImportAfterImport;
+
   /// If true, the empty line at the start of blocks is kept.
   /// \code
   ///    true:                                  false:
   ///    if (foo) {                     vs.     if (foo) {
   ///                                             bar();
   ///      bar();                               }
   ///    }
   /// \endcode
return AccessModifierOffset == R.AccessModifierOffset &&
            IndentGotoLabels == R.IndentGotoLabels &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentExternBlock == R.IndentExternBlock &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
            JavaImportGroups == R.JavaImportGroups &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&
+           JavaStaticImportAfterImport == R.JavaStaticImportAfterImport &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
            NamespaceIndentation == R.NamespaceIndentation &&
            NamespaceMacros == R.NamespaceMacros &&
            ObjCBinPackProtocolList == R.ObjCBinPackProtocolList &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
     IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
     IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
     IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
     IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
+    IO.mapOptional("JavaStaticImportAfterImport",
+                   Style.JavaStaticImportAfterImport);
     IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
     IO.mapOptional("MacroBlockEnd", Style.MacroBlockEnd);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
     IO.mapOptional("NamespaceMacros", Style.NamespaceMacros);
     IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
+  LLVMStyle.JavaStaticImportAfterImport = false;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
   LLVMStyle.ObjCBreakBeforeNestedBlockParam = true;
   LLVMStyle.ObjCSpaceAfterProperty = false;
if (!affectsRange(Ranges, ImportsBeginOffset, ImportsEndOffset))
     return;
   SmallVector<unsigned, 16> Indices;
   SmallVector<unsigned, 16> JavaImportGroups;
   for (unsigned i = 0, e = Imports.size(); i != e; ++i) {
     Indices.push_back(i);
     JavaImportGroups.push_back(
         findJavaImportGroup(Style, Imports[i].Identifier));
   }
+  bool StaticImportAfterNormalImport = Style.JavaStaticImportAfterImport;
   llvm::sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
     // Negating IsStatic to push static imports above non-static imports.
-    return std::make_tuple(!Imports[LHSI].IsStatic, JavaImportGroups[LHSI],
-                           Imports[LHSI].Identifier) <
-           std::make_tuple(!Imports[RHSI].IsStatic, JavaImportGroups[RHSI],
-                           Imports[RHSI].Identifier);
+    return std::make_tuple(!Imports[LHSI].IsStatic ^
+                               StaticImportAfterNormalImport,
+                           JavaImportGroups[LHSI], Imports[LHSI].Identifier) <
+           std::make_tuple(!Imports[RHSI].IsStatic ^
+                               StaticImportAfterNormalImport,
+                           JavaImportGroups[RHSI], Imports[RHSI].Identifier);
   });
 
   // Deduplicate imports.
   Indices.erase(std::unique(Indices.begin(), Indices.end(),
                             [&](unsigned LHSI, unsigned RHSI) {
                               return Imports[LHSI].Text == Imports[RHSI].Text;
                             }),
                 Indices.end());

clang/unittests/Format/SortImportsTestJava.cpp

EXPECT_EQ("// clang-format off\n"
             sort("// clang-format off\n"
                  "import org.b;\n"
                  "import org.a;\n"
                  "// clang-format on\n"
                  "import org.d;\n"
                  "import org.c;\n"));
 }
 
+TEST_F(SortImportsTestJava, FormatJavaStaticImportAfterImport) {
+  FmtStyle.JavaStaticImportAfterImport = true;
+
+  EXPECT_EQ("import com.test.b;\n"
+            "import com.test.c;\n"
+            "\n"
+            "import org.b;\n"
+            "\n"
+            "import com.b;\n"
+            "\n"
+            "import static com.test.a;\n"
+            "\n"
+            "import static org.a;\n"
+            "\n"
+            "import static com.a;\n",
+            sort("import static com.test.a;\n"
+                 "import static org.a;\n"
+                 "import static com.a;\n"
+                 "import com.test.b;\n"
+                 "import org.b;\n"
+                 "import com.b;\n"
+                 "import com.test.c;\n"));
+}
+
 TEST_F(SortImportsTestJava, DeduplicateImports) {
   EXPECT_EQ("import org.a;\n", sort("import org.a;\n"
                                     "import org.a;\n"));
 }
 
 TEST_F(SortImportsTestJava, NoNewlineAtEnd) {
   EXPECT_EQ("import org.a;\n"
             "import org.b;",