[clang-tidy] Reworked TransformerClangTidyCheck to simplify usage of Options

[njames93]

Summary

Changed TransformerClangTidyCheck to have a virtual method generate the rule. This has the advantages of making handling of any check options much simpler.

[gribozavr2]

@ymandel owns transformers.

[njames93]

• Renamed makeRule to buildRule to avoid ambiguity with tooling::makeRule

[ymandel]

Thanks for the suggestioned change. However, I think we can do somewhat simpler. What do you think of just providing a setter on the super class

void setRule(Optional<transformer::RewriteRule> Rule) { this->Rule = std::move(Rule); }

Subclasses will call setRule in their constructor body . Or not. The code in the superclass doesn't change either way, since it already needed to check the optional. There's no initialization logic, no virtual functions, no need to delete the existing constructors (though we should deprecate them in favor of this simpler mechanism).

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

44–58

This should be deprecated and then deleted after a delay (1-2 weeks) giving clients time to transition rather than breaking them right off.

62

Why delete this? It is the most commonly used constructor and cleaner than the OOP goop of the virtual method.

[njames93]

• Add back old constructors, one being deprecated

[njames93]

There are a few reasons for using the virtual method:

• It keeps everything contained in the one class.
• In the case where options are needed it avoid extra work in the derived class.
• It's consistent with how all base class checks handle the specifics of derived checks.

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

62

I'll add that back in, but I feel this constructor is no different in most cases to just placing the rule builder in the virtual method

[ymandel]

Thanks for the changes!

In D80697#2062517, @njames93 wrote:

There are a few reasons for using the virtual method:

I think I'm missing something (or I didn't explain my intention well):

• It keeps everything contained in the one class.

Everything is still in one class -- the constructor of the derived class just calls setRule.

• In the case where options are needed it avoid extra work in the derived class.

Why wouldn't this also avoid the extra work? The constructor of the derived class initializes the local variables and then in its body calls setRule. For example, in the class you modified above, the constructor barely changes. Just the body:

... {
  setRule(buildRule());
}

where buildRule is now just a static function are non-virtual private function and doesn't need to return optional, because the logic deciding about constructed it can just be used instead to guard the call to setRule as need be. Given the virtual method isLanguageVersionSupported, there usually won't be any such logic.

• It's consistent with how all base class checks handle the specifics of derived checks.

Sorry, I'm not sure what you mean -- can you expand on this?

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

 // 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(Name, Context) {}
+//
+//   Optional<RewriteRule> makeRule() const override {
+//     return MyCheckAsRewriteRule;
+//   }
 // };
 //
 // `TransformerClangTidyCheck` recognizes this clang-tidy option:
 //
 //  * IncludeStyle. A string specifying which file naming convention is used by
 //      the source code, 'llvm' or 'google'.  Default is 'llvm'. The naming
 //      convention influences how canonical headers are distinguished from other
 //      includes.
 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
-  // general. Because the primary purpose of clang-tidy checks is to provide
-  // users with diagnostics, we assume that a missing explanation is a bug.  If
-  // no explanation is desired, indicate that explicitly (for example, by
-  // passing `text("no explanation")` to `makeRule` as the `Explanation`
-  // argument).
-  TransformerClangTidyCheck(std::function<Optional<transformer::RewriteRule>(
-                                const LangOptions &, const OptionsView &)>
-                                MakeRule,
-                            StringRef Name, ClangTidyContext *Context);
-
-  // Convenience overload of the constructor when the rule doesn't depend on any
-  // of the language or clang-tidy options.
-  TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
-                            ClangTidyContext *Context);
+  TransformerClangTidyCheck(StringRef Name, ClangTidyContext *Context);
 
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
-                           Preprocessor *ModuleExpanderPP) override;
+                           Preprocessor *ModuleExpanderPP) final;
   void registerMatchers(ast_matchers::MatchFinder *Finder) final;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
 
   /// Derived classes that override this function should call this method from
   /// the overridden method.
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
+  /// This generates the rewrite rule to be used by the check.It can return \c
+  /// None to handle cases where the options disable the check.
+  ///
+  /// All cases in the rule must have a non-null \c Explanation, even though \c
+  /// Explanation is optional for RewriteRule in general. Because the primary
+  /// purpose of clang-tidy checks is to provide users with diagnostics, we
+  /// assume that a missing explanation is a bug. If no explanation is desired,
+  /// indicate that explicitly (for example, by passing `text("no explanation")`
+  /// to `makeRule` as the `Explanation` argument).
+  virtual Optional<transformer::RewriteRule> makeRule() const = 0;
+
 private:
