[clang-tidy] In TransformerClangTidyCheck, require Explanation field.

[ymandel]

Summary

In general, the Explanation field is optional in RewriteRule cases. But,
because the primary purpose of clang-tidy checks is to provide users with
diagnostics, we assume that a missing explanation is a bug. This change adds an
assertion that checks all cases for an explanation, and updates the code to rely
on that assertion correspondingly.

[ilya-biryukov]

LGTM, thanks!

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

38

NIT: this might be the first place people look at when writing the check, so we might want to provide a real explanation here.
Something like negate condition and revert then and else branches

[ymandel]

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

38

will do. But, realized this is a bit ugly so sent https://reviews.llvm.org/D62390 to add support for the explanation to makeRule.

[ymandel]

updated to use new version of makeRule; changed explanation text in test

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

 // 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) {}
 // };
+//
 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, `MyRule.Case[0].Explanation =
+  // text("no explanation")`).
   TransformerClangTidyCheck(tooling::RewriteRule R, StringRef Name,
-                            ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context), Rule(std::move(R)) {}
-
+                            ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) final;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
 
 private:
   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"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 using tooling::RewriteRule;
 
+TransformerClangTidyCheck::TransformerClangTidyCheck(tooling::RewriteRule R,
+                                                     StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), Rule(std::move(R)) {
+  for (const auto &Case : Rule.Cases) {
+    assert(Case.Explanation != nullptr &&
+           "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);
 }
 
 void TransformerClangTidyCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
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";
+  Expected<std::string> Explanation = Case.Explanation(Result);
+  if (!Explanation) {
+    llvm::errs() << "Error in explanation: "
+                 << llvm::toString(Explanation.takeError()) << "\n";
+    return;
   }
-  DiagnosticBuilder Diag = diag(RootLoc, Message);
+  DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
   for (const auto &T : *Transformations) {
     Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
   }
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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

 RewriteRule invertIf() {
   using namespace ::clang::ast_matchers;
   using tooling::change;
   using tooling::node;
   using tooling::statement;
   using tooling::stencil::cat;
 
   StringRef C = "C", T = "T", E = "E";
-  return tooling::makeRule(ifStmt(hasCondition(expr().bind(C)),
+  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))));
+  Rule.Cases[0].Explanation = tooling::text("no explanation");
+  return Rule;
 }
 
 class IfInverterCheck : public TransformerClangTidyCheck {
 public:
   IfInverterCheck(StringRef Name, ClangTidyContext *Context)
       : TransformerClangTidyCheck(invertIf(), Name, Context) {}
 };