clang-format IncludeBlocks: Regroup determination of "main" for framework-style includes fix

[poelmanc]

Summary

Most modern libraries and applications include files with relative paths from a root (e.g. #include <foo/bar/baz.h> versus #include "baz.h".) When regrouping, a file's "main include" should be left at the top (given priority 0.) The existing IncludeCategoryManager::isMainHeader() checks only the file stem (e.g. baz.h), and fails to identify main includes with angle brackets despite a comment saying // remove the surrounding "" or <>.

This patch fixes these issues by comparing all directory components present in both the #include string and the file name, and by allowing angle bracket includes to be considered "main". It adds a new IsMainHeader unit test to check behavior of framework-style includes, which would fail without this patch.

[HazardyKnusperkeks]

clang/unittests/Tooling/HeaderIncludesTest.cpp

106

I would also add "baz.h", since the test is named IsMainHeader.

[MyDeveloperDay]

I think I'd be concern about the <string.h> case (lets ignore the fact that I should be including <string>

so imagine I'm writing a string class and I have

string.cpp including "string.h"

but as part of my string class because I'm somehow extending std::string

#include "string.h"
#include <string.h>

class mystring : public std::string
{
}

Which file wins?

I think lots of people will always consider <> to mean system paths, I think if there is any chance that <> could change the order then I think it needs to be an option that is off by default.

I don't think this is a bug, I think you are asking for an enhancement.

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

237

Does this need to be an option?

[poelmanc]

Thanks for the speedy reply and the great tool. With appropriate Regex and Priority settings, IncludeCategories worked flawlessly for me aside from not identifying the main header. Treating #include "foo/bar/baz.h" as the main header in file bar/random/baz.h seems like a bug, but I certainly see the dangers of changing current <> behavior. I also considered treating <> includes as main headers only if they also contain a forward-slash, e.g.:

if (!IncludeName.startswith("\"") && !IncludeName.contains("/"))
  return false;

That would resolve the <string.h> case, although #include <sys/types.h> in a file anything/sys/types.h would be identified as the main header. So making an option seems like the cleanest solution. Say, bool IncludeIsMainAllowBraces?

[poelmanc]

In D96744#2564828, @MyDeveloperDay wrote:

Does this need to be an option?

It's easy to add an option, but there are already two main include-related options, so before adding a third I wanted to give this some thought. As someone new to IncludeCategories, I was genuinely impressed at how easy it was to use and how it gave me such complete control over the grouping via regular expressions. Yet in comparison the determination of main headers was less clear and more hard-coded; I had to examine the source code to figure out that the comparison is case-insensitive, it doesn't consider <> includes, only file stems are considered (e.g. the foo/bar in foo/bar/baz.h is ignored), and the behaviors of IncludeIsMainSourceRegex and IncludeIsMainRegex were a bit murky.

That's all fixable with a patch and minor documentation tweaks, but I wanted to consider some alternatives. Users of the IncludeCategories feature are comfortable with regular expressions, so imagine eliminating special handling of main headers entirely, and instead enabling users to write their own IncludeCategories rules for putting main headers first? We'd need to give them some bits of the source file path to use in their Regex, say called ${SourceStem} and ${SourcePath}.

Users unconcerned with formatting foo_test.cc or foo_proto.cc files could get the current functionality by including a simple rule like:

- Regex:           '"(.*\/)?${SourceStem}(\..*)?"'
  Priority:        0
  CaseSensitive:   false

I want case sensitivity, matching at least one component of the path, and angle brackets, so I'd use something like:

- Regex:           '[<"]${SourcePath:1}${SourceStem}\..*[>"]'
  Priority:        0
  CaseSensitive:   true

Then adding a generally-useful ApplyToSourcesRegex feature to apply any category regex only to certain source files, and an ability to trim a regular expression from the end of ${SourceStem}, gives users full control, including mimicking current isMainHeader() behavior with rules like:

- Regex:           '"(.*\/)?${SourceStem-<insert-old-IncludeIsMainRegex>}(\..*)?"'
  Priority:        0
  CaseSensitive:   false
  ApplyToSourcesRegex: `((.*\.(c|cc|cpp|c\+\+|cxx|m|mm))|<insert-old-IncludeIsMainSourceRegex>)`

For backwards-compatibility we'd automatically add the above rule if no special ${Source tokens appear in any rules, and we can deprecate IncludeIsMainRegex and IncludeIsMainSourceRegex at any point. The special tokens for use in Regex are defined as:

• ${SourcePath:<n>} is a regular expression matching n or more trailing components of the source directory file path, using forward slashes and including any trailing slash (0 <= n < 10)
• ${SourceStem} is a string starting after the last / in the source file path and includes up to but excluding the first dot
• ${SourceStem-<re>} is ${SourceStem} with any trailing characters matching regular expression re removed

I have this coded up and it passes the ToolingTests, but wanted to run the idea by others. Thoughts? Should I update this patch with these changes? Start a separate issue? Stick with a new option IncludeIsMainAllowBraces?

[mitchell-stellar]

I am in favor of a new option, IncludeIsMainAllowBraces. It keeps things simple and does not cause any backwards-compatibility issues. The ${} variables are clever, but if "users unconcerned with formatting ... could get the current functionality by including a simple rule [that includes a variable]", I think it might be asking too much. Being able to write (and read) an include regex without referring to documentation seems ideal to me. You mentioned automatically adding variables in rules for backwards-compatibility, but I'd be a bit concerned about robustness and too much magic going on behind the scenes for users to understand if something goes wrong or has unexpected/unexplained results.

Admittedly I don't fully comprehend the proposed additions and all their ins-and-outs, but I do understand a new, simple option.

[poelmanc]

We eventually updated our coding standards and our fairly massive code base to change only the very top "main" include from #include <path/to/Foo.h> to #include "Foo.h".

For all include statements after that, we still use framework-style includes (#include <path/to/Bar.h>).

It worked great and now we're happily using clang-format to order and group all our include statements, with no need for any changes to clang-format. Thanks to all for maintaining such an amazing tool!

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

if (CategoryRegexs[i].match(IncludeName)) {
         Ret = Style.IncludeCategories[i].Priority;
       break;
     }
   if (CheckMainHeader && IsMainFile && Ret > 0 && isMainHeader(IncludeName))
     Ret = 0;
   return Ret;
 }
 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.
   StringRef HeaderStem = llvm::sys::path::stem(IncludeName);
   StringRef FileStem = llvm::sys::path::stem(FileName); // foo.cu for foo.cu.cc
   StringRef MatchingFileStem = matchingStem(FileName);  // foo for foo.cu.cc
   // main-header examples:
   //  1) foo.h => foo.cc
   //  2) foo.h => foo.cu.cc
   //  3) foo.proto.h => foo.proto.cc
   //
   // non-main-header examples:
   //  1) foo.h => bar.cc
   //  2) foo.proto.h => foo.cc
   StringRef Matching;
   if (MatchingFileStem.startswith_lower(HeaderStem))
     Matching = MatchingFileStem; // example 1), 2)
   else if (FileStem.equals_lower(HeaderStem))
     Matching = FileStem; // example 3)
   if (!Matching.empty()) {
     llvm::Regex MainIncludeRegex(HeaderStem.str() + Style.IncludeIsMainRegex,
                                  llvm::Regex::IgnoreCase);
-    if (MainIncludeRegex.match(Matching))
+    if (MainIncludeRegex.match(Matching)) {
+      // Matching is non-empty so these should be non-empty as well, making
+      // ++rbegin(IncludeName) and ++rbegin(FileName) safe.
+      assert(!IncludeName.empty());
+      assert(!FileName.empty());
+      // Checked stems above. Check remaining common path components here.
+      auto IncludePathRIter = ++llvm::sys::path::rbegin(IncludeName);
+      auto FilePathRiter = ++llvm::sys::path::rbegin(FileName);
+      for (; IncludePathRIter != llvm::sys::path::rend(IncludeName) &&
+             FilePathRiter != llvm::sys::path::rend(FileName);
+           ++IncludePathRIter, ++FilePathRiter) {
+        if (*IncludePathRIter != *FilePathRiter)
+          return false;
+      }
       return true;
     }
+  }
   return false;
 }
 
 HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
                                const IncludeStyle &Style)
     : FileName(FileName), Code(Code), FirstIncludeOffset(-1),
       MinInsertOffset(
           getOffsetAfterHeaderGuardsAndComments(FileName, Code, Style)),