+  void instantiateRule();
   Optional<transformer::RewriteRule> Rule;
+  bool HasMadeRule = false;
   const IncludeSorter::IncludeStyle IncludeStyle;
   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

 using transformer::RewriteRule;
 
 #ifndef NDEBUG
 static bool hasExplanation(const RewriteRule::Case &C) {
   return C.Explanation != nullptr;
 }
 #endif
 
-// This constructor cannot dispatch to the simpler one (below), because, in
-// order to get meaningful results from `getLangOpts` and `Options`, we need the
-// `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)),
-      IncludeStyle(Options.getLocalOrGlobal("IncludeStyle",
-                                            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,
+TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
                                                      ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context), Rule(std::move(R)),
+    : ClangTidyCheck(Name, Context),
       IncludeStyle(Options.getLocalOrGlobal("IncludeStyle",
                                             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");
-}
+                                            IncludeSorter::IS_LLVM)) {}
 
 void TransformerClangTidyCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  instantiateRule();
   // 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(), IncludeStyle);
     PP->addPPCallbacks(Inserter->CreatePPCallbacks());
   }
 }
 
 void TransformerClangTidyCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
+  instantiateRule();
   if (Rule)
     for (auto &Matcher : transformer::detail::buildMatchers(*Rule))
       Finder->addDynamicMatcher(Matcher, this);
 }
 
 void TransformerClangTidyCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())

 }
 
 void TransformerClangTidyCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", IncludeStyle,
                 IncludeSorter::getMapping());
 }
 
+void TransformerClangTidyCheck::instantiateRule() {
+  if (!HasMadeRule) {
+    HasMadeRule = true;
+    if ((Rule = makeRule()))
+      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");
+  }
+}
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - The 'fuchsia-restrict-system-headers' check was renamed to :doc:`portability-restrict-system-includes
   <clang-tidy/checks/portability-restrict-system-includes>`
 
 Other improvements
 ^^^^^^^^^^^^^^^^^^
 
 - For 'run-clang-tidy.py' add option to use alpha checkers from clang-analyzer.
 
+- ``TransformerClangTidyChecks`` have been reworked to have enable simpler
+  handling of options. This change will require a small tweak to any check that
+  derives from the ``TransformerClangTidyChecks`` base class.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

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

 //===---- TransformerClangTidyCheckTest.cpp - 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "../clang-tidy/utils/TransformerClangTidyCheck.h"
 #include "ClangTidyTest.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Transformer/RangeSelector.h"
+#include "clang/Tooling/Transformer/RewriteRule.h"
 #include "clang/Tooling/Transformer/Stencil.h"
 #include "clang/Tooling/Transformer/Transformer.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace {
 using namespace ::clang::ast_matchers;
 
 using transformer::cat;
 using transformer::change;
 using transformer::IncludeFormat;
 using transformer::node;
 using transformer::RewriteRule;
 using transformer::statement;
 
 // Invert the code of an if-statement, while maintaining its semantics.
