[clang-tidy] In TransformerClangTidyCheck, support option IncludeStyle.

[ymandel]

Summary

The new option allows the user to specify which file naming convention is used
by the source code ('llvm' or 'google').

[gribozavr2]

LGTM, but I don't understand why this option is called "SourceNamingStyle". Existing ClangTidy checkers call it "IncludeStyle".

[ymandel]

In D79380#2019570, @gribozavr2 wrote:

LGTM, but I don't understand why this option is called "SourceNamingStyle". Existing ClangTidy checkers call it "IncludeStyle".

Thanks for the review!

Good question. I'd planned to call it "IncludeStyle" only to discover that it is misnamed. We could keep the name for consistency if it's in widespread use, but given that (IMO) it is quite misleading, I'd prefer to avoid it. At least to me, the name (and its description "A string specifying which include-style is used, llvm or google. Default is llvm.") implies that it controls how the includes are formatted. How they are grouped and ordered, for example. Yet, they don't. The IncludeSorter groups and orders the headers in exactly the same way no matter the style specification. It's only effect is to determine how a given file is related to header files, specifically how to determine that a header "corresponds" to a the file being examined -- that is, it is _this_ file's header rather than some arbitrary header. So, I figured that "SourceNamingStyle" captures this difference.

WDYT?

[gribozavr2]

It's only effect is to determine how a given file is related to header files, specifically how to determine that a header "corresponds" to a the file being examined -- that is, it is _this_ file's header rather than some arbitrary header.

... and the results of that algorithm directly affect how the includes are sorted. We could also conceivably get more styles in IncludeSorter that affect how headers are sorted (for example, related to case sensitivity).

Although I personally don't think this option is misnamed, I can see your point though. But given that the current name is used in many existing checkers, I don't think we should diverge in this patch. I think the right way would be to rename the option consistently everywhere, and to maintain a compatibility alias with a warning for at least one release.

[ymandel]

In D79380#2020135, @gribozavr2 wrote:

It's only effect is to determine how a given file is related to header files, specifically how to determine that a header "corresponds" to a the file being examined -- that is, it is _this_ file's header rather than some arbitrary header.

... and the results of that algorithm directly affect how the includes are sorted. We could also conceivably get more styles in IncludeSorter that affect how headers are sorted (for example, related to case sensitivity).

Sure, but the option itself isn't changing the style, even it influences the output. Agreed, though, that we could concievably leverage this to actually control details of the underlying algorithm, as you suggest. Although, I think the right answer is just to leave it all to clang-format.

Although I personally don't think this option is misnamed, I can see your point though. But given that the current name is used in many existing checkers, I don't think we should diverge in this patch. I think the right way would be to rename the option consistently everywhere, and to maintain a compatibility alias with a warning for at least one release.

Will do. I'm fine w/ maintaining consistency.

<rant>
I think a lot of my frustration stems from lost time trying to write tests against what I assumed was the behavior and then more time digginging into the code to actually determine the behavior (because it was only vaguely documented). Worse, IncludeSorter seems to have a bug for a corner case that probably only arisese naturally in tests -- if you add two headers to a file w/o any existing headers, it always adds them in the order recieved and separates them into different blocks. So, this experience left me with likely an overly strong aversion to the name. :)
</rant>

[ymandel]

renamed option to IncludeStyle, for consistency w/ existing checks.

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

 //===---------- TransformerClangTidyCheck.h - clang-tidy ------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H
 
 #include "../ClangTidy.h"
-#include "../utils/IncludeInserter.h"
+#include "IncludeInserter.h"
+#include "IncludeSorter.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Tooling/Transformer/Transformer.h"
 #include <deque>
 #include <vector>
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 // A base class for defining a ClangTidy check based on a `RewriteRule`.
 //
 // For example, given a rule `MyCheckAsRewriteRule`, one can define a tidy check
 // as follows:
 //
 // class MyCheck : public TransformerClangTidyCheck {
 //  public:
 //   MyCheck(StringRef Name, ClangTidyContext *Context)
 //       : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
 // };
