[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

1707

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

1707

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

1707

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

1972

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

1708

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

914

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

1708

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

914

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

1972

Done.

clang/include/clang/Format/Format.h

1708

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

1708

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

* ``TCS_Wrapped`` (in configuration: ``Wrapped``)
     that a container should be formatted one-per-line (i.e. not bin-packed).
     So inserting a trailing comma counteracts bin-packing.
 
 
 
 **JavaImportGroups** (``std::vector<std::string>``)
   A vector of prefixes ordered by the desired groups for Java imports.
 
-  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.
+  One group's prefix can be a subset of another - the longest prefix is
+  always matched. Within a group, the imports are ordered lexicographically.
+  Static imports are grouped separately and follow the same group rules.
+  By default, static imports are placed before non-static imports,
+  but this behavior is changed by another option,
+  ``SortJavaStaticImport``.
 
   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']
**JavaScriptQuotes** (``JavaScriptQuoteStyle``)
 
   * ``JSQS_Double`` (in configuration: ``Double``)
     Always use double quotes.
 
     .. code-block:: js
 
        string1 = "foo";
        string2 = "bar";
 
 
 
 **JavaScriptWrapImports** (``bool``)
   Whether to wrap JavaScript import/export statements.
 
   .. code-block:: js
 
      true:
**SortIncludes** (``bool``)
   If ``true``, clang-format will sort ``#includes``.
 
   .. code-block:: c++
 
      false:                                 true:
      #include "b.h"                 vs.     #include "a.h"
      #include "a.h"                         #include "b.h"
 
+**SortJavaStaticImport** (``SortJavaStaticImportOptions``)
+  When sorting Java imports, by default static imports are placed before
+  non-static imports. If ``JavaStaticImportAfterImport`` is ``After``,
+  static imports are placed after non-static imports.
+
+  Possible values:
+
+  * ``SJSIO_Before`` (in configuration: ``Before``)
+    Static imports are placed before non-static imports.
+
+    .. code-block:: java
+
+      import static org.example.function1;
+
+      import org.example.ClassA;
+
+  * ``SJSIO_After`` (in configuration: ``After``)
+    Static imports are placed after non-static imports.
+
+    .. code-block:: java
+
+      import org.example.ClassA;
+
+      import static org.example.function1;
+
+
+
 **SortUsingDeclarations** (``bool``)
   If ``true``, clang-format will sort using declarations.
 
   The order of using declarations is defined as follows:
   Split the strings by "::" and discard any initial empty strings. The last
   element of each list is a non-namespace name; all others are namespace
   names. Sort the lists of names lexicographically, where the sort order of
   individual names is that all non-namespace names come before all namespace

clang/include/clang/Format/Format.h

   ///    false:
   ///    LoooooooooooooooooooooooooooooooooooooooongReturnType
   ///    LoooooooooooooooooooooooooooooooongFunctionDeclaration();
   /// \endcode
   bool IndentWrappedFunctionNames;
 
   /// A vector of prefixes ordered by the desired groups for Java imports.
   ///
-  /// 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.
+  /// One group's prefix can be a subset of another - the longest prefix is
+  /// always matched. Within a group, the imports are ordered lexicographically.
+  /// Static imports are grouped separately and follow the same group rules.
+  /// By default, static imports are placed before non-static imports,
+  /// but this behavior is changed by another option,
+  /// ``SortJavaStaticImport``.
   ///
   /// 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{.yaml}
   ///   JavaImportGroups: ['com.example', 'com', 'org']
   /// \endcode
   ///

   ///      bar();                               }
   ///    }
   /// \endcode
   bool KeepEmptyLinesAtTheStartOfBlocks;
 
   /// Supported languages.
   ///
   /// When stored in a configuration file, specifies the language, that the
   /// configuration targets. When passed to the ``reformat()`` function, enables
   /// syntax features specific to the language.
   enum LanguageKind {
     /// Do not use.
     LK_None,
     /// Should be used for C, C++.
     LK_Cpp,
     /// Should be used for C#.
     LK_CSharp,
     /// Should be used for Java.

   /// If ``true``, clang-format will sort ``#includes``.
   /// \code
   ///    false:                                 true:
   ///    #include "b.h"                 vs.     #include "a.h"
   ///    #include "a.h"                         #include "b.h"
   /// \endcode
   bool SortIncludes;
 
+  /// Position for Java Static imports.
+  enum SortJavaStaticImportOptions {
+    /// Static imports are placed before non-static imports.
+    /// \code{.java}
+    ///   import static org.example.function1;
+    ///
+    ///   import org.example.ClassA;
+    /// \endcode
+    SJSIO_Before,
+    /// Static imports are placed after non-static imports.
+    /// \code{.java}
+    ///   import org.example.ClassA;
+    ///
+    ///   import static org.example.function1;
+    /// \endcode
+    SJSIO_After,
+  };
+
+  /// When sorting Java imports, by default static imports are placed before
+  /// non-static imports. If ``JavaStaticImportAfterImport`` is ``After``,
+  /// static imports are placed after non-static imports.
+  SortJavaStaticImportOptions SortJavaStaticImport;
+
   /// If ``true``, clang-format will sort using declarations.
   ///
   /// The order of using declarations is defined as follows:
   /// Split the strings by "::" and discard any initial empty strings. The last
   /// element of each list is a non-namespace name; all others are namespace
   /// names. Sort the lists of names lexicographically, where the sort order of
   /// individual names is that all non-namespace names come before all namespace
   /// names, and within those groups, names are in case-insensitive
return AccessModifierOffset == R.AccessModifierOffset &&
            PenaltyBreakFirstLessLess == R.PenaltyBreakFirstLessLess &&
            PenaltyBreakString == R.PenaltyBreakString &&
            PenaltyExcessCharacter == R.PenaltyExcessCharacter &&
            PenaltyReturnTypeOnItsOwnLine == R.PenaltyReturnTypeOnItsOwnLine &&
            PenaltyBreakTemplateDeclaration ==
                R.PenaltyBreakTemplateDeclaration &&
            PointerAlignment == R.PointerAlignment &&
            RawStringFormats == R.RawStringFormats &&
+           SortJavaStaticImport == R.SortJavaStaticImport &&
            SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
            SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
            SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
            SpaceBeforeCtorInitializerColon ==
                R.SpaceBeforeCtorInitializerColon &&
            SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&

clang/lib/Format/Format.cpp

static void enumeration(IO &IO,
                           FormatStyle::BitFieldColonSpacingStyle &Value) {
     IO.enumCase(Value, "Both", FormatStyle::BFCS_Both);
     IO.enumCase(Value, "None", FormatStyle::BFCS_None);
     IO.enumCase(Value, "Before", FormatStyle::BFCS_Before);
     IO.enumCase(Value, "After", FormatStyle::BFCS_After);
   }
 };
 
+template <>
+struct ScalarEnumerationTraits<FormatStyle::SortJavaStaticImportOptions> {
+  static void enumeration(IO &IO,
+                          FormatStyle::SortJavaStaticImportOptions &Value) {
+    IO.enumCase(Value, "Before", FormatStyle::SJSIO_Before);
+    IO.enumCase(Value, "After", FormatStyle::SJSIO_After);
+  }
+};
+
 template <> struct MappingTraits<FormatStyle> {
   static void mapping(IO &IO, FormatStyle &Style) {
     // When reading, read the language first, we need it for getPredefinedStyle.
     IO.mapOptional("Language", Style.Language);
 
     if (IO.outputting()) {
       StringRef StylesArray[] = {"LLVM",   "Google", "Chromium", "Mozilla",
                                  "WebKit", "GNU",    "Microsoft"};
IO.mapOptional("PenaltyBreakTemplateDeclaration",
                    Style.PenaltyBreakTemplateDeclaration);
     IO.mapOptional("PenaltyExcessCharacter", Style.PenaltyExcessCharacter);
     IO.mapOptional("PenaltyReturnTypeOnItsOwnLine",
                    Style.PenaltyReturnTypeOnItsOwnLine);
     IO.mapOptional("PointerAlignment", Style.PointerAlignment);
     IO.mapOptional("RawStringFormats", Style.RawStringFormats);
     IO.mapOptional("ReflowComments", Style.ReflowComments);
     IO.mapOptional("SortIncludes", Style.SortIncludes);
+    IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport);
     IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
     IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
     IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
     IO.mapOptional("SpaceAfterTemplateKeyword",
                    Style.SpaceAfterTemplateKeyword);
     IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
     IO.mapOptional("SpaceBeforeCpp11BracedList",
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.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;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.PenaltyBreakString = 1000;
   LLVMStyle.PenaltyExcessCharacter = 1000000;
   LLVMStyle.PenaltyReturnTypeOnItsOwnLine = 60;
   LLVMStyle.PenaltyBreakBeforeFirstCallParameter = 19;
   LLVMStyle.PenaltyBreakTemplateDeclaration = prec::Relational;
 
   LLVMStyle.DisableFormat = false;
   LLVMStyle.SortIncludes = true;
+  LLVMStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before;
   LLVMStyle.SortUsingDeclarations = true;
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("STRINGIZE");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("PP_STRINGIZE");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("BOOST_PP_STRINGIZE");
 
   // Defaults that differ when not C++.
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.SortJavaStaticImport == FormatStyle::SJSIO_After;
   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/FormatTest.cpp

CHECK_PARSE("BitFieldColonSpacing: Both", BitFieldColonSpacing,
               FormatStyle::BFCS_Both);
   CHECK_PARSE("BitFieldColonSpacing: None", BitFieldColonSpacing,
               FormatStyle::BFCS_None);
   CHECK_PARSE("BitFieldColonSpacing: Before", BitFieldColonSpacing,
               FormatStyle::BFCS_Before);
   CHECK_PARSE("BitFieldColonSpacing: After", BitFieldColonSpacing,
               FormatStyle::BFCS_After);
 
+  Style.SortJavaStaticImport = FormatStyle::SJSIO_Before;
+  CHECK_PARSE("SortJavaStaticImport: After", SortJavaStaticImport,
+              FormatStyle::SJSIO_After);
+  CHECK_PARSE("SortJavaStaticImport: Before", SortJavaStaticImport,
+              FormatStyle::SJSIO_Before);
+
   // FIXME: This is required because parsing a configuration simply overwrites
   // the first N elements of the list instead of resetting it.
   Style.ForEachMacros.clear();
   std::vector<std::string> BoostForeach;
   BoostForeach.push_back("BOOST_FOREACH");
   CHECK_PARSE("ForEachMacros: [BOOST_FOREACH]", ForEachMacros, BoostForeach);
   std::vector<std::string> BoostAndQForeach;
   BoostAndQForeach.push_back("BOOST_FOREACH");

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, SortJavaStaticImport) {
+  FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_Before;
+  EXPECT_EQ("import static com.test.a;\n"
+            "\n"
+            "import static org.a;\n"
+            "\n"
+            "import static com.a;\n"
+            "\n"
+            "import com.test.b;\n"
+            "\n"
+            "import org.b;\n"
+            "\n"
+            "import com.b;\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"));
+
+  FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After;
+  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, SortJavaStaticImportAsGroup) {
+  FmtStyle.SortJavaStaticImport = FormatStyle::SJSIO_After;
+
+  EXPECT_EQ("import com.test.a;\n"
+            "import com.test.b;\n"
+            "\n"
+            "import static org.a;\n"
+            "import static org.b;\n",
+            sort("import com.test.a;\n"
+                 "import static org.a;\n"
+                 "import com.test.b;\n"
+                 "import static org.b;\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;",