[clang-tidy] IncludeInserter: allow <> in header name

[alexfh]

Summary

This adds a pair of overloads for create(MainFile)?IncludeInsertion methods that
use the presence of the <> in the file name to control whether the #include
directive will use angle brackets or quotes. Motivating examples:
https://reviews.llvm.org/D82089#inline-789412 and
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp#L433

The overloads with the IsAngled parameter can be removed after the users are
updated.

Update usages of createIncludeInsertion.

Update (almost all) usages of createMainFileIncludeInsertion.

[hokein]

clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp

31

this looks like a bug fix, I think the check lit test didn't cover it before, maybe add a header verification in test/clang-tidy/checkers/cppcoreguidelines-init-variables.cpp.

clang-tools-extra/clang-tidy/utils/IncludeInserter.h

71

Add a FIXME saying this would be removed eventually.

85

The same here.

[njames93]

Should a line be added to the release notes to explain this behaviour:
'Checks that specify files to include now support wrapping the include in angle brackets to create a system include'?

[njames93]

clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp

98–103

To save duplication could this not just dispatch to createIncludeInsertion(FileID, StringRef)

[alexfh]

• clang-format
• addressed review comments

clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp

31

I've already added this ;)

clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp

98–103

Good point! Thanks!

[alexfh]

• Updated release notes and default option values in docs

clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp

void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) {
   Diagnostic << FixItHint::CreateReplacement(
       ComparisonExpr->getSourceRange(),
       (StartswithStr + "(" + HaystackExprCode + ", " + NeedleExprCode + ")")
           .str());
 
   // Create a preprocessor #include FixIt hint (CreateIncludeInsertion checks
   // whether this already exists).
   Diagnostic << IncludeInserter.createIncludeInsertion(
-      Source.getFileID(ComparisonExpr->getBeginLoc()), AbseilStringsMatchHeader,
-      false);
+      Source.getFileID(ComparisonExpr->getBeginLoc()), AbseilStringsMatchHeader);
 }
 
 void StringFindStartswithCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   IncludeInserter.registerPreprocessor(PP);
 }
 
 void StringFindStartswithCheck::storeOptions(

clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp

 AST_MATCHER(VarDecl, isLocalVarDecl) { return Node.isLocalVarDecl(); }
 } // namespace
 
 InitVariablesCheck::InitVariablesCheck(StringRef Name,
                                        ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
                                                utils::IncludeSorter::IS_LLVM)),
-      MathHeader(Options.get("MathHeader", "math.h")) {}
+      MathHeader(Options.get("MathHeader", "<math.h>")) {}
 
 void InitVariablesCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
   Options.store(Opts, "MathHeader", MathHeader);
 }
 
 void InitVariablesCheck::registerMatchers(MatchFinder *Finder) {
   std::string BadDecl = "badDecl";
auto Diagnostic =
         diag(MatchedDecl->getLocation(), "variable %0 is not initialized")
         << MatchedDecl
         << FixItHint::CreateInsertion(
                MatchedDecl->getLocation().getLocWithOffset(
                    MatchedDecl->getName().size()),
                InitializationString);
     if (AddMathInclude) {
       Diagnostic << IncludeInserter.createIncludeInsertion(
-          Source.getFileID(MatchedDecl->getBeginLoc()), MathHeader, false);
+          Source.getFileID(MatchedDecl->getBeginLoc()), MathHeader);
     }
   }
 }
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