+//
+// `TransformerClangTidyCheck` recognizes this clang-tidy option:
+//
+//  * SourceNamingStyle. A string specifying which file naming convention is
+//      used by the source code, 'llvm' or 'google'.  Default is 'llvm'.
 class TransformerClangTidyCheck : public ClangTidyCheck {
 public:
   // \p MakeRule generates the rewrite rule to be used by the check, based on
   // the given language and clang-tidy options. It can return \c None to handle
   // cases where the options disable the check.
   //
   // All cases in the rule generated by \p MakeRule must have a non-null \c
   // Explanation, even though \c Explanation is optional for RewriteRule in
public:
 
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) final;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
 
 private:
   Optional<transformer::RewriteRule> Rule;
-  std::unique_ptr<clang::tidy::utils::IncludeInserter> Inserter;
+  const IncludeSorter::IncludeStyle SourceNamingStyle;
+  std::unique_ptr<IncludeInserter> Inserter;
 };
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_CLANG_TIDY_CHECK_H

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

 // `ClangTidyCheck()` constructor to have been called. If we were to dispatch,
 // we would be accessing `getLangOpts` and `Options` before the underlying
 // `ClangTidyCheck` instance was properly initialized.
 TransformerClangTidyCheck::TransformerClangTidyCheck(
     std::function<Optional<RewriteRule>(const LangOptions &,
                                         const OptionsView &)>
         MakeRule,
     StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context), Rule(MakeRule(getLangOpts(), Options)) {
+    : ClangTidyCheck(Name, Context), Rule(MakeRule(getLangOpts(), Options)),
+      SourceNamingStyle(Options.getLocalOrGlobal("SourceNamingStyle",
+                                                 IncludeSorter::getMapping(),
+                                                 IncludeSorter::IS_LLVM)) {
   if (Rule)
     assert(llvm::all_of(Rule->Cases, hasExplanation) &&
            "clang-tidy checks must have an explanation by default;"
            " explicitly provide an empty explanation if none is desired");
 }
 
 TransformerClangTidyCheck::TransformerClangTidyCheck(RewriteRule R,
                                                      StringRef Name,
                                                      ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context), Rule(std::move(R)) {
+    : ClangTidyCheck(Name, Context), Rule(std::move(R)),
+      SourceNamingStyle(Options.getLocalOrGlobal("SourceNamingStyle",
+                                                 IncludeSorter::getMapping(),
+                                                 IncludeSorter::IS_LLVM)) {
   assert(llvm::all_of(Rule->Cases, hasExplanation) &&
          "clang-tidy checks must have an explanation by default;"
          " explicitly provide an empty explanation if none is desired");
 }
 
 void TransformerClangTidyCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   // Only allocate and register the IncludeInsert when some `Case` will add
   // includes.
   if (Rule && llvm::any_of(Rule->Cases, [](const RewriteRule::Case &C) {
         return !C.AddedIncludes.empty();
       })) {
-    Inserter = std::make_unique<IncludeInserter>(
-        SM, getLangOpts(), utils::IncludeSorter::IS_LLVM);
+    Inserter =
+        std::make_unique<IncludeInserter>(SM, getLangOpts(), SourceNamingStyle);
     PP->addPPCallbacks(Inserter->CreatePPCallbacks());
   }
 }
 
 void TransformerClangTidyCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   if (Rule)
     for (auto &Matcher : transformer::detail::buildMatchers(*Rule))

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

std::string Expected = R"cc(#include <clang/OtherLib.h>
     int f(int x);
     int h(int x) { return other(); }
   )cc";
 
   EXPECT_EQ(Expected,
             test::runCheckOnCode<IncludeCheck<IncludeFormat::Angled>>(Input));
 }
 
