Fix clang-formats IncludeCategory to match the documentation

[Febbe]

Summary

• Main Include Files have now always prio (0,0)
• They can't be matched by negative matchers anymore.
• SortPriority now truly defaults to Priority
• If it is unclear, which include is the main include, use both instead of using the first.

fixes #58284
fixes #60589

[Febbe]

Still some issues with SortPriorities.
The following settings (note the SortPriority of '^<.*' == 1) which will produce an extra group for the attached includes:

IncludeCategories:
  - Regex:           '^<ext/.*\.h>'
    Priority:        2
    SortPriority:    2
  - Regex:           '^<.*\.h>'
    Priority:        1
    SortPriority:    1
  - Regex:           '^<.*'
    Priority:        2
    SortPriority:    1
  - Regex:           '.*'
    Priority:        -1
    SortPriority:    1

#include "toolset_febbe.h" // <- Not expected to be in group 1

#include "zhello_world.h" // <- Not expected to be in group 2

#include <cstddef>
#include <cstdint>

#include <ext/bar.h>

#include <iostream> // <- no group expected

But at least it does what the documentation states and the most users would expect for the default case (Priority == SortPriority).

It seems that the include list is currently first sorted (by SortPriority), and then split into groups, which does not make much sense, since it also rearranges the groups and splits them up. But the sorting before the regroup is at least documented.

[Febbe]

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

17

<limits> is used, but not included. Currently, it works, since another header already included it.

[Febbe]

Fixed the Unit Tests

• rewritten one test, which made the assumption, that there can be only one main header.
  • it now asserts, that all matching headers are considered as main header.
• replaced initialisations of IncludeCategories.SortPriority to zero with the value from IncludeCategories.Priority
  • the previous approach to set the SortPriority when it's 0 to Priority, instead of initializing it directly as intended had several drawbacks:
    • values of 0 were not possible and resulted in weird behav.
    • when a main include was matched by another matcher, it got annother sortpriority than 0.

[Febbe]

Added priority to the sorting algorithm.
This prevents unwanted splits and weird resorts of groups with the same priority, but with different and overlapping (other groups) SortPriority.
The new system can now be understood as Primary.SecondaryPriority instead of:

sort by SecondPriority + bucket By PrimaryPriority

[Febbe]

I think I am done, and you can review it now.

With the latest changes, SortPriority is no longer required to match Priority, it can always be 0, since Priority is now used in the sorting algorithm.
This is what the documentation tends to depict:

"and #includes are sorted first according to increasing category number and then alphabetically within each category."
" There is a third and optional field SortPriority which can used while IncludeBlocks = IBS_Regroup to define the priority in which #includes should be ordered."

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

1381

I don't follow why this is necessary. And I don't feel comfortable with it.

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

218–219

Drop the braces (now).

clang/lib/Tooling/Inclusions/IncludeStyle.cpp

28–29

A big no! You will break existing configurations.

[Febbe]

I've added some elaborations and justifications for the criticized changes.

clang/lib/Format/Format.cpp

1381

This was required, but is not required necessarily, my last update of the patch made this change most likely obsolete.

But the actual reason is, that SortPriority is described in the documentation as "optional value which is defaulted to Priority, if absent", but it is neither a (std::)optional value nor it was defaulted to Priority if absent.
It was indirectly updated from 0 to Priority in the algorithm.

Since I moved the buggy, re-initialization of SortPriority to the first initialization:
clang/lib/Tooling/Inclusions/IncludeStyle.cpp:L20 this change must also be covered by the tests: All zero initializations must now be default to Priority to not change the test behavior, which is what you want.

Either by creating a Constructor without SortPriority, which defaults to Priority,
creating a factory function, or by doing this for all tests manually.

Imagine the tests would also test the interpretation of the JSON input. Then the tests would not require an adjustment to be semantically equal.

But since I added Priority to the sorting after this change, this is kind of irrelevant.

clang/lib/Tooling/Inclusions/IncludeStyle.cpp

28–29

Can you elaborate?
Do you mean the change from mapOptional to mapRequired?
From what I thought, this change only affects clarity (improvement), not the semantic:

• the parent JSON list/array entry is already optional.
• therefore any of them is required, but none is useful alone.
  • an empty regex, just matches nothing => regex required
  • an empty Priority should default (documentation) to INT_MAX, but actually did not (0).
• Also, from the documentations view, omitting one of them was never intended.

The only effect this change will have for old configurations, is, that it breaks "wrong" configurations. It's like abusing undefined behav.: You can't be sure it works after a compiler update.

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

1381

My main question is, does this change behavior? In either case, please add some tests to show the difference, or the lack thereof.

clang/lib/Tooling/Inclusions/IncludeStyle.cpp

28–29

Can you elaborate?
Do you mean the change from mapOptional to mapRequired?
From what I thought, this change only affects clarity (improvement), not the semantic:

• the parent JSON list/array entry is already optional.
• therefore any of them is required, but none is useful alone.
  • an empty regex, just matches nothing => regex required

While a configuration without a regex may be stupid, it currently is accepted.

• an empty Priority should default (documentation) to INT_MAX, but actually did not (0).

Here one has to decide if the documentation or the behavior has to be fixed. I have not a preference, @MyDeveloperDay
may have.

• Also, from the documentations view, omitting one of them was never intended.

Nevertheless it is accepted and thus there is most likely a configuration which uses it.

The only effect this change will have for old configurations, is, that it breaks "wrong" configurations. It's like abusing undefined behav.: You can't be sure it works after a compiler update.

Changing what happens in such cases is one thing (but also not desired), just completely turning clang-format off, through erroring on the configuration parsing is in my view not acceptable.

[Febbe]

• Removed some, not required changes.
• Reverted mapOptional -> mapRequired change, since it might break existing configs

[Febbe]

Added requested tests:

• since we now support priorities, equal to 0 (the main include priority) sorting should work now for those.
• multiple files can be main-includes now, the tests expects that now.
• negative priorities do not override the main include priority anymore, which is also tested

[Febbe]

clang/lib/Format/Format.cpp

1381

I removed all changes to the tests, which do not change the behavior.

One test required a change. It assumed, that only one file can be a main include, which is, in my opinion, not correct. I've made the change, that all files, which are matched as main include are ordered with priority 0 (this especially means includes introduced by IncludeIsMainRegex and IncludeIsMainSourceRegex). The previous implementation ignored those, if another main file was found earlier. It could also result in unpredictable resorting.

[HazardyKnusperkeks]

Looks good to me, apart from that one issue.

clang/lib/Tooling/Inclusions/IncludeStyle.cpp

23

Please move this into the mapping function.

clang/unittests/Format/SortIncludesTest.cpp

11

Don't need that, or?

548

While I may accept there are more than one main header, this should not happen in my opinion.

557

End comments with full stop.

[Febbe]

clang/unittests/Format/SortIncludesTest.cpp

548

Yes, that is a conflict of interests. There is no perfect way of implementing this, without the help of clang-tidy / the clang language server:

To arguably define a file as a main include, all declarations must be analyzed, whether they are predeclared in the include or not.
But we don't have the help of the clang AST.

My expectation is, that when I randomly shuffle the includes, that they are always sorted to the same result.
Currently, this is not the case, and depending on the rules it can furthermore happen, that a main include "a.h" is exchanged with the "llvm/a.h".
This should also not happen, but it does, and it is not covered by the tests.

I consider the false negative of a correct main include worse than a false positive.
Therefore, I changed it.
In addition, my approach has the advantage that all includes are sorted in the same way, regardless of the order.

But if you want, we could introduce a new Option like: enum FindMainIncludes{FMI_First, FMI_All, FMI_Off = 0};
With First to match the current behavior, All for the new behavior.
But then matched includes with a negative priority would be swapped with the other possible main_include at each run.
I don't know how to prevent this.
The best solution I can imagine, is still a comment of the programmer, that the respective include is or is not a main include.
E.g. // clang-format pragma: no_main_include

Another idea would be, to insert all main includes into a list with the matchers' priority, and then take the last negative or the first positive, but only if it is not partially sortable with the neighbors.
This might be a good heuristic, whether the include was previously detected as main include.
But I don't know if this is implementable with the current algorithm.

