Revert "[clang-format] SortIncludes should support "@import" lines in Objective-C"

[krasimir]

Summary

This reverts commit d46fa023caa2db5a9f1e21dd038bcb626261d958.

Regressed include order in some cases with trailing comments, see the
comments on https://reviews.llvm.org/D121370.

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h

private:
 
   // All possible priorities.
   std::set<int> Priorities;
 
   // Matches a whole #include directive.
   llvm::Regex IncludeRegex;
 };
 
-/// \returns a regex that can match various styles of C++ includes.
-/// For example:
-/// \code
-/// #include <foo.h>
-/// @import bar;
-/// #include "bar.h"
-/// \endcode
-llvm::Regex getCppIncludeRegex();
-
-/// \returns the last match in the list of matches that is not empty.
-llvm::StringRef getIncludeNameFromMatches(
-    const llvm::SmallVectorImpl<llvm::StringRef> &Matches);
-
-/// \returns the given include name and removes the following symbols from the
-/// beginning and ending of the include name: " > < ;
-llvm::StringRef trimInclude(llvm::StringRef IncludeName);
-
 } // namespace tooling
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_INCLUSIONS_HEADERINCLUDES_H

clang/lib/Format/Format.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <algorithm>
-#include <limits>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <unordered_map>
 
 #define DEBUG_TYPE "format-formatter"
 
 using clang::format::FormatStyle;
static void sortCppIncludes(const FormatStyle &Style,
   // FIXME: better error handling. For now, just skip the replacement for the
   // release version.
   if (Err) {
     llvm::errs() << llvm::toString(std::move(Err)) << "\n";
     assert(false);
   }
 }
 
+namespace {
+
+const char CppIncludeRegexPattern[] =
+    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+
+} // anonymous namespace
+
 tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
                                       ArrayRef<tooling::Range> Ranges,
                                       StringRef FileName,
                                       tooling::Replacements &Replaces,
                                       unsigned *Cursor) {
   unsigned Prev = llvm::StringSwitch<size_t>(Code)
                       .StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
                       .Default(0);
   unsigned SearchFrom = 0;
-  llvm::Regex IncludeRegex(tooling::getCppIncludeRegex());
+  llvm::Regex IncludeRegex(CppIncludeRegexPattern);
   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).
   //
