Implement substitutions of placeholders in the replacement text of the fixits.

[alexfh]

Summary

This allows using configurable strings in fixits, such as user name or
email in TODO() comments in certain styles.

[djasper]

I am not 100% sure the additional code and maintenance costs here pay for the added benefit. Some random thoughts:

• Simply replacing predefined variables is powerful in some ways, but might be insufficient for other configurations things we might want to do. Maybe we should instead consider adapting the system that creates/formats Clang's diagnostics.
• Added the right username in a TODO comment seems to be a really special case. First, it seems the only thing I can come up with that cannot be easily determine when the actual check is executed. Second, the value of adding the right user name (or of this check having an automated fix at all) seems relatively small.

I'd like to think some more (and maybe get additional impact), before going forward with this patch as implemented.

[klimek]

My expectation would have been to add the user name to the config, and have a method to get the user name accessible from checks; if somebody then wants to insert a unique token for the username (such as <<<USERNAME>>>) and resolve that later, that's fine - and it can be resolved by the outer layer (which needs to exist anyway in that case) running sed over the changed files.

[alexfh]

We probably don't need this solution until we have at least one more use case for the delayed substitution. I've sent an alternative patch: D5440.

clang-tidy/ClangTidyDiagnosticConsumer.h

private:
 
   /// \brief Sets the \c DiagnosticsEngine so that Diagnostics can be generated
   /// correctly.
   void setDiagnosticsEngine(DiagnosticsEngine *Engine);
 
   /// \brief Store an \p Error.
   void storeError(const ClangTidyError &Error);
 
+  /// \brief Applies \c ClangTidyOptions::Substitutions to each
+  /// \c ClangTidyError::Fix's replacement text.
+  void applySubstitutions(ClangTidyError &Error) const;
+
   std::vector<ClangTidyError> Errors;
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   std::unique_ptr<GlobList> CheckFilter;
 

clang-tidy/ClangTidyDiagnosticConsumer.cpp

 GlobList &ClangTidyContext::getChecksFilter() {
   assert(CheckFilter != nullptr);
   return *CheckFilter;
 }
 
 /// \brief Store a \c ClangTidyError.
 void ClangTidyContext::storeError(const ClangTidyError &Error) {
   Errors.push_back(Error);
+  applySubstitutions(Errors.back());
+}
+
+void ClangTidyContext::applySubstitutions(ClangTidyError &Error) const {
+  std::vector<std::pair<const tooling::Replacement &, tooling::Replacement>>
+      ChangedReplacements;
+  for (const auto &Replacement : Error.Fix) {
+    std::string ReplacementText = Replacement.getReplacementText().str();
+    for (const auto &Substitution : getOptions().Substitutions) {
+      const std::string &From = Substitution.first;
+      const std::string &To = Substitution.second;
+      std::string::size_type Pos = 0;
+      while ((Pos = ReplacementText.find(From, Pos)) != std::string::npos) {
+        ReplacementText = ReplacementText.substr(0, Pos) + To +
+                          ReplacementText.substr(Pos + From.size());
+        Pos += To.size();
+      }
+    }
+    if (Replacement.getReplacementText() != ReplacementText) {
+      ChangedReplacements.emplace_back(
+          Replacement, tooling::Replacement(
+                           Replacement.getFilePath(), Replacement.getOffset(),
+                           Replacement.getLength(), ReplacementText));
+    }
+  }
+
+  for (const auto &ChangedReplacement : ChangedReplacements) {
+    Error.Fix.erase(ChangedReplacement.first);
+    Error.Fix.insert(ChangedReplacement.second);
+  }
 }
 
 StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   llvm::DenseMap<unsigned, std::string>::const_iterator I =
       CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";

clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
   /// \brief Turns on temporary destructor-based analysis.
   llvm::Optional<bool> AnalyzeTemporaryDtors;
 
   typedef std::pair<std::string, std::string> StringPair;
   typedef std::map<std::string, std::string> OptionMap;
 
   /// \brief Key-value mapping used to store check-specific options.
   OptionMap CheckOptions;