[HazardyKnusperkeks]

clang/unittests/Format/SortIncludesTest.cpp

548

It is very unfortunate that llvm/a.h would be detected as a main header, but would currently the relative order be kept and thus a.h always be the main header?

I don't think you will find someone (of the usual reviewers) to be in favor of the pragma, and I certainly don't want such a thing in my code.

A heuristic seems to be a good way. I'd say from all the candidates we take the one without any directories in it.

[Febbe]

clang/unittests/Format/SortIncludesTest.cpp

548

but would currently the relative order be kept and thus a.h always be the main header?

No, unfortunately, clang-format never sorted the path after its dept, std::filesystem::path would do that, but clang-format uses the std::less operator of StringRef which is a lexicographical comparison.
Therefore, m.h will always be ordered after llvm/m.h. Changing that would reorder nearly all headers in all projects using clang-format. But I could do this only for the main-include candidates.

Nevertheless, I don't think this would be a good heuristic either, because me, and many others are using an extra include folder for public headers (API) and the src directory for private headers:

my-lib/
├─ include/
│  ├─ public_headers.md
│  ├─ a.h
├─ src/
│  ├─ private_headers_and_source.md
│  ├─ a_priv.h
│  ├─ a.cpp

With your proposed solution, the primary main header is ordered after the main include.
Let me implement my heuristic first, maybe it will work well enough to prevent the reordering, when the main header is sorted by hand (just like before, but now it also should work for negative priorities).

Otherwise, we must reconsider which tradeoff we choose (maybe a completely new one).

clang/lib/Format/Format.cpp

FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.FixNamespaceComments = true;
   LLVMStyle.ForEachMacros.push_back("foreach");
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IfMacros.push_back("KJ_IF_MAYBE");
   LLVMStyle.IncludeStyle.IncludeCategories = {
       {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
       {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
       {".*", 1, 0, false}};
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
   LLVMStyle.IndentAccessModifiers = false;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentExternBlock = FormatStyle::IEBS_AfterExternBlock;
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
if (!affectsRange(Ranges, IncludesBeginOffset, IncludesEndOffset))
     return;
   SmallVector<unsigned, 16> Indices =
       llvm::to_vector<16>(llvm::seq<unsigned>(0, Includes.size()));
 
   if (Style.SortIncludes == FormatStyle::SI_CaseInsensitive) {
     llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
       const auto LHSFilenameLower = Includes[LHSI].Filename.lower();
       const auto RHSFilenameLower = Includes[RHSI].Filename.lower();
-      return std::tie(Includes[LHSI].Priority, LHSFilenameLower,
-                      Includes[LHSI].Filename) <
-             std::tie(Includes[RHSI].Priority, RHSFilenameLower,
-                      Includes[RHSI].Filename);
+      return std::tie(Includes[LHSI].Category, Includes[LHSI].Priority,
+                      LHSFilenameLower, Includes[LHSI].Filename) <
+             std::tie(Includes[RHSI].Category, Includes[RHSI].Priority,
+                      RHSFilenameLower, Includes[RHSI].Filename);
     });
   } else {
     llvm::stable_sort(Indices, [&](unsigned LHSI, unsigned RHSI) {
-      return std::tie(Includes[LHSI].Priority, Includes[LHSI].Filename) <
-             std::tie(Includes[RHSI].Priority, Includes[RHSI].Filename);
+      return std::tie(Includes[LHSI].Category, Includes[LHSI].Priority,
+                      Includes[LHSI].Filename) <
+             std::tie(Includes[RHSI].Category, Includes[RHSI].Priority,
+                      Includes[RHSI].Filename);
     });
   }
 
   // The index of the include on which the cursor will be put after
   // sorting/deduplicating.
   unsigned CursorIndex;
   // The offset from cursor to the end of line.
   unsigned CursorToEOLOffset;
tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
                                       unsigned *Cursor) {
   unsigned Prev = llvm::StringSwitch<size_t>(Code)
                       .StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
                       .Default(0);
   unsigned SearchFrom = 0;
   SmallVector<StringRef, 4> Matches;
   SmallVector<IncludeDirective, 16> IncludesInBlock;
 
-  // In compiled files, consider the first #include to be the main #include of
-  // the file if it is not a system #include. This ensures that the header
-  // doesn't have hidden dependencies
-  // (http://llvm.org/docs/CodingStandards.html#include-style).
-  //
   // FIXME: Do some validation, e.g. edit distance of the base name, to fix
   // cases where the first #include is unlikely to be the main header.
   tooling::IncludeCategoryManager Categories(Style.IncludeStyle, FileName);
   bool FirstIncludeBlock = true;
-  bool MainIncludeFound = false;
+  // Todo(remove before merge): removed: multiple files can be main includes
+  // This option caused random sorting, depending which was detected first.
   bool FormattingOff = false;
 
   // '[' must be the first and '-' the last character inside [...].
   llvm::Regex RawStringRegex(
       "R\"([][A-Za-z0-9_{}#<>%:;.?*+/^&\\$|~!=,'-]*)\\(");
   SmallVector<StringRef, 2> RawStringMatches;
   std::string RawStringTermination = ")\"";
 
if (!FormattingOff && !MergeWithNextLine) {
           // FIXME: This is somehow simplified check that probably does not work
           // correctly if there are multiple comments on a line.
           Pos = Code.find("*/", SearchFrom);
           Line = Code.substr(
               Prev, (Pos != StringRef::npos ? Pos + 2 : Code.size()) - Prev);
         }
         int Category = Categories.getIncludePriority(
             IncludeName,
-            /*CheckMainHeader=*/!MainIncludeFound && FirstIncludeBlock);
-        int Priority = Categories.getSortIncludePriority(
-            IncludeName, !MainIncludeFound && FirstIncludeBlock);
-        if (Category == 0)
-          MainIncludeFound = true;
+            /*CheckMainHeader=*/FirstIncludeBlock);
+        int Priority =
+            Categories.getSortIncludePriority(IncludeName, FirstIncludeBlock);
+        // Todo(remove before merge): reason for removal: every header can have
+        // 0,0 priority
         IncludesInBlock.push_back(
             {IncludeName, Line, Prev, Category, Priority});
       } else if (!IncludesInBlock.empty() && !EmptyLineSkipped) {
         sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code,
                         Replaces, Cursor);
         IncludesInBlock.clear();
         if (Trimmed.startswith("#pragma hdrstop")) // Precompiled headers.
           FirstIncludeBlock = true;

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

 //===--- HeaderIncludes.cpp - Insert/Delete #includes --*- C++ -*----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