const bool EmptyLineSkipped =
         Trimmed.empty() &&
         (Style.IncludeStyle.IncludeBlocks == tooling::IncludeStyle::IBS_Merge ||
          Style.IncludeStyle.IncludeBlocks ==
              tooling::IncludeStyle::IBS_Regroup);
 
     bool MergeWithNextLine = Trimmed.endswith("\\");
     if (!FormattingOff && !MergeWithNextLine) {
       if (IncludeRegex.match(Line, &Matches)) {
-        StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
-        // This addresses https://github.com/llvm/llvm-project/issues/38995
-        bool WithSemicolon = false;
-        if (!IncludeName.startswith("\"") && !IncludeName.startswith("<") &&
-            IncludeName.endswith(";")) {
-          WithSemicolon = true;
-        }
-
+        StringRef IncludeName = Matches[2];
         if (Line.contains("/*") && !Line.contains("*/")) {
           // #include with a start of a block comment, but without the end.
           // Need to keep all the lines until the end of the comment together.
           // 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 = WithSemicolon ? std::numeric_limits<int>::max()
-                                     : Categories.getSortIncludePriority(
-                                           IncludeName, !MainIncludeFound &&
-                                                            FirstIncludeBlock);
+        int Priority = Categories.getSortIncludePriority(
+            IncludeName, !MainIncludeFound && FirstIncludeBlock);
         if (Category == 0)
           MainIncludeFound = true;
         IncludesInBlock.push_back(
             {IncludeName, Line, Prev, Category, Priority});
       } else if (!IncludesInBlock.empty() && !EmptyLineSkipped) {
         sortCppIncludes(Style, IncludesInBlock, Ranges, FileName, Code,
                         Replaces, Cursor);
         IncludesInBlock.clear();
formatReplacements(StringRef Code, const tooling::Replacements &Replaces,
   };
   return processReplacements(Reformat, Code, *SortedReplaces, Style);
 }
 
 namespace {
 
 inline bool isHeaderInsertion(const tooling::Replacement &Replace) {
   return Replace.getOffset() == UINT_MAX && Replace.getLength() == 0 &&
-         tooling::getCppIncludeRegex().match(Replace.getReplacementText());
+         llvm::Regex(CppIncludeRegexPattern)
+             .match(Replace.getReplacementText());
 }
 
 inline bool isHeaderDeletion(const tooling::Replacement &Replace) {
   return Replace.getOffset() == UINT_MAX && Replace.getLength() == 1;
 }
 
 // FIXME: insert empty lines between newly created blocks.
-static tooling::Replacements
+tooling::Replacements
 fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
                         const FormatStyle &Style) {
   if (!Style.isCpp())
     return Replaces;
 
   tooling::Replacements HeaderInsertions;
   std::set<llvm::StringRef> HeadersToDelete;
   tooling::Replacements Result;
fixCppIncludeInsertions(StringRef Code, const tooling::Replacements &Replaces,
   if (HeaderInsertions.empty() && HeadersToDelete.empty())
     return Replaces;
 
   StringRef FileName = Replaces.begin()->getFilePath();
   tooling::HeaderIncludes Includes(FileName, Code, Style.IncludeStyle);
 
   for (const auto &Header : HeadersToDelete) {
     tooling::Replacements Replaces =
-        Includes.remove(tooling::trimInclude(Header), Header.startswith("<"));
+        Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
     for (const auto &R : Replaces) {
       auto Err = Result.add(R);
       if (Err) {
         // Ignore the deletion on conflict.
         llvm::errs() << "Failed to add header deletion replacement for "
                      << Header << ": " << llvm::toString(std::move(Err))
                      << "\n";
       }
     }
   }
 
-  llvm::Regex IncludeRegex = tooling::getCppIncludeRegex();
+  llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
   llvm::SmallVector<StringRef, 4> Matches;
   for (const auto &R : HeaderInsertions) {
     auto IncludeDirective = R.getReplacementText();
     bool Matched = IncludeRegex.match(IncludeDirective, &Matches);
     assert(Matched && "Header insertion replacement must have replacement text "
                       "'#include ...'");
     (void)Matched;
-    StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
-    auto Replace = Includes.insert(tooling::trimInclude(IncludeName),
-                                   IncludeName.startswith("<"));
+    auto IncludeName = Matches[2];
+    auto Replace =
+        Includes.insert(IncludeName.trim("\"<>"), IncludeName.startswith("<"));
     if (Replace) {
       auto Err = Result.add(*Replace);
       if (Err) {
         llvm::consumeError(std::move(Err));
         unsigned NewOffset =
             Result.getShiftedCodePosition(Replace->getOffset());
         auto Shifted = tooling::Replacement(FileName, NewOffset, 0,
                                             Replace->getReplacementText());

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

return getOffsetAfterTokenSequence(
         skipComments(Lex, Tok);
         unsigned MaxOffset = SM.getFileOffset(Tok.getLocation());
         while (checkAndConsumeInclusiveDirective(Lex, Tok))
           MaxOffset = SM.getFileOffset(Tok.getLocation());
         return MaxOffset;
       });
 }
 
+inline StringRef trimInclude(StringRef IncludeName) {
+  return IncludeName.trim("\"<>");
+}
+
+const char IncludeRegexPattern[] =
+    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
+
 // The filename of Path excluding extension.
 // Used to match implementation with headers, this differs from sys::path::stem:
 //  - in names with multiple dots (foo.cu.cc) it terminates at the *first*
 //  - an empty stem is never returned: /foo/.bar.x => .bar
 //  - we don't bother to handle . and .. specially
 StringRef matchingStem(llvm::StringRef Path) {
   StringRef Name = llvm::sys::path::filename(Path);
   return Name.substr(0, Name.find('.', 1));

 HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
                                const IncludeStyle &Style)
     : FileName(FileName), Code(Code), FirstIncludeOffset(-1),
       MinInsertOffset(
           getOffsetAfterHeaderGuardsAndComments(FileName, Code, Style)),
       MaxInsertOffset(MinInsertOffset +
                       getMaxHeaderInsertionOffset(
                           FileName, Code.drop_front(MinInsertOffset), Style)),
-      Categories(Style, FileName), IncludeRegex(getCppIncludeRegex()) {
+      Categories(Style, FileName),
+      IncludeRegex(llvm::Regex(IncludeRegexPattern)) {
   // Add 0 for main header and INT_MAX for headers that are not in any
   // category.
   Priorities = {0, INT_MAX};
   for (const auto &Category : Style.IncludeCategories)
     Priorities.insert(Category.Priority);
   SmallVector<StringRef, 32> Lines;
   Code.drop_front(MinInsertOffset).split(Lines, "\n");
 
   unsigned Offset = MinInsertOffset;
   unsigned NextLineOffset;
   SmallVector<StringRef, 4> Matches;
   for (auto Line : Lines) {
     NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);
     if (IncludeRegex.match(Line, &Matches)) {
-      StringRef IncludeName = tooling::getIncludeNameFromMatches(Matches);
       // If this is the last line without trailing newline, we need to make
       // sure we don't delete across the file boundary.
       addExistingInclude(
-          Include(IncludeName,
+          Include(Matches[2],
                   tooling::Range(
                       Offset, std::min(Line.size() + 1, Code.size() - Offset))),
           NextLineOffset);
     }
     Offset = NextLineOffset;
   }
 
   // Populate CategoryEndOfssets:
if (Err) {
       auto ErrMsg = "Unexpected conflicts in #include deletions: " +
                     llvm::toString(std::move(Err));
       llvm_unreachable(ErrMsg.c_str());
     }
   }
   return Result;
 }
 
-llvm::Regex getCppIncludeRegex() {
-  static const char CppIncludeRegexPattern[] =
-      R"(^[\t\ ]*[@#][\t\ ]*(import|include)([^"]*("[^"]+")|[^<]*(<[^>]+>)|[\t\ ]*([^;]+;)))";
-  return llvm::Regex(CppIncludeRegexPattern);
-}
-
-llvm::StringRef getIncludeNameFromMatches(
-    const llvm::SmallVectorImpl<llvm::StringRef> &Matches) {
-  for (auto Match : llvm::reverse(Matches)) {
-    if (!Match.empty())
-      return Match;
-  }
-  llvm_unreachable("No non-empty match group found in list of matches");
-  return llvm::StringRef();
-}
-
-llvm::StringRef trimInclude(llvm::StringRef IncludeName) {
-  return IncludeName.trim("\"<>;");
-}
-
 } // namespace tooling
 } // namespace clang

clang/unittests/Format/SortIncludesTest.cpp

EXPECT_EQ("#include \"a.h\"\n"
             "#include \"b.h\"\n"
             "#include \"c.h\"\n",
             sort("#include \"a.h\"\n"
                  "#include \\\n"
                  "\"c.h\"\n"
                  "#include \"b.h\"\n"));
 }
 
-TEST_F(SortIncludesTest, SupportAtImportLines) {
-  // Test from https://github.com/llvm/llvm-project/issues/38995
-  EXPECT_EQ("#import \"a.h\"\n"
-            "#import \"b.h\"\n"
-            "#import \"c.h\"\n"
-            "#import <d/e.h>\n"
-            "@import Foundation;\n",
-            sort("#import \"b.h\"\n"
-                 "#import \"c.h\"\n"
-                 "#import <d/e.h>\n"
-                 "@import Foundation;\n"
-                 "#import \"a.h\"\n"));
-
-  // Slightly more complicated test that shows sorting in each priorities still
-  // works.
-  EXPECT_EQ("#import \"a.h\"\n"
-            "#import \"b.h\"\n"
-            "#import \"c.h\"\n"
-            "#import <d/e.h>\n"
-            "@import Base;\n"
-            "@import Foundation;\n"
-            "@import base;\n"
-            "@import foundation;\n",
-            sort("#import \"b.h\"\n"
-                 "#import \"c.h\"\n"
-                 "@import Base;\n"
-                 "#import <d/e.h>\n"
-                 "@import foundation;\n"
-                 "@import Foundation;\n"
-                 "@import base;\n"
-                 "#import \"a.h\"\n"));
-
-  // Test that shows main headers in two groups are still found and sorting
-  // still works. The @import's are kept in their respective group but are
-  // put at the end of each group.
-  FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
-  EXPECT_EQ("#import \"foo.hpp\"\n"
-            "#import \"b.h\"\n"
-            "#import \"c.h\"\n"
-            "#import <d/e.h>\n"
-            "@import Base;\n"
-            "@import Foundation;\n"
-            "@import foundation;\n"
-            "\n"
-            "#import \"foo.h\"\n"
-            "#include \"foobar\"\n"
-            "#import <f/g.h>\n"
-            "@import AAAA;\n"
-            "@import aaaa;\n",
-            sort("#import \"b.h\"\n"
-                 "@import Foundation;\n"
-                 "@import foundation;\n"
-                 "#import \"c.h\"\n"
-                 "#import <d/e.h>\n"
-                 "@import Base;\n"
-                 "#import \"foo.hpp\"\n"
-                 "\n"
-                 "@import aaaa;\n"
-                 "#import <f/g.h>\n"
-                 "@import AAAA;\n"
-                 "#include \"foobar\"\n"
-                 "#import \"foo.h\"\n",
-                 "foo.c", 2));
-
-  // Regrouping and putting @import's in the very last group
-  FmtStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
-  EXPECT_EQ("#import \"foo.hpp\"\n"
-            "\n"
-            "#import \"b.h\"\n"
-            "#import \"c.h\"\n"
-            "#import \"foo.h\"\n"
-            "#include \"foobar\"\n"
-            "\n"
-            "#import <d/e.h>\n"
-            "#import <f/g.h>\n"
-            "\n"
-            "@import AAAA;\n"
-            "@import Base;\n"
-            "@import Foundation;\n"
-            "@import aaaa;\n"
-            "@import foundation;\n",
-            sort("#import \"b.h\"\n"
-                 "@import Foundation;\n"
-                 "@import foundation;\n"
-                 "#import \"c.h\"\n"
-                 "#import <d/e.h>\n"
-                 "@import Base;\n"
-                 "#import \"foo.hpp\"\n"
-                 "\n"
-                 "@import aaaa;\n"
-                 "#import <f/g.h>\n"
-                 "@import AAAA;\n"
-                 "#include \"foobar\"\n"
-                 "#import \"foo.h\"\n",
-                 "foo.c"));
-}
-
 TEST_F(SortIncludesTest, LeavesMainHeaderFirst) {
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
   EXPECT_EQ("#include \"llvm/a.h\"\n"
             "#include \"b.h\"\n"
             "#include \"c.h\"\n",
             sort("#include \"llvm/a.h\"\n"
                  "#include \"c.h\"\n"
                  "#include \"b.h\"\n",