-RewriteRule invertIf() {
+class IfInverterCheck : public TransformerClangTidyCheck {
+public:
+  IfInverterCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(Name, Context) {}
+
+  Optional<RewriteRule> makeRule() const override {
     StringRef C = "C", T = "T", E = "E";
     RewriteRule Rule = tooling::makeRule(
         ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
                hasElse(stmt().bind(E))),
         change(statement(std::string(RewriteRule::RootID)),
                cat("if(!(", node(std::string(C)), ")) ",
                    statement(std::string(E)), " else ",
                    statement(std::string(T)))),
         cat("negate condition and reverse `then` and `else` branches"));
     return Rule;
   }
-
-class IfInverterCheck : public TransformerClangTidyCheck {
-public:
-  IfInverterCheck(StringRef Name, ClangTidyContext *Context)
-      : TransformerClangTidyCheck(invertIf(), Name, Context) {}
 };
 
 // Basic test of using a rewrite rule as a ClangTidy.
 TEST(TransformerClangTidyCheckTest, Basic) {
   const std::string Input = R"cc(
     void log(const char* msg);
     void foo() {
       if (10 > 1.0)
const std::string Expected = R"(
     }
   )";
   EXPECT_EQ(Expected, test::runCheckOnCode<IfInverterCheck>(Input));
 }
 
 class IntLitCheck : public TransformerClangTidyCheck {
 public:
   IntLitCheck(StringRef Name, ClangTidyContext *Context)
-      : TransformerClangTidyCheck(tooling::makeRule(integerLiteral(),
-                                                    change(cat("LIT")),
-                                                    cat("no message")),
-                                  Name, Context) {}
+      : TransformerClangTidyCheck(Name, Context) {}
+
+  Optional<RewriteRule> makeRule() const override {
+    return tooling::makeRule(integerLiteral(), change(cat("LIT")),
+                             cat("no message"));
+  }
 };
 
 // Tests that two changes in a single macro expansion do not lead to conflicts
 // in applying the changes.
 TEST(TransformerClangTidyCheckTest, TwoChangesInOneMacroExpansion) {
   const std::string Input = R"cc(
 #define PLUS(a,b) (a) + (b)
     int f() { return PLUS(3, 4); }
   )cc";
   const std::string Expected = R"cc(
 #define PLUS(a,b) (a) + (b)
     int f() { return PLUS(LIT, LIT); }
   )cc";
 
   EXPECT_EQ(Expected, test::runCheckOnCode<IntLitCheck>(Input));
 }
 
 class BinOpCheck : public TransformerClangTidyCheck {
 public:
   BinOpCheck(StringRef Name, ClangTidyContext *Context)
-      : TransformerClangTidyCheck(
-            tooling::makeRule(
-                binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
-                change(node("r"), cat("RIGHT")), cat("no message")),
-            Name, Context) {}
+      : TransformerClangTidyCheck(Name, Context) {}
+
+  Optional<RewriteRule> makeRule() const override {
+    return tooling::makeRule(
+        change(node("r"), cat("RIGHT")), cat("no message"));
+  }
 };
 
 // Tests case where the rule's match spans both source from the macro and its
 // argument, while the change spans only the argument AND there are two such
 // matches. We verify that both replacements succeed.
 TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) {
   const std::string Input = R"cc(
 #define M(a,b) (1 + a) * (1 + b)
     int f() { return M(3, 4); }
   )cc";
   const std::string Expected = R"cc(
 #define M(a,b) (1 + a) * (1 + b)
     int f() { return M(RIGHT, RIGHT); }
   )cc";
 
   EXPECT_EQ(Expected, test::runCheckOnCode<BinOpCheck>(Input));
 }
 
 // A trivial rewrite-rule generator that requires Objective-C code.