+class IncludeOrderCheck : public TransformerClangTidyCheck {
+  static RewriteRule rule() {
+    using namespace ::clang::ast_matchers;
+    RewriteRule Rule = transformer::makeRule(integerLiteral(), change(cat("5")),
+                                             cat("no message"));
+    addInclude(Rule, "bar.h", IncludeFormat::Quoted);
+    return Rule;
+  }
+
+public:
+  IncludeOrderCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(rule(), Name, Context) {}
+};
+
+TEST(TransformerClangTidyCheckTest, AddIncludeObeysSortStyleLocalOption) {
+  std::string Input = R"cc(#include "input.h"
+int h(int x) { return 3; })cc";
+
+  std::string TreatsAsLibraryHeader = R"cc(#include "input.h"
+
+#include "bar.h"
+int h(int x) { return 5; })cc";
+
+  std::string TreatsAsNormalHeader = R"cc(#include "bar.h"
+#include "input.h"
+int h(int x) { return 5; })cc";
+
+  ClangTidyOptions Options;
+  std::map<StringRef, StringRef> PathsToContent = {{"input.h", "\n"}};
+  Options.CheckOptions["test-check-0.SourceNamingStyle"] = "llvm";
+  EXPECT_EQ(TreatsAsLibraryHeader, test::runCheckOnCode<IncludeOrderCheck>(
+                                       Input, nullptr, "inputTest.cpp", None,
+                                       Options, PathsToContent));
+  EXPECT_EQ(TreatsAsNormalHeader, test::runCheckOnCode<IncludeOrderCheck>(
+                                      Input, nullptr, "input_test.cpp", None,
+                                      Options, PathsToContent));
+
+  Options.CheckOptions["test-check-0.SourceNamingStyle"] = "google";
+  EXPECT_EQ(TreatsAsNormalHeader,
+            test::runCheckOnCode<IncludeOrderCheck>(
+                Input, nullptr, "inputTest.cc", None, Options, PathsToContent));
+  EXPECT_EQ(TreatsAsLibraryHeader, test::runCheckOnCode<IncludeOrderCheck>(
+                                       Input, nullptr, "input_test.cc", None,
+                                       Options, PathsToContent));
+}
+
+TEST(TransformerClangTidyCheckTest, AddIncludeObeysSortStyleGlobalOption) {
+  std::string Input = R"cc(#include "input.h"
+int h(int x) { return 3; })cc";
+
+  std::string TreatsAsLibraryHeader = R"cc(#include "input.h"
+
+#include "bar.h"
+int h(int x) { return 5; })cc";
+
+  std::string TreatsAsNormalHeader = R"cc(#include "bar.h"
+#include "input.h"
+int h(int x) { return 5; })cc";
+
+  ClangTidyOptions Options;
+  std::map<StringRef, StringRef> PathsToContent = {{"input.h", "\n"}};
+  Options.CheckOptions["SourceNamingStyle"] = "llvm";
+  EXPECT_EQ(TreatsAsLibraryHeader, test::runCheckOnCode<IncludeOrderCheck>(
+                                       Input, nullptr, "inputTest.cpp", None,
+                                       Options, PathsToContent));
+  EXPECT_EQ(TreatsAsNormalHeader, test::runCheckOnCode<IncludeOrderCheck>(
+                                      Input, nullptr, "input_test.cpp", None,
+                                      Options, PathsToContent));
+
+  Options.CheckOptions["SourceNamingStyle"] = "google";
+  EXPECT_EQ(TreatsAsNormalHeader,
+            test::runCheckOnCode<IncludeOrderCheck>(
+                Input, nullptr, "inputTest.cc", None, Options, PathsToContent));
+  EXPECT_EQ(TreatsAsLibraryHeader, test::runCheckOnCode<IncludeOrderCheck>(
+                                       Input, nullptr, "input_test.cc", None,
+                                       Options, PathsToContent));
+}
+
 } // namespace
 } // namespace utils
 } // namespace tidy
 } // namespace clang