[clang-tidy] Add support writing a check as a Transformer rewrite rule.

[ymandel]

Summary

This revision introduces an adaptor from Transformer's rewrite rules
(clang::tooling::RewriteRule) to ClangTidyCheck. For example, given a
RewriteRule MyCheckAsRewriteRule, it lets one define a tidy check as follows:

class MyTidyCheck : public TransformerClangTidyCheck {
 public:
  MyTidyCheck(StringRef Name, ClangTidyContext *Context)
      : TransformerClangTidyCheck(MyCheckAsRewriteRule, Name, Context) {}
};

[Eugene.Zelenko]

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

23

Please don't use auto when return type is not spelled at same statement or iterator. Same for other places.

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

11

Unnecessary empty line.

[ymandel]

Reduced use of auto.

[ymandel]

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

23

I left only the iterator's auto, expanded the rest. Let me know if I've converted too many. I wonder about this one here, for example, because the type of the map is essentially an internal detail of BoundNodes.

[Eugene.Zelenko]

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

52

It's iterator, so auto could be used instead of type.

[ymandel]

reinstated an auto

[ymandel]

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

52

great, i wasn't sure whether this counted as such.

[JonasToth]

I like the new framework!

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

44

these if-else are single statement, so the braces can be ellided.

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

39

If we allow to pass in a callable, that returns a RewriteRulewe would have a more flexible framework in clang-tidy.
Going with this for now is ok in my opinion as the only good usecase that comes to my mind would be statistics.

But If some transformation needs to remember what has been done before only a RewriteRule as state is not sufficient and stateful actions would require global state (MEH!).

[ymandel]

responded to comments and shortened (unnecessarily long) variable name.

[ymandel]

In D61386#1487735, @JonasToth wrote:

I like the new framework!

Great!

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

39

If we allow to pass in a callable, that returns a RewriteRule we would have a more flexible framework in clang-tidy.
Going with this for now is ok in my opinion as the only good usecase that comes to my mind would be statistics.

I'm fine with something more general, but I don't think I follow the particulars you have in mind. Even w/ a callable, wouldn't that only be invoked *once* by clang-tidy, when it creates the class? What kind of statistics do you have in mind?

But If some transformation needs to remember what has been done before only a RewriteRule as state is not sufficient and stateful actions would require global state (MEH!).

Indeed. The current design does preclude a (non gross) way of collecting state over multiple invocations. Since this is rare, I think that's fine for now. But, our plan is to add support for a Translation Unit level API that supports things like collecting state, adding and removing header includes, inserting using decls and any other TU level operations.

[ymandel]

Gentle ping...

[ilya-biryukov]

@aaron.ballman, would be a good reviewer for this? I'm happy to look at the transformer bits, but I'm definitely the wrong one to asses it from the clang-tidy perspective.

[ymandel]

Updated to incorporate changes to Transformer.

[ymandel]

Ping. Is anyone willing to be the reviewer for this revision?

Thanks!

[gribozavr]

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

32

I'd prefer a name like "TransformerClangTidyCheck", it will only appear in a few places, but is much more clear.

38

I'd suggest to add comments telling users to not override these methods in subclasses.

[ymandel]

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

32

will do, but should I also rename the files correspondingly?

38

sure. would marking them as final make sense, then?

[gribozavr]

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

32

Yes please.

38

Right, that's the perfect tool for the job.

[ymandel]

Renamed TransformerTidy to TransformerClangTidyCheck; classes and files.

[ymandel]

Thanks for the review.

[MyDeveloperDay]

Just a passing comment, Is this:

a) a different type of check that would be used for a different type of check than previous clang-tidy checks
b) or is this a replacement on the previous style?

Is there any documentation about the benefits of this method vs the old method? or of why we would potentially use one and not the other?

Is there a change needed to add_new_check.py to have it generate this new style? (its ok if the answer is yes in a future revision)

Do you foresee people using this mechanism going foward?

I do like the brevity of it and I do like the GTEST unit tests, (I struggled more with the lit tests than anything else, mainly for python on cygwin reasons)

It looks good, even if it looks like I need to go learn how tooling::stencil works... a little bit of documentation might not go a miss on how to get started with it...

[ymandel]