auto Diag = diag(Matched->getExprLoc(),
                      "instead");
     if (!GslHeader.empty()) {
       Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
            << FixItHint::CreateReplacement(
                   SourceRange(BaseRange.getEnd().getLocWithOffset(1),
                               IndexRange.getBegin().getLocWithOffset(-1)),
                   ", ")
            << FixItHint::CreateReplacement(Matched->getEndLoc(), ")")
-           << Inserter.createMainFileIncludeInsertion(GslHeader,
-                                                      /*IsAngled=*/false);
+           << Inserter.createMainFileIncludeInsertion(GslHeader);
     }
     return;
   }
 
   const auto *StdArrayDecl =
       Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("type");
 
   // For static arrays, this is handled in clang-diagnostic-array-bounds.

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 namespace {
 
-constexpr char StdMemoryHeader[] = "memory";
 constexpr char ConstructorCall[] = "constructorCall";
 constexpr char ResetCall[] = "resetCall";
 constexpr char NewExpression[] = "newExpression";
 
 std::string GetNewExprName(const CXXNewExpr *NewExpr,
                            const SourceManager &SM,
                            const LangOptions &Lang) {
   StringRef WrittenName = Lexer::getSourceText(

 const char MakeSmartPtrCheck::PointerType[] = "pointerType";
 
 MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name, ClangTidyContext *Context,
                                      StringRef MakeSmartPtrFunctionName)
     : ClangTidyCheck(Name, Context),
       Inserter(Options.getLocalOrGlobal("IncludeStyle",
                                         utils::IncludeSorter::IS_LLVM)),
       MakeSmartPtrFunctionHeader(
-          Options.get("MakeSmartPtrFunctionHeader", StdMemoryHeader)),
+          Options.get("MakeSmartPtrFunctionHeader", "<memory>")),
       MakeSmartPtrFunctionName(
           Options.get("MakeSmartPtrFunction", MakeSmartPtrFunctionName)),
       IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
 
 void MakeSmartPtrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", Inserter.getStyle());
   Options.store(Opts, "MakeSmartPtrFunctionHeader", MakeSmartPtrFunctionHeader);
   Options.store(Opts, "MakeSmartPtrFunction", MakeSmartPtrFunctionName);
bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
   }
   return true;
 }
 
 void MakeSmartPtrCheck::insertHeader(DiagnosticBuilder &Diag, FileID FD) {
   if (MakeSmartPtrFunctionHeader.empty()) {
     return;
   }
-  Diag << Inserter.createIncludeInsertion(
-      FD, MakeSmartPtrFunctionHeader,
-      /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader);
+  Diag << Inserter.createIncludeInsertion(FD, MakeSmartPtrFunctionHeader);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
   }
 
   // Use std::move in the initialization list.
   Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")")
        << FixItHint::CreateInsertion(
               Initializer->getLParenLoc().getLocWithOffset(1), "std::move(")
        << Inserter.createIncludeInsertion(
               Result.SourceManager->getFileID(Initializer->getSourceLocation()),
-              "utility",
-              /*IsAngled=*/true);
+              "<utility>");
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp

CharSourceRange Range = Lexer::makeFileCharRange(
         CharSourceRange::getTokenRange(E->getSourceRange()), SM, LangOptions());
 
     if (Range.isInvalid())
       return;
 
     auto Diag = diag(Range.getBegin(), "use std::move to transfer ownership")
                 << FixItHint::CreateInsertion(Range.getBegin(), "std::move(")
                 << FixItHint::CreateInsertion(Range.getEnd(), ")")