-Optional<RewriteRule> needsObjC(const LangOptions &LangOpts,
-                                const ClangTidyCheck::OptionsView &Options) {
-  if (!LangOpts.ObjC)
-    return None;
-  return tooling::makeRule(clang::ast_matchers::functionDecl(),
-                           change(cat("void changed() {}")), cat("no message"));
-}
-
 class NeedsObjCCheck : public TransformerClangTidyCheck {
 public:
   NeedsObjCCheck(StringRef Name, ClangTidyContext *Context)
-      : TransformerClangTidyCheck(needsObjC, Name, Context) {}
+      : TransformerClangTidyCheck(Name, Context) {}
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.ObjC;
+  }
+
+  Optional<RewriteRule> makeRule() const override {
+    return tooling::makeRule(clang::ast_matchers::functionDecl(),
+                             change(cat("void changed() {}")),
+                             cat("no message"));
+  }
 };
 
 // Verify that the check only rewrites the code when the input is Objective-C.
 TEST(TransformerClangTidyCheckTest, DisableByLang) {
   const std::string Input = "void log() {}";
   EXPECT_EQ(Input,
             test::runCheckOnCode<NeedsObjCCheck>(Input, nullptr, "input.cc"));
 
   EXPECT_EQ("void changed() {}",
             test::runCheckOnCode<NeedsObjCCheck>(Input, nullptr, "input.mm"));
 }
 
 // A trivial rewrite rule generator that checks config options.
-Optional<RewriteRule> noSkip(const LangOptions &LangOpts,
-                             const ClangTidyCheck::OptionsView &Options) {
-  if (Options.get("Skip", "false") == "true")
+class ConfigurableCheck : public TransformerClangTidyCheck {
+public:
+  ConfigurableCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(Name, Context),
+        Skip(Options.get("Skip", false)) {}
+
+  Optional<RewriteRule> makeRule() const override {
+    if (Skip)
       return None;
     return tooling::makeRule(clang::ast_matchers::functionDecl(),
                              change(cat("void nothing()")), cat("no message"));
   }
 
-class ConfigurableCheck : public TransformerClangTidyCheck {
-public:
-  ConfigurableCheck(StringRef Name, ClangTidyContext *Context)
-      : TransformerClangTidyCheck(noSkip, Name, Context) {}
+private:
+  const bool Skip;
 };
 
 // Tests operation with config option "Skip" set to true and false.
 TEST(TransformerClangTidyCheckTest, DisableByConfig) {
   const std::string Input = "void log(int);";
   const std::string Expected = "void nothing();";
   ClangTidyOptions Options;
 
   Options.CheckOptions["test-check-0.Skip"] = "true";
   EXPECT_EQ(Input, test::runCheckOnCode<ConfigurableCheck>(
                        Input, nullptr, "input.cc", None, Options));
 
   Options.CheckOptions["test-check-0.Skip"] = "false";
   EXPECT_EQ(Expected, test::runCheckOnCode<ConfigurableCheck>(
                           Input, nullptr, "input.cc", None, Options));
 }
 
-RewriteRule replaceCall(IncludeFormat Format) {
+template <IncludeFormat Format>
+class IncludeCheck : public TransformerClangTidyCheck {
+public:
+  IncludeCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(Name, Context) {}
+
+  Optional<RewriteRule> makeRule() const override {
     using namespace ::clang::ast_matchers;
     RewriteRule Rule =
         tooling::makeRule(callExpr(callee(functionDecl(hasName("f")))),
                           change(cat("other()")), cat("no message"));
     addInclude(Rule, "clang/OtherLib.h", Format);
     return Rule;
   }
-
-template <IncludeFormat Format>
-class IncludeCheck : public TransformerClangTidyCheck {
-public:
-  IncludeCheck(StringRef Name, ClangTidyContext *Context)
-      : TransformerClangTidyCheck(replaceCall(Format), Name, Context) {}
 };
 
 TEST(TransformerClangTidyCheckTest, AddIncludeQuoted) {
 
   std::string Input = R"cc(
     int f(int x);
     int h(int x) { return f(x); }
   )cc";
std::string Expected = R"cc(#include <clang/OtherLib.h>
     int h(int x) { return other(); }
   )cc";
 
   EXPECT_EQ(Expected,
             test::runCheckOnCode<IncludeCheck<IncludeFormat::Angled>>(Input));
 }
 
 class IncludeOrderCheck : public TransformerClangTidyCheck {
-  static RewriteRule rule() {
+public:
+  IncludeOrderCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(Name, Context) {}
+
+  Optional<RewriteRule> makeRule() const override {
     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"