In D61386#1510690, @MyDeveloperDay wrote:

Just a passing comment, Is this:

a) a different type of check that would be used for a different type of check than previous clang-tidy checks
b) or is this a replacement on the previous style?

This is a replacement for a certain class of checks. Mostly for simple checks that are primarily pattern + reconstruction, without complex logic (at least for now). The long-term goal is to provide a concise alternative for the large majority of checks.

Is there any documentation about the benefits of this method vs the old method? or of why we would potentially use one and not the other?

There's a doc describing the framework here
https://docs.google.com/document/d/1ppw0RhjwsrbBcHYhI85pe6ISDbA6r5d00ot3N8cQWeQ/edit?usp=sharing
and some corresponding discussion on the cfe-dev mailing list (and in the doc comments).

Is there a change needed to add_new_check.py to have it generate this new style? (its ok if the answer is yes in a future revision)

Do you foresee people using this mechanism going forward?

Not yet, but once it is more mature, yes. I would hope that it will be the more common approach. At that point, we can adjust the scripts accordingly.

I do like the brevity of it and I do like the GTEST unit tests, (I struggled more with the lit tests than anything else, mainly for python on cygwin reasons)

It looks good, even if it looks like I need to go learn how tooling::stencil works... a little bit of documentation might not go a miss on how to get started with it...

Thanks. Yes, it very much requires that you familiarize yourself with Stencil and RewriteRule (Tooling/Refactoring/Stencil.h and Transformer.h). I'm not sure what more to add that is specific to clang tidy. The intent is that, if your check can be expressed as a RewriteRule then there's really nothing you need to know about clang-tidy beyond what's in the header file -- you just drop the rule into TransformerClangTidyCheck. If you have questions after you see the Tooling documentation, please let me know and I'm happy to expand the comments here. Both Stencil and Transformer are quite and still under active development. Once they've stabilized a bit, I also plan to send out a more general announcement about this to the cfe-dev list.

[ilya-biryukov]

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

46 ↗