-                << Inserter.createMainFileIncludeInsertion("utility",
-                                                           /*IsAngled=*/true);
+                << Inserter.createMainFileIncludeInsertion("<utility>");
 
     return;
   }
 
   SourceLocation AutoPtrLoc;
   if (const auto *TL = Result.Nodes.getNodeAs<TypeLoc>(AutoPtrTokenId)) {
     //   std::auto_ptr<int> i;
     //        ^

clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp

void ReplaceRandomShuffleCheck::check(const MatchFinder::MatchResult &Result) {
   if (ContainerText.startswith("std::"))
     NewName = "std::" + NewName;
 
   Diag << FixItHint::CreateRemoval(MatchedDecl->getSourceRange());
   Diag << FixItHint::CreateInsertion(MatchedDecl->getBeginLoc(), NewName);
   Diag << IncludeInserter.createIncludeInsertion(
       Result.Context->getSourceManager().getFileID(
           MatchedCallExpr->getBeginLoc()),
-      "random", /*IsAngled=*/true);
+      "<random>");
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp

void TypePromotionInMathFnCheck::check(const MatchFinder::MatchResult &Result) {
 
   // Suggest including <cmath> if the function we're suggesting is declared in
   // <cmath> and it's not already included.  We never have to suggest including
   // <math.h>, because the functions we're suggesting moving away from are all
   // declared in <math.h>.
   if (FnInCmath)
     Diag << IncludeInserter.createIncludeInsertion(
         Result.Context->getSourceManager().getFileID(Call->getBeginLoc()),
-        "cmath", /*IsAngled=*/true);
+        "<cmath>");
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

void UnnecessaryValueParamCheck::handleMoveFix(const ParmVarDecl &Var,
   if (CopyArgument.getBeginLoc().isMacroID())
     return;
   const auto &SM = Context.getSourceManager();
   auto EndLoc = Lexer::getLocForEndOfToken(CopyArgument.getLocation(), 0, SM,
                                            Context.getLangOpts());
   Diag << FixItHint::CreateInsertion(CopyArgument.getBeginLoc(), "std::move(")
        << FixItHint::CreateInsertion(EndLoc, ")")
        << Inserter.createIncludeInsertion(
-              SM.getFileID(CopyArgument.getBeginLoc()), "utility",
-              /*IsAngled=*/true);
+              SM.getFileID(CopyArgument.getBeginLoc()), "<utility>");
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/utils/IncludeInserter.h

public:
   explicit IncludeInserter(IncludeSorter::IncludeStyle Style);
 
   /// Registers this with the Preprocessor \p PP, must be called before this
   /// class is used.
   void registerPreprocessor(Preprocessor *PP);
 
   /// Creates a \p Header inclusion directive fixit in the File \p FileID.
   /// Returns ``llvm::None`` on error or if the inclusion directive already
   /// exists.
   llvm::Optional<FixItHint>
   createIncludeInsertion(FileID FileID, llvm::StringRef Header, bool IsAngled);
 
+  /// Creates a \p Header inclusion directive fixit in the File \p FileID.
+  /// When \p Header is enclosed in angle brackets, uses angle brackets in the
+  /// inclusion directive, otherwise uses quotes.
+  /// Returns ``llvm::None`` on error or if the inclusion directive already
+  /// exists.
+  llvm::Optional<FixItHint>
+  createIncludeInsertion(FileID FileID, llvm::StringRef Header);
+
   /// Creates a \p Header inclusion directive fixit in the main file.
   /// Returns``llvm::None`` on error or if the inclusion directive already
   /// exists.
   llvm::Optional<FixItHint>
   createMainFileIncludeInsertion(llvm::StringRef Header, bool IsAngled);
 
+  /// Creates a \p Header inclusion directive fixit in the main file.
+  /// When \p Header is enclosed in angle brackets, uses angle brackets in the
+  /// inclusion directive, otherwise uses quotes.
+  /// Returns``llvm::None`` on error or if the inclusion directive already
+  /// exists.
+  llvm::Optional<FixItHint>
+  createMainFileIncludeInsertion(llvm::StringRef Header);
+
   IncludeSorter::IncludeStyle getStyle() const { return Style; }
 
 private:
   void addInclude(StringRef FileName, bool IsAngled,
                   SourceLocation HashLocation, SourceLocation EndLocation);
 
   IncludeSorter &getOrCreate(FileID FileID);
 

clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp

IncludeInserter::createIncludeInsertion(FileID FileID, StringRef Header,
   // angled.
   if (!InsertedHeaders[FileID].insert(Header).second)
     return llvm::None;
 
   return getOrCreate(FileID).CreateIncludeInsertion(Header, IsAngled);
 }
 
 llvm::Optional<FixItHint>
+IncludeInserter::createIncludeInsertion(FileID FileID, llvm::StringRef Header) {
+  bool IsAngled = Header.consume_front("<");
+  if (IsAngled != Header.consume_back(">"))
+    return llvm::None;
+  return createIncludeInsertion(FileID, Header, IsAngled);
+}
+
+llvm::Optional<FixItHint>
 IncludeInserter::createMainFileIncludeInsertion(StringRef Header,
                                                 bool IsAngled) {
   assert(SourceMgr && "SourceMgr shouldn't be null; did you remember to call "
                       "registerPreprocessor()?");
   return createIncludeInsertion(SourceMgr->getMainFileID(), Header, IsAngled);
 }
 
+llvm::Optional<FixItHint>
+IncludeInserter::createMainFileIncludeInsertion(StringRef Header) {
+  assert(SourceMgr && "SourceMgr shouldn't be null; did you remember to call "
+                      "registerPreprocessor()?");
+  bool IsAngled = Header.consume_front("<");
+  if (IsAngled != Header.consume_back(">"))
+    return llvm::None;
+  return createIncludeInsertion(SourceMgr->getMainFileID(), Header, IsAngled);
+}
+
 void IncludeInserter::addInclude(StringRef FileName, bool IsAngled,
                                  SourceLocation HashLocation,
                                  SourceLocation EndLocation) {
   assert(SourceMgr && "SourceMgr shouldn't be null; did you remember to call "
                       "registerPreprocessor()?");
   FileID FileID = SourceMgr->getFileID(HashLocation);
   getOrCreate(FileID).AddInclude(FileName, IsAngled, HashLocation, EndLocation);
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// CHECK-FIXES: {{^}}#include <math.h>
 
 // Ensure that function declarations are not changed.
 void some_func(int x, double d, bool b, const char *p);
 
 // Ensure that function arguments are not changed
 int identity_function(int x) {
   return x;
 }

clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp

public:
 
   void registerMatchers(ast_matchers::MatchFinder *Finder) override {
     Finder->addMatcher(ast_matchers::declStmt().bind("stmt"), this);
   }
 
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override {
     auto Diag = diag(Result.Nodes.getNodeAs<DeclStmt>("stmt")->getBeginLoc(),
                      "foo, bar");
-    for (StringRef Header : HeadersToInclude()) {
-      Diag << Inserter.createMainFileIncludeInsertion(Header,
-                                                      IsAngledInclude());
+    for (StringRef Header : headersToInclude()) {
+      Diag << Inserter.createMainFileIncludeInsertion(Header);
     }
   }
 
-  virtual std::vector<StringRef> HeadersToInclude() const = 0;
-  virtual bool IsAngledInclude() const = 0;
+  virtual std::vector<StringRef> headersToInclude() const = 0;
 
   utils::IncludeInserter Inserter{utils::IncludeSorter::IS_Google};
 };
 
 class NonSystemHeaderInserterCheck : public IncludeInserterCheckBase {
 public:
   NonSystemHeaderInserterCheck(StringRef CheckName, ClangTidyContext *Context)
       : IncludeInserterCheckBase(CheckName, Context) {}
 
-  std::vector<StringRef> HeadersToInclude() const override {
+  std::vector<StringRef> headersToInclude() const override {
     return {"path/to/header.h"};
   }
-  bool IsAngledInclude() const override { return false; }
 };
 
 class EarlyInAlphabetHeaderInserterCheck : public IncludeInserterCheckBase {
 public:
   EarlyInAlphabetHeaderInserterCheck(StringRef CheckName, ClangTidyContext *Context)
       : IncludeInserterCheckBase(CheckName, Context) {}
 
-  std::vector<StringRef> HeadersToInclude() const override {
+  std::vector<StringRef> headersToInclude() const override {
     return {"a/header.h"};
   }
-  bool IsAngledInclude() const override { return false; }
 };
 
 class MultipleHeaderInserterCheck : public IncludeInserterCheckBase {
 public:
   MultipleHeaderInserterCheck(StringRef CheckName, ClangTidyContext *Context)
       : IncludeInserterCheckBase(CheckName, Context) {}
 
-  std::vector<StringRef> HeadersToInclude() const override {
+  std::vector<StringRef> headersToInclude() const override {
     return {"path/to/header.h", "path/to/header2.h", "path/to/header.h"};
   }
-  bool IsAngledInclude() const override { return false; }
 };
 
 class CSystemIncludeInserterCheck : public IncludeInserterCheckBase {
 public:
   CSystemIncludeInserterCheck(StringRef CheckName, ClangTidyContext *Context)
       : IncludeInserterCheckBase(CheckName, Context) {}
 
-  std::vector<StringRef> HeadersToInclude() const override {
-    return {"stdlib.h"};
+  std::vector<StringRef> headersToInclude() const override {
+    return {"<stdlib.h>"};
   }
-  bool IsAngledInclude() const override { return true; }
 };
 
 class CXXSystemIncludeInserterCheck : public IncludeInserterCheckBase {
 public:
   CXXSystemIncludeInserterCheck(StringRef CheckName, ClangTidyContext *Context)
       : IncludeInserterCheckBase(CheckName, Context) {}
 
-  std::vector<StringRef> HeadersToInclude() const override { return {"set"}; }
-  bool IsAngledInclude() const override { return true; }
+  std::vector<StringRef> headersToInclude() const override { return {"<set>"}; }
+};
+
+class InvalidIncludeInserterCheck : public IncludeInserterCheckBase {
+public:
+  InvalidIncludeInserterCheck(StringRef CheckName, ClangTidyContext *Context)
+      : IncludeInserterCheckBase(CheckName, Context) {}
+
+  std::vector<StringRef> headersToInclude() const override {
+    return {"a.h", "<stdlib.h", "cstdlib>", "b.h", "<c.h>", "<d>"};
+  }
 };
 
 template <typename Check>
 std::string runCheckOnCode(StringRef Code, StringRef Filename) {
   std::vector<ClangTidyError> Errors;
   return test::runCheckOnCode<Check>(Code, &Errors, Filename, None,
                                      ClangTidyOptions(),
                                      {// Main file include
void foo() {
   int a = 0;
 })";
 
   EXPECT_EQ(PostCode, runCheckOnCode<EarlyInAlphabetHeaderInserterCheck>(
                           PreCode, "workspace_folder/clang_tidy/tests/"
                                    "insert_includes_test_header.cc"));
 }
 
+TEST(IncludeInserterTest, InvalidHeaderName) {
+  const char *PreCode = R"(
+#include "clang_tidy/tests/insert_includes_test_header.h"
+
+#include <list>
+#include <map>
+
+#include "path/to/a/header.h"
+
+void foo() {
+  int a = 0;
+})";
+  const char *PostCode = R"(
+#include "clang_tidy/tests/insert_includes_test_header.h"
+
+#include <c.h>
+
+#include <d>
+#include <list>
+#include <map>
+
+#include "a.h"
+#include "b.h"
+#include "path/to/a/header.h"
+
+void foo() {
+  int a = 0;
+})";
+
+  EXPECT_EQ(PostCode, runCheckOnCode<InvalidIncludeInserterCheck>(
+                          PreCode, "clang_tidy/tests/"
+                                   "insert_includes_test_header.cc"));
+}
+
 } // anonymous namespace
 } // namespace tidy
 } // namespace clang
 
 #endif