[clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

[ymandel]

Summary

Tidy check behavior often depends on language and/or clang-tidy options. This revision allows a user of TranformerClangTidyCheck to pass rule _generator_ in place of a rule, where the generator takes both the language and clang-tidy options. Additionally, the generator returns an Optional to allow for the case where the check is deemed irrelevant/disable based on those options.

[gribozavr]

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

52

Can we dispatch to the other constructor?

59

Keep the definition order of constructors consistent with declaration order?

[ymandel]

Thanks for the review!

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

52

I think not, but please correct me if I'm wrong (and clearly I should add a comment explaining this): in order to get meaningful results from getLangOpts and Options, we need the ClangTidyCheck() constructor to have been called. If we were to dispatch, it would look like:

: TransformerClangTidyCheck(MakeRule(getLangOpts(), Options), Name, Context) {}

which, if I understand correctly, means that MakeRule will access that data _before_ the check object is properly initialized.

That said, I can factor out the assertion into a method to avoid the redundancy. WDYT?

[gribozavr]

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

52

You're right. I don't think it is necessary to factor out the assertion -- up to you.

[ymandel]

Added tests.

[ymandel]

Adjust comments in test.

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

52

Decided not to factor.

[gribozavr]

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

92

Would adding -std=c99 to ExtraArgs, and setting Filename to input.c work?

[ymandel]

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

92

No, nothing seems to work. I can't find a way to ever set bits in LangOptions. I'm rather confused why that's the case...

[ymandel]

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

92

I found the answer: getLangOpts() is not guaranteed to work at check-creation time. This makes sense, since a single check may be run against many different files. I'd say its a flaw in the API though since it seems like a feature of the check, when its really a feature of the file. We should update the API to warn about this and possibly improve it to avoid the confusion.

I'll update the code to save the std::function and invoke at registerMatchers() time instead.

[ymandel]

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

92

Never mind my previous point -- this only holds for the particular way that runCheckOnCode is implemented (which is arguably a bug). For the real clang tidy, LangOpts is a feature of the tool invocation and is therefore constant across the lifetime of the check.

I will send a change to runCheckOnCode that fixes this -- that is, ensures LangOpts is set *before* the Check is constructed, and then this will work correctly.

[ymandel]

Rebase onto fix to runCheckOnCode.

[gribozavr]

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

69

"Objective-C" (with a dash)

[ymandel]

resp. to comments

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

 //
 // class MyCheck : public TransformerClangTidyCheck {
 //  public:
 //   MyCheck(StringRef Name, ClangTidyContext *Context)
 //       : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
 // };
 class TransformerClangTidyCheck : public ClangTidyCheck {
 public:
-  // All cases in \p R 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).
+  // \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<tooling::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(tooling::RewriteRule R, StringRef Name,
                             ClangTidyContext *Context);
+
   void registerMatchers(ast_matchers::MatchFinder *Finder) final;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
 
 private:
-  tooling::RewriteRule Rule;
+  Optional<tooling::RewriteRule> Rule;
 };
 
 } // 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

 //===---------- TransformerClangTidyCheck.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 "TransformerClangTidyCheck.h"
 #include "llvm/ADT/STLExtras.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 using tooling::RewriteRule;
 
+static bool hasExplanation(const RewriteRule::Case &C) {
+  return C.Explanation != nullptr;
+}
+
+// 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)) {
+  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)) {
-  assert(llvm::all_of(Rule.Cases, [](const RewriteRule::Case &C) {
-                       return C.Explanation != nullptr;
-                     }) &&
+  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::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
-  Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
+  if (Rule)
+    Finder->addDynamicMatcher(tooling::detail::buildMatcher(*Rule), this);
 }
 
 void TransformerClangTidyCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
     return;
 
   // Verify the existence and validity of the AST node that roots this rule.
   const ast_matchers::BoundNodes::IDToNodeMap &NodesMap = Result.Nodes.getMap();
   auto Root = NodesMap.find(RewriteRule::RootID);
   assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
   SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
       Root->second.getSourceRange().getBegin());
   assert(RootLoc.isValid() && "Invalid location for Root node of match.");
 
-  RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, Rule);
+  assert(Rule && "check() should not fire if Rule is None");
+  RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, *Rule);
   Expected<SmallVector<tooling::detail::Transformation, 1>> Transformations =
       tooling::detail::translateEdits(Result, Case.Edits);
   if (!Transformations) {
     llvm::errs() << "Rewrite failed: "
                  << llvm::toString(Transformations.takeError()) << "\n";
     return;
   }
 

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

 #include "clang/Tooling/Refactoring/Stencil.h"
 #include "clang/Tooling/Refactoring/Transformer.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace {
+using tooling::change;
 using tooling::RewriteRule;
+using tooling::text;
+using tooling::stencil::cat;
 
 // Invert the code of an if-statement, while maintaining its semantics.
 RewriteRule invertIf() {
   using namespace ::clang::ast_matchers;
-  using tooling::change;
   using tooling::node;
   using tooling::statement;
-  using tooling::text;
-  using tooling::stencil::cat;
 
   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(RewriteRule::RootID),
           cat("if(!(", node(C), ")) ", statement(E), " else ", statement(T))),
TEST(TransformerClangTidyCheckTest, Basic) {
   const std::string Expected = R"(
     void log(const char* msg);
     void foo() {
       if(!(10 > 1.0)) log("ok"); else log("oh no!");
     }
   )";
   EXPECT_EQ(Expected, test::runCheckOnCode<IfInverterCheck>(Input));
 }
+
+// A trivial rewrite rule generator that requires C99 code to operate.
+Optional<RewriteRule> needsC99(const LangOptions &LangOpts,
+                               const ClangTidyCheck::OptionsView &Options) {
+  if (!LangOpts.C99)
+    return None;
+  return tooling::makeRule(clang::ast_matchers::functionDecl(),
+                           change(cat("void nothing()")), text("no message"));
+}
+
+class NeedsC99Check : public TransformerClangTidyCheck {
+public:
+  NeedsC99Check(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(needsC99, Name, Context) {}
+};
+
+// Pass a C++ source file and expect no change, since the check requires `C99`
+// support to fire.
+TEST(TransformerClangTidyCheckTest, DisableByLang) {
+  // FIXME: We should be testing both the positive and negative case. However,
+  // as far as I can tell, all of the LangOpts are always set to false, so I
+  // have been unable to test the positive case (that is, where the
+  // `LangOpts.C99` is true).
+  const std::string Input = "void log(int);";
+  EXPECT_EQ(Input, test::runCheckOnCode<NeedsC99Check>(Input));
+}
+
+// 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")
+    return None;
+  return tooling::makeRule(clang::ast_matchers::functionDecl(),
+                           change(cat("void nothing()")), text("no message"));
+}
+
+class ConfigurableCheck : public TransformerClangTidyCheck {
+public:
+  ConfigurableCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(noSkip, Name, Context) {}
+};
+
+// 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));
+}
+
 } // namespace
 } // namespace utils
 } // namespace tidy
 } // namespace clang