(On Diff #200536)

The users will see this for every case without explanation, right?
I'd expect the rules without explanation to be somewhat common, maybe don't show any message at all in that case?

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

1 ↗

(On Diff #200536)

NIT: maybe use TransformerCheck for brevity? I'm not a clang-tidy maintainer, though, so not sure whether that aligns with the rest of the code.

[ymandel]

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

46 ↗

(On Diff #200536)

There's no option to call diag() without a message. We could pass an empty string , but that may be confusing given the way the message is concatenated here:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204

So, no matter what, there will be some message to go w/ the diagnostic. I figure that being explicit about the lack of explanation is better than an empty string, but don't feel strongly about this.

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

1 ↗

(On Diff #200536)

It was TransformerTidy and Dmitri suggested I be explicit. I think TransformerCheck is better than TransformerTidy, but I also see the argument in spelling it out. WDYT?

[ilya-biryukov]

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

46 ↗

(On Diff #200536)

Ah, so all the options are bad. I can see why you had this design in transformers in the first place.
I wonder if we should check the explanations are always set for rewrite rules inside the clang-tidy transformation?

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

1 ↗

(On Diff #200536)

Let's stick with Dmitri's suggestion. He has much more experience with LLVM than I do :-)

[ymandel]

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

46 ↗

(On Diff #200536)

Ah, so all the options are bad. I can see why you had this design in transformers in the first place.

Heh. indeed.

I wonder if we should check the explanations are always set for rewrite rules inside the clang-tidy transformation?> Quoted Text

I would have thought so, but AFAIK, most folks who write one-off transformations use clang-tidy, rather than writing a standalone tool. They just ignore the diagnostics, i gather. Transformer may shift that somewhat if we improve the experience of writing a (throwaway) standalone tool, but for the time being I think we can't assume that.

[ymandel]

adjusted as required after syncing latest changes to Stencil and Transformer.

[ymandel]

Replace previous diff which had the wrong base.

[ilya-biryukov]

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

46 ↗

(On Diff #200536)

We should focus on minimizing maintenance cost for long-term fixes rather than one-off transformations. The cost of passing an empty string to a required explanation field for one-off transformations is rather small and falls into the hands of a person writing the check, the cost of constantly finding and fixing the long-lived upstream checks that don't have explanation (leading to bad user experience) is high and will probably fall into the hands of clang-tidy maintainers in addition to people writing the checks.

FWIW, having a new API of top of plain transformers should be better than clang-tidy for those writing one-off transformations in the long run. I don't think clang-tidy was ever designed to cover to cover those use-cases, even though today it seems to be the fastest way to do those.

[ymandel]

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

46 ↗

(On Diff #200536)

Agreed on all points. I'll send a followup that enforces that constraint.

clang-tools-extra/clang-tidy/utils/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyUtils
   ASTUtils.cpp
   DeclRefExprUtils.cpp
   ExceptionAnalyzer.cpp
   ExprSequence.cpp
   FixItHintUtils.cpp
   HeaderFileExtensionsUtils.cpp
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
   LexerUtils.cpp
   NamespaceAliaser.cpp
   OptionsUtils.cpp
+  TransformerTidy.cpp
   TypeTraits.cpp
   UsingInserter.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
+  clangToolingRefactor
   )

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

+//===---------- TransformerTidy.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_TIDY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H
+
+#include "../ClangTidy.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include <deque>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+// A base class for defining a ClangTidy check based on a rewrite rule.
+//
+// For example, given a RewriteRule `MyCheckAsRewriteRule`, one can define your
+// tidy check as follows:
+//
+// class MyTidyCheck : public TransformerTidy {
+//  public:
+//   MyTidyCheck(StringRef Name, ClangTidyContext *Context)
+//       : TransformerTidy(MyCheckAsRewriteRule, Name, Context) {}
+// };
+class TransformerTidy : public ClangTidyCheck {
+public:
+  TransformerTidy(tooling::RewriteRule R, StringRef Name,
+                  ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  tooling::RewriteRule Rule;
+};
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_TRANSFORMER_TIDY_H

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

+//===---------- TransformerTidy.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 "TransformerTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+void TransformerTidy::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  Finder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
+}
+
+void TransformerTidy::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(tooling::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.");
+
+  tooling::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;
+  }
+
+  // No rewrite applied, but no error encountered either.
+  if (Transformations->empty())
+    return;
+
+  StringRef Message = "no explanation";
+  if (Case.Explanation) {
+    if (Expected<std::string> E = Case.Explanation(Result))
+      Message = *E;
+    else
+      llvm::errs() << "Error in explanation: " << llvm::toString(E.takeError())
+                   << "\n";
+  }
+  DiagnosticBuilder Diag = diag(RootLoc, Message);
+  for (const auto &T : *Transformations) {
+    Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
+  }
+}
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang

clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

add_extra_unittest(ClangTidyTests
   IncludeInserterTest.cpp
   GoogleModuleTest.cpp
   LLVMModuleTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp
+  TransformerTidyTest.cpp
   )
 
 target_link_libraries(ClangTidyTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangLex
   clangSerialization
   clangTidy
   clangTidyAndroidModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyObjCModule
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )

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

+//===---- TransformerTidyTest.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/TransformerTidy.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+namespace {
+// Invert the code of an if-statement, while maintaining its semantics.
+tooling::RewriteRule invertIf() {
+  using namespace ::clang::ast_matchers;
+  using tooling::change;
+  using tooling::stencil::cat;
+  using tooling::stencil::node;
+  using tooling::stencil::sNode;
+
+  StringRef C = "C", T = "T", E = "E";
+  return tooling::makeRule(
+      ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
+             hasElse(stmt().bind(E))),
+      change<Stmt>(cat("if(!(", node(C), ")) ", sNode(E), " else ", sNode(T))));
+}
+
+class IfInverterTidy : public TransformerTidy {
+public:
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+      : TransformerTidy(invertIf(), Name, Context) {}
+};
+
+// Basic test of using a rewrite rule as a ClangTidy.
+TEST(TransformerTidyTest, Basic) {
+  const std::string Input = R"cc(
+    void log(const char* msg);
+    void foo() {
+      if (10 > 1.0)
+        log("oh no!");
+      else
+        log("ok");
+    }
+  )cc";
+  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<IfInverterTidy>(Input));
+}
+} // namespace
+} // namespace utils
+} // namespace tidy
+} // namespace clang