+#include <limits>
 #include <optional>
 
 namespace clang {
 namespace tooling {
 namespace {
 
 LangOptions createLangOpts() {
   LangOptions LangOpts;
   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = 1;
IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
                FileName.endswith(".cxx") || FileName.endswith(".m") ||
                FileName.endswith(".mm");
   if (!Style.IncludeIsMainSourceRegex.empty()) {
     llvm::Regex MainFileRegex(Style.IncludeIsMainSourceRegex);
     IsMainFile |= MainFileRegex.match(FileName);
   }
 }
 
+constexpr int DefaultMainIncludePriority = 0;
+constexpr int DefaultMainIncludeSortPriority = 0;
+
 int IncludeCategoryManager::getIncludePriority(StringRef IncludeName,
                                                bool CheckMainHeader) const {
-  int Ret = INT_MAX;
+  if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+    return DefaultMainIncludePriority;
+
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
-    if (CategoryRegexs[i].match(IncludeName)) {
-      Ret = Style.IncludeCategories[i].Priority;
-      break;
-    }
-  if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
-    Ret = 0;
-  return Ret;
+    if (CategoryRegexs[i].match(IncludeName))
+      return Style.IncludeCategories[i].Priority;
+
+  return std::numeric_limits<int>::max();
 }
 
 int IncludeCategoryManager::getSortIncludePriority(StringRef IncludeName,
                                                    bool CheckMainHeader) const {
-  int Ret = INT_MAX;
+  if (CheckMainHeader && IsMainFile && isMainHeader(IncludeName))
+    return DefaultMainIncludeSortPriority;
+
   for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i)
-    if (CategoryRegexs[i].match(IncludeName)) {
-      Ret = Style.IncludeCategories[i].SortPriority;
-      if (Ret == 0)
-        Ret = Style.IncludeCategories[i].Priority;
-      break;
-    }
-  if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
-    Ret = 0;
-  return Ret;
+    if (CategoryRegexs[i].match(IncludeName))
+      return Style.IncludeCategories[i].SortPriority;
+
+  return std::numeric_limits<int>::max();
 }
+
 bool IncludeCategoryManager::isMainHeader(StringRef IncludeName) const {
   if (!IncludeName.startswith("\""))
     return false;
 
   IncludeName =
       IncludeName.drop_front(1).drop_back(1); // remove the surrounding "" or <>
   // Not matchingStem: implementation files may have compound extensions but
   // headers may not.

clang/lib/Tooling/Inclusions/IncludeStyle.cpp

 //===--- IncludeStyle.cpp - Style of C++ #include directives -----*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
+#include <limits>
 
 using clang::tooling::IncludeStyle;
 
 namespace llvm {
 namespace yaml {
 
+// Todo (remove before merge): changes here are required,
+// because the explicit override for the default values of 0 are moved from
+// the algorithm to this place
+// Since we can't make those values required, we must set the previously
+// intended default values here, to prevent behaviour changes.
+
+constexpr int DefaultPriority = std::numeric_limits<int>::max();
+
 void MappingTraits<IncludeStyle::IncludeCategory>::mapping(
     IO &IO, IncludeStyle::IncludeCategory &Category) {
   IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapOptional("Priority", Category.Priority, DefaultPriority);
+  IO.mapOptional("SortPriority", Category.SortPriority, Category.Priority);
   IO.mapOptional("CaseSensitive", Category.RegexIsCaseSensitive);
 }
 
 void ScalarEnumerationTraits<IncludeStyle::IncludeBlocksStyle>::enumeration(
     IO &IO, IncludeStyle::IncludeBlocksStyle &Value) {
   IO.enumCase(Value, "Preserve", IncludeStyle::IBS_Preserve);
   IO.enumCase(Value, "Merge", IncludeStyle::IBS_Merge);
   IO.enumCase(Value, "Regroup", IncludeStyle::IBS_Regroup);
 }
 
 } // namespace yaml
 } // namespace llvm

clang/unittests/Format/SortIncludesTest.cpp

 //===- unittest/Format/SortIncludesTest.cpp - Include sort unit tests -----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FormatTestUtils.h"
 #include "clang/Format/Format.h"
+#include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Debug.h"
 #include "gtest/gtest.h"
 
 #define DEBUG_TYPE "format-test"
 
 namespace clang {
 namespace format {
EXPECT_EQ("#include <a>\n"
             "#include \"llvm/a.h\"\n",
             sort("#include <a>\n"
                  "\n"
                  "#include \"llvm/a.h\"\n"
                  "#include \"c.h\"\n"
                  "#include \"b.h\"\n",
                  "a.cc"));
 
-  // Only recognize the first #include with a matching basename as main include.
+  // Todo (remove-before-merge): I consider the assumption, that there is only
+  // one main include as wrong.
+  // E.g. a.cpp -> a.priv.h && a.h
+  // E.g. a_test.cpp -> a_test.h && a.h
+  // Maybe add a "//clang-format pragma: not_main" to remove false positives
+
+  // Recognize all possible main #include's with a matching basename as main
+  // include.
   EXPECT_EQ("#include \"a.h\"\n"
+            "#include \"llvm/a.h\"\n"
             "#include \"b.h\"\n"
+            "#include \"c.h\"\n",
+            sort("#include \"b.h\"\n"
+                 "#include \"a.h\"\n"
                  "#include \"c.h\"\n"
                  "#include \"llvm/a.h\"\n",
+                 "a.cc"));
+
+  // Keep manually splitted groups in place
+  EXPECT_EQ("#include \"a.h\"\n"
+            "#include \"b.h\"\n"
+            "#include \"c.h\"\n"
+            "// no main:\n"
+            "#include \"llvm/a.h\"\n",
             sort("#include \"b.h\"\n"
                  "#include \"a.h\"\n"
                  "#include \"c.h\"\n"
+                 "// no main:\n"
                  "#include \"llvm/a.h\"\n",
                  "a.cc"));
+
+  // Keep both main files together (with priority 0)
+  Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  EXPECT_EQ("#include \"a.h\"\n"
+            "#include \"a_test.h\"\n"
+            "#include \"b.h\"\n"
+            "#include \"c.h\"\n",
+            sort("#include \"b.h\"\n"
+                 "#include \"a.h\"\n"
+                 "#include \"c.h\"\n"
+                 "#include \"a_test.h\"\n",
+                 "a_test.cc"));
 }
 
 TEST_F(SortIncludesTest, LeavesMainHeaderFirstInAdditionalExtensions) {
   Style.IncludeIsMainRegex = "([-_](test|unittest))?|(Impl)?$";
   EXPECT_EQ("#include \"b.h\"\n"
             "#include \"c.h\"\n"
             "#include \"llvm/a.h\"\n",
             sort("#include \"llvm/a.h\"\n"
TEST_F(SortIncludesTest, NegativePriorities) {
   // check stable when re-run
   EXPECT_EQ("#include \"important_os_header.h\"\n"
             "#include \"c_main.h\"\n"
             "#include \"a_other.h\"\n",
             sort("#include \"important_os_header.h\"\n"
                  "#include \"c_main.h\"\n"
                  "#include \"a_other.h\"\n",
                  "c_main.cc", 0));
+  // All negative
+  Style.IncludeCategories = {{".*important_os_header.*", -3, 0, false},
+                             {".*", -2, 0, false}};
+
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+            "#include \"a_other.h\"\n"
+            "#include \"c_main.h\"\n",
+            sort("#include \"c_main.h\"\n"
+                 "#include \"a_other.h\"\n"
+                 "#include \"important_os_header.h\"\n",
+                 "c_main.cc"));
+}
+
+TEST_F(SortIncludesTest, ZeroPriorities) {
+  Style.IncludeCategories = {{".*important_os_header.*", 0, -1, false},
+                             {".*", 1, 0, false}};
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+            "#include \"c_main.h\"\n"
+            "#include \"a_other.h\"\n",
+            sort("#include \"c_main.h\"\n"
+                 "#include \"a_other.h\"\n"
+                 "#include \"important_os_header.h\"\n",
+                 "c_main.cc"));
+
+  // check stable when re-run
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+            "#include \"c_main.h\"\n"
+            "#include \"a_other.h\"\n",
+            sort("#include \"important_os_header.h\"\n"
+                 "#include \"c_main.h\"\n"
+                 "#include \"a_other.h\"\n",
+                 "c_main.cc", 0));
+
+  // All zero, sorted after name
+  Style.IncludeCategories = {{".*important_os_header.*", 0, 0, false},
+                             {".*", 0, 0, false}};
+
+  EXPECT_EQ("#include \"a_other.h\"\n"
+            "#include \"c_main.h\"\n"
+            "#include \"important_os_header.h\"\n",
+            sort("#include \"important_os_header.h\"\n"
+                 "#include \"c_main.h\"\n"
+                 "#include \"a_other.h\"\n",
+                 "c_main.cc"));
+
+  // Grouped:
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
+  Style.IncludeCategories = {{".*important_os_header.*", 0, -1, false},
+                             {".*", 1, 0, false}};
+
+  EXPECT_EQ("#include \"important_os_header.h\"\n"
+            "#include \"c_main.h\"\n"
+            "\n"
+            "#include \"a_other.h\"\n",
+            sort("#include \"a_other.h\"\n"
+                 "#include \"c_main.h\"\n"
+                 "#include \"important_os_header.h\"\n",
+                 "c_main.cc"));
 }
 
 TEST_F(SortIncludesTest, PriorityGroupsAreSeparatedWhenRegroupping) {
   Style.IncludeCategories = {{".*important_os_header.*", -1, 0, false},
                              {".*", 1, 0, false}};
   Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
 
   EXPECT_EQ("#include \"important_os_header.h\"\n"