clang/unittests/Tooling/HeaderIncludesTest.cpp

std::string Expected = "#ifndef A_H\n"
                          "#include <vector>\n"
                          "class A {};\n"
                          "#define MMM 123\n"
                          "#endif";
 
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, IsMainHeader) {
+  Style = format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp)
+              .IncludeStyle;
+  std::vector<StringRef> FileNames{"foo/bar/baz.cpp", "foo/bar/baz.cu.cpp",
+                                   "foo/bar/baz_test.cu.cpp"};
+  for (const StringRef &FileName : FileNames) {
+    IncludeCategoryManager Manager(Style, FileName);
+    // These framework-style includes should all be considered "main".
+    EXPECT_EQ(Manager.getIncludePriority("<foo/bar/baz.h>", true), 0)
+        << "for source file " << FileName;
+    EXPECT_EQ(Manager.getIncludePriority("\"bar/baz.h\"", true), 0)
+        << "for source file " << FileName;
+    EXPECT_EQ(Manager.getIncludePriority("<baz.h>", true), 0)
+        << "for source file " << FileName;
+    // These should not be considered "main" as the paths to baz.h differ.
+    EXPECT_NE(Manager.getIncludePriority("<bar/foo/baz.h>", true), 0)
+        << "for source file " << FileName;
+    EXPECT_NE(Manager.getIncludePriority("\"foo/baz.h\"", true), 0)
+        << "for source file " << FileName;
+  }
+}
+
 TEST_F(HeaderIncludesTest, InsertAfterMainHeader) {
   std::string Code = "#include \"fix.h\"\n"
                      "\n"
                      "int main() {}";
   std::string Expected = "#include \"fix.h\"\n"
                          "#include <a>\n"
                          "\n"
                          "int main() {}";