+
+  /// \brief Maps placeholders to the content that they should be replaced with.
+  ///
+  /// ClangTidy supports substitiution of placeholders in replacement strings of
+  /// the \c FixItHints. \c Substitutions maps placeholders to the text inserted
+  /// instead.
+  OptionMap Substitutions;
+
+  /// \brief Placeholders available globally.
+  struct Placeholders {
+    /// \brief This placeholder can be used whenever the user's name/email is
+    /// needed (e.g. in TODO() comments in certain styles).
+    static const char User[];
+  };
 };
 
 /// \brief Abstract interface for retrieving various ClangTidy options.
 class ClangTidyOptionsProvider {
 public:
   virtual ~ClangTidyOptionsProvider() {}
 
   /// \brief Returns global options, which are independent of the file.

clang-tidy/ClangTidyOptions.cpp

struct NOptionMap {
   }
   std::vector<ClangTidyOptions::StringPair> Options;
 };
 
 template <> struct MappingTraits<ClangTidyOptions> {
   static void mapping(IO &IO, ClangTidyOptions &Options) {
     MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
         IO, Options.CheckOptions);
+    MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NSubsts(
+        IO, Options.Substitutions);
     IO.mapOptional("Checks", Options.Checks);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
     IO.mapOptional("CheckOptions", NOpts->Options);
+    IO.mapOptional("Substitutions", NSubsts->Options);
+  }
+  static StringRef validate(IO &io, ClangTidyOptions &Options) {
+    for (const auto &S : Options.Substitutions) {
+      StringRef Key = S.first;
+      if (!Key.startswith("${") || !Key.endswith("}"))
+        return "Substitution name has to be enclosed in ${}";
+    }
+    return StringRef();
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
 
+const char ClangTidyOptions::Placeholders::User[] = "${ClangTidy.User}";
+
 ClangTidyOptions
 ClangTidyOptions::mergeWith(const ClangTidyOptions &Other) const {
   ClangTidyOptions Result = *this;
 
   // Merge comma-separated glob lists by appending the new value after a comma.
   if (Other.Checks)
     Result.Checks =
         (Result.Checks && !Result.Checks->empty() ? *Result.Checks + "," : "") +
         *Other.Checks;
 
   if (Other.HeaderFilterRegex)
     Result.HeaderFilterRegex = Other.HeaderFilterRegex;
   if (Other.AnalyzeTemporaryDtors)
     Result.AnalyzeTemporaryDtors = Other.AnalyzeTemporaryDtors;
 
   for (const auto &KeyValue : Other.CheckOptions)
     Result.CheckOptions[KeyValue.first] = KeyValue.second;
 
+  for (const auto &KeyValue : Other.Substitutions)
+    Result.Substitutions[KeyValue.first] = KeyValue.second;
+
   return Result;
 }
 
 FileOptionsProvider::FileOptionsProvider(
     const ClangTidyGlobalOptions &GlobalOptions,
     const ClangTidyOptions &FallbackOptions,
     const ClangTidyOptions &OverrideOptions)
     : DefaultOptionsProvider(GlobalOptions, FallbackOptions),

clang-tidy/google/TodoCommentCheck.h

 namespace readability {
 
 /// \brief Finds TODO comments without a username or bug number.
 ///
 /// Corresponding cpplint.py check: readability/todo
 class TodoCommentCheck : public ClangTidyCheck {
 public:
   TodoCommentCheck(StringRef Name, ClangTidyContext *Context);
-  ~TodoCommentCheck();
   void registerPPCallbacks(CompilerInstance &Compiler) override;
 
 private:
   class TodoCommentHandler;
   std::unique_ptr<TodoCommentHandler> Handler;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_TODOCOMMENTCHECK_H

clang-tidy/google/TodoCommentCheck.cpp

if (!TodoMatch.match(Text, &Matches))
       return false;
 
     StringRef Username = Matches[1];
     StringRef Comment = Matches[3];
 
     if (!Username.empty())
       return false;
 
-    // If the username is missing put in the current user's name. Not ideal but
-    // works for running tidy locally.
-    // FIXME: Can we get this from a more reliable source?
-    const char *User = std::getenv("USER");
-    if (!User)
-      User = "unknown";
-    std::string NewText = ("// TODO(" + Twine(User) + "): " + Comment).str();
+    std::string NewText =
+        ("// TODO(" + Twine(ClangTidyOptions::Placeholders::User) + "): " +
+         Comment).str();
 
     Check.diag(Range.getBegin(), "missing username/bug in TODO")
         << FixItHint::CreateReplacement(CharSourceRange::getCharRange(Range),
                                         NewText);
     return false;
   }
 
 private:
   TodoCommentCheck &Check;
   llvm::Regex TodoMatch;
 };
 
 TodoCommentCheck::TodoCommentCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       Handler(llvm::make_unique<TodoCommentHandler>(*this)) {}
 
-TodoCommentCheck::~TodoCommentCheck() {}
-
 void TodoCommentCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Compiler.getPreprocessor().addCommentHandler(Handler.get());
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tidy/tool/ClangTidyMain.cpp

 ///  This tool uses the Clang Tooling infrastructure, see
 ///    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 ///  for details on setting it up with LLVM source tree.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/Support/Process.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
 using namespace clang::tooling;
 using namespace llvm;
 
 static cl::OptionCategory ClangTidyCategory("clang-tidy options");
 
int clangTidyMain(int argc, const char **argv) {
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
     OverrideOptions.Checks = Checks;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (AnalyzeTemporaryDtors.getNumOccurrences() > 0)
     OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
 
+  // FIXME: Allow this to be overridden from a config file.
+  if (llvm::Optional<std::string> User = llvm::sys::Process::GetEnv("USER"))
+    OverrideOptions.Substitutions[ClangTidyOptions::Placeholders::User] = *User;
+
   auto OptionsProvider = llvm::make_unique<FileOptionsProvider>(
       GlobalOptions, FallbackOptions, OverrideOptions);
 
   std::string FileName = OptionsParser.getSourcePathList().front();
   ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FileName);
   std::vector<std::string> EnabledChecks = getCheckNames(EffectiveOptions);
 
   // FIXME: Allow using --list-checks without positional arguments.

unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp

 #include "ClangTidy.h"
 #include "ClangTidyTest.h"
+#include "clang/Lex/Lexer.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace test {
 
 class TestCheck : public ClangTidyCheck {
 public:

 TEST(ClangTidyDiagnosticConsumer, SortsErrors) {
   std::vector<ClangTidyError> Errors;
   runCheckOnCode<TestCheck>("int a;", &Errors);
   EXPECT_EQ(2ul, Errors.size());
   EXPECT_EQ("type specifier", Errors[0].Message.Message);
   EXPECT_EQ("variable", Errors[1].Message.Message);
 }
 
+TEST(ClangTidyDiagnosticConsumer, HandlesSubstitutions) {
+  class SubstitutionsTestCheck : public TestCheck {
+  public:
+    SubstitutionsTestCheck(StringRef Name, ClangTidyContext *Context)
+        : TestCheck(Name, Context) {}
+    void check(const ast_matchers::MatchFinder::MatchResult &Result) override {
+      const VarDecl *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+      diag(Var->getTypeSpecStartLoc(), "around type")
+          << FixItHint::CreateInsertion(Var->getTypeSpecStartLoc(),
+                                        ClangTidyOptions::Placeholders::User)
+          << FixItHint::CreateInsertion(
+                 Lexer::getLocForEndOfToken(Var->getTypeSpecStartLoc(), 0,
+                                            *Result.SourceManager,
+                                            Result.Context->getLangOpts()),
+                 ClangTidyOptions::Placeholders::User);
+      diag(Var->getLocation(), "instead of name")
+          << FixItHint::CreateReplacement(
+              CharSourceRange::getTokenRange(Var->getLocation(),
+                                             Var->getLocation()),
+              "\\${aaa}/${aaa}${aaa}${}}}{{");
+    }
+  };
+
+  std::vector<ClangTidyError> Errors;
+  ClangTidyOptions Options;
+  std::string ActualUser = "<<<user \\ / $ ^ \\\\ \n\t>>>";
+  Options.Substitutions[ClangTidyOptions::Placeholders::User] = ActualUser;
+  Options.Substitutions["${aaa}"] = "123";
+  std::string Replacement2 = "\\123/123123${}}}{{";
+  std::string Result = runCheckOnCode<SubstitutionsTestCheck>(
+      "int abc;", &Errors, "input.cc", None, Options);
+  EXPECT_EQ(ActualUser + "int" + ActualUser + " " + Replacement2 + ";", Result);
+  EXPECT_EQ(2ul, Errors.size());
+  EXPECT_EQ("around type", Errors[0].Message.Message);
+  EXPECT_EQ(2ul, Errors[0].Fix.size());
+  for (const auto &Fix : Errors[0].Fix)
+    EXPECT_EQ(ActualUser, Fix.getReplacementText());
+
+  EXPECT_EQ("instead of name", Errors[1].Message.Message);
+  EXPECT_EQ(1ul, Errors[1].Fix.size());
+  EXPECT_EQ(Replacement2, Errors[1].Fix.begin()->getReplacementText());
+}
+
 TEST(GlobList, Empty) {
   GlobList Filter("");
 
   EXPECT_TRUE(Filter.contains(""));
   EXPECT_FALSE(Filter.contains("aaa"));
 }
 
 TEST(GlobList, Nothing) {

unittests/clang-tidy/ClangTidyTest.h

bool BeginSourceFileAction(CompilerInstance &Compiler,
     return true;
   }
 
   ClangTidyCheck &Check;
   ClangTidyContext *Context;
 };
 
 template <typename T>
-std::string runCheckOnCode(StringRef Code,
-                           std::vector<ClangTidyError> *Errors = nullptr,
-                           const Twine &Filename = "input.cc",
-                           ArrayRef<std::string> ExtraArgs = None) {
+std::string
+runCheckOnCode(StringRef Code, std::vector<ClangTidyError> *Errors = nullptr,
+               ArrayRef<std::string> ExtraArgs = None,
+               llvm::Optional<ClangTidyOptions> OverrideOptions = None) {
   ClangTidyOptions Options;
   Options.Checks = "*";
+  if (OverrideOptions)
+    Options = Options.mergeWith(*OverrideOptions);
   ClangTidyContext Context(llvm::make_unique<DefaultOptionsProvider>(
       ClangTidyGlobalOptions(), Options));
   ClangTidyDiagnosticConsumer DiagConsumer(Context);
   T Check("test-check", &Context);
   std::vector<std::string> ArgCXX11(1, "-std=c++11");
   ArgCXX11.insert(ArgCXX11.end(), ExtraArgs.begin(), ExtraArgs.end());
 
   if (!tooling::runToolOnCodeWithArgs(new TestPPAction(Check, &Context), Code,