[clang-tidy] More clearly separate public, check-facing APIs from internal ones.

[sammccall]

Summary

This codifies the mostly-current state:

• The stuff in ClangTidy.h is reasonable for checks to depend on.
• The stuff in ClangTidyDiagnosticConsumer.h is a mish-mash of clang-tidy
• checks are expected to treat ClangTidyContext as opaque

implementation details that checks shouldn't depend on (and most don't!)

But because there's no comments and the former currently #includes the latter,
this isn't clear and is violated in places:

• exportReplacements is public but consumes ClangTidyError, a private API
• Context exports all sorts of details publicly that checks shouldn't depend on, but checks must use Context->getOptions() in some cases

[sammccall]

Add FIXME to fuchsia check that uses private APIs.

[steveire]

clang-tidy/ClangTidy.h

11

Clang C++ code does not have any stability requirements. That's quite well-established.

This comment adds a new requirement of stability for this C++ API. You should probably put a RFC on cfe-dev about it. This header is no more public or stable than any other Clang header.

[sammccall]

clang-tidy/ClangTidy.h

11

Clang C++ code does not have any stability requirements. That's quite well-established.

I was aiming for "stability is a design goal", not "this is a stable API and you may not break it".
Happy to take a shot at rewording, maybe you have suggestions?

This comment adds a new requirement of stability for this C++ API. You should probably put a RFC on cfe-dev about it.

Stability here has always been an aim; I'm trying to document the current state of the world.

e.g. in D15528 @alexfh wrote:

It's one of the goals of clang-tidy to provide an easy way to maintain out-of-tree checks.

This header is no more public or stable than any other Clang header.

I wish that were true - my current yakshave is reducing the need for registerPPCallbacks to make clang-tidy more flexible for use in a library. Removing it entirely would be great but having talked to some clang-tidy maintainers, out-of-tree checks are at least a factor here.

[alexfh]

clang-tidy/ClangTidy.h

11

I tend to agree with Stephen re: lack of stability guarantees of Clang APIs. Clang-tidy API on its own is not nearly enough to create a useful check, thus any out-of-tree check will use a significant amount of Clang APIs. Though the project aims at providing an easy way to maintain out-of-tree checks, there's little benefit in clang-tidy APIs being more stable than the API surface of Clang needed for an average out-of-tree check. Maintainers of any clang-based out-of-tree code should be ready to update their code with every change of the clang revision they use. Clang-tidy checks are not an exception. Putting this documentation here may send a very wrong message and serve badly both to the clang-tidy developers and to the folks who decide to depend on it.

A softer version of this (something along the lines of "A large number of out-of-tree checks depend on this API, so be careful when introducing potentially breaking changes.") may work here, but the same note will be needed on the whole AST, ASTMatchers, and Lex libraries in Clang (and many other ones, I guess).

[sammccall]

Rethink comment about public API stability.

[sammccall]

Fair enough.
I think it's worth saying *something* here, as the stability of these API has value and churning them has costs that far exceed (2 of magnitude?) the other C++ APIs in clang-tidy. And out-of-tree dependencies are something I usually want to be aware of when changing an API.

But softened it a lot and made it explicit no stability guarantee is being made.

[steveire]

clang-tidy/ClangTidy.h

11

I have a proposal coming up which involves a breaking change to AST_MATCHER* macros (many of which are out of tree), so I'm really just trying to make sure no backward compatibility concern opposes that.

I still see no point in the comment, if you don't also put the same comment in ASTMatchers.h, each header in AST/ etc. The files have the same out-of-tree use, and no one of them deserves a comment more than the others.

In fact, I had the idea just last week that I want to write a documentation page outlining compatibility guarantees in various user-facing parts of llvm/clang. It would be a single page containing the two sections about compatibility here: https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility, C++ API stability, whether clang-query interpreter commands are stable, whether driver command lines and plumbing command lines are stable etc. AFAIK, that does not exist yet.

Regarding C++ API compatibility something along the lines here (http://clang-developers.42468.n3.nabble.com/getLocStart-versus-getStartLoc-tp4061010p4061292.html) and here (http://clang-developers.42468.n3.nabble.com/API-Removal-Notice-4-weeks-getStartLoc-getLocStart-getLocEnd-td4061566.html) . A note like yours would belong on such a page too. I don't think it belongs in this header.

[lebedev.ri]

clang-tidy/ClangTidy.h

11

I wish that were true - my current yakshave is reducing the need for registerPPCallbacks to make clang-tidy more flexible for use in a library. Removing it entirely would be great but having talked to some clang-tidy maintainers, out-of-tree checks are at least a factor here.

What do you mean by that, exactly?
Having PP callbacks in the clang-tidy checks is essential for many checks.

clang-tidy/ClangTidy.h

 //===--- ClangTidy.h - clang-tidy -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
+// This file defines the public ClangTidy API exposed to checks.
+//
+// Note that a large number of checks use this API, including many out-of-tree.
+// API stability isn't guaranteed, but breaking changes should happen carefully.
+//===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H
 
-#include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include <memory>
 #include <type_traits>
 #include <vector>
 
 namespace clang {
 
 class CompilerInstance;
 namespace tooling {
 class CompilationDatabase;
 }
 
 namespace tidy {
+// Checks should treat ClangTidyContext as opaque.
+class ClangTidyContext;
 
 /// \brief Provides access to the ``ClangTidyCheck`` options via check-local
 /// names.
 ///
 /// Methods of this class prepend ``CheckName + "."`` to translate check-local
 /// option names to global option names.
 class OptionsView {
 public:

 /// useful/necessary.
 class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
 public:
   /// \brief Initializes the check with \p CheckName and \p Context.
   ///
   /// Derived classes must implement the constructor with this signature or
   /// delegate it. If a check needs to read options, it can do this in the
   /// constructor using the Options.get() methods below.
-  ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context)
-      : CheckName(CheckName), Context(Context),
-        Options(CheckName, Context->getOptions().CheckOptions) {
-    assert(Context != nullptr);
-    assert(!CheckName.empty());
-  }
+  ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context);
 
   /// \brief Override this to register ``PPCallbacks`` with ``Compiler``.
   ///
   /// This should be used for clang-tidy checks that analyze preprocessor-
   /// dependent properties, e.g. the order of include directives.
   virtual void registerPPCallbacks(CompilerInstance &Compiler) {}
 
   /// \brief Override this to register AST matchers with \p Finder.

 private:
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
   StringRef getID() const override { return CheckName; }
   std::string CheckName;
   ClangTidyContext *Context;
 
 protected:
   OptionsView Options;
-  /// \brief Returns the main file name of the current translation unit.
-  StringRef getCurrentMainFile() const { return Context->getCurrentFile(); }
-  /// \brief Returns the language options from the context.
-  LangOptions getLangOpts() const { return Context->getLangOpts(); }
+  /// Returns the main file name of the current translation unit.
+  StringRef getCurrentMainFile() const;
+  /// Returns the language options from the current translation unit.
+  LangOptions getLangOpts() const;
 };
 
 class ClangTidyCheckFactories;
 
 class ClangTidyASTConsumerFactory {
 public:
   ClangTidyASTConsumerFactory(ClangTidyContext &Context);
 

 //
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext &Context, bool Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
 
-/// \brief Serializes replacements into YAML and writes them to the specified
-/// output stream.
-void exportReplacements(StringRef MainFilePath,
-                        const std::vector<ClangTidyError> &Errors,
-                        raw_ostream &OS);
-
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H

clang-tidy/ClangTidy.cpp

 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Frontend/FixItRewriter.h"
 #include "clang/Rewrite/Frontend/FrontendActions.h"
 #if CLANG_ENABLE_STATIC_ANALYZER
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #endif // CLANG_ENABLE_STATIC_ANALYZER
-#include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Refactoring.h"
-#include "clang/Tooling/ReplacementsYaml.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include <algorithm>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
ClangTidyASTConsumerFactory::CreateASTConsumer(
   auto WorkingDir = Compiler.getSourceManager()
                         .getFileManager()
                         .getVirtualFileSystem()
                         ->getCurrentWorkingDirectory();
   if (WorkingDir)
     Context.setCurrentBuildDirectory(WorkingDir.get());
 
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
-  CheckFactories->createChecks(&Context, Checks);
+  CheckFactories->createChecks(
+      [this](StringRef Name) { return Context.isCheckEnabled(Name); }, &Context,
+      Checks);
 
   ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
 
   std::unique_ptr<ClangTidyProfiling> Profiling;
   if (Context.getEnableProfiling()) {
     Profiling = llvm::make_unique<ClangTidyProfiling>(
         Context.getProfileStorageParams());
     FinderOptions.CheckProfiling.emplace(Profiling->Records);
#endif // CLANG_ENABLE_STATIC_ANALYZER
 
   std::sort(CheckNames.begin(), CheckNames.end());
   return CheckNames;
 }
 
 ClangTidyOptions::OptionMap ClangTidyASTConsumerFactory::getCheckOptions() {
   ClangTidyOptions::OptionMap Options;
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
-  CheckFactories->createChecks(&Context, Checks);
+  CheckFactories->createChecks(
+      [this](StringRef Name) { return Context.isCheckEnabled(Name); }, &Context,
+      Checks);
   for (const auto &Check : Checks)
     Check->storeOptions(Options);
   return Options;
 }
 
+ClangTidyCheck::ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context)
+    : CheckName(CheckName), Context(Context),
+      Options(CheckName, Context->getOptions().CheckOptions) {
+  assert(Context != nullptr);
+  assert(!CheckName.empty());
+}
+
 DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message,
                                        DiagnosticIDs::Level Level) {
   return Context->diag(CheckName, Loc, Message, Level);
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   Context->setSourceManager(Result.SourceManager);
   check(Result);
 }
 
+StringRef ClangTidyCheck::getCurrentMainFile() const {
+  return Context->getCurrentFile();
+}
+
+LangOptions ClangTidyCheck::getLangOpts() const {
+  return Context->getLangOpts();
+}
+
 OptionsView::OptionsView(StringRef CheckName,
                          const ClangTidyOptions::OptionMap &CheckOptions)
     : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions) {}
 
 std::string OptionsView::get(StringRef LocalName, StringRef Default) const {
   const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
   if (Iter != CheckOptions.end())
     return Iter->second;
for (const ClangTidyError &Error : Context.getErrors()) {
     Reporter.reportDiagnostic(Error);
     // Return to the initial directory to correctly resolve next Error.
     FileSystem.setCurrentWorkingDirectory(InitialWorkingDir.get());
   }
   Reporter.Finish();
   WarningsAsErrorsCount += Reporter.getWarningsAsErrorsCount();
 }
 
-void exportReplacements(const llvm::StringRef MainFilePath,
-                        const std::vector<ClangTidyError> &Errors,
-                        raw_ostream &OS) {
-  TranslationUnitDiagnostics TUD;
-  TUD.MainSourceFile = MainFilePath;
-  for (const auto &Error : Errors) {
-    tooling::Diagnostic Diag = Error;
-    TUD.Diagnostics.insert(TUD.Diagnostics.end(), Diag);
-  }
-
-  yaml::Output YAML(OS);
-  YAML << TUD;
-}
-
 } // namespace tidy
 } // namespace clang

clang-tidy/ClangTidyDiagnosticConsumer.h

 //===--- ClangTidyDiagnosticConsumer.h - clang-tidy -------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
+// This file contains private APIs that checks should not depend on.
+// FIXME: the contents have diverged significantly from the name, split this up.
+//===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H
 
 #include "ClangTidyOptions.h"
 #include "ClangTidyProfiling.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"

 /// FIXME: Make Diagnostics flexible enough to support this directly.
 struct ClangTidyError : tooling::Diagnostic {
   ClangTidyError(StringRef CheckName, Level DiagLevel, StringRef BuildDirectory,
                  bool IsWarningAsError);
 
   bool IsWarningAsError;
 };
 
+/// Serializes replacements into YAML and writes them to the specified stream.
+void exportReplacements(StringRef MainFilePath,
+                        const std::vector<ClangTidyError> &Errors,
+                        raw_ostream &OS);
+
 /// \brief Read-only set of strings represented as a list of positive and
 /// negative globs. Positive globs add all matched strings to the set, negative
 /// globs remove them in the order of appearance in the list.
 class GlobList {
 public:
   /// \brief \p GlobList is a comma-separated list of globs (only '*'
   /// metacharacter is supported) with optional '-' prefix to denote exclusion.
   GlobList(StringRef Globs);

clang-tidy/ClangTidyDiagnosticConsumer.cpp

 ///
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
+#include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include <tuple>
 #include <vector>
 using namespace clang;
 using namespace tidy;
 
 namespace {

 } // end anonymous namespace
 
 ClangTidyError::ClangTidyError(StringRef CheckName,
                                ClangTidyError::Level DiagLevel,
                                StringRef BuildDirectory, bool IsWarningAsError)
     : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
       IsWarningAsError(IsWarningAsError) {}
 
+void tidy::exportReplacements(const llvm::StringRef MainFilePath,
+                        const std::vector<ClangTidyError> &Errors,
+                        raw_ostream &OS) {
+  clang::tooling::TranslationUnitDiagnostics TUD;
+  TUD.MainSourceFile = MainFilePath;
+  for (const auto &Error : Errors) {
+    tooling::Diagnostic Diag = Error;
+    TUD.Diagnostics.insert(TUD.Diagnostics.end(), Diag);
+  }
+
+  llvm::yaml::Output YAML(OS);
+  YAML << TUD;
+}
+
 // Returns true if GlobList starts with the negative indicator ('-'), removes it
 // from the GlobList.
 static bool ConsumeNegativeIndicator(StringRef &GlobList) {
   GlobList = GlobList.trim(" \r\n");
   if (GlobList.startswith("-")) {
     GlobList = GlobList.substr(1);
     return true;
   }

clang-tidy/ClangTidyModule.h

public:
   /// \endcode
   template <typename CheckType> void registerCheck(StringRef CheckName) {
     registerCheckFactory(CheckName,
                          [](StringRef Name, ClangTidyContext *Context) {
                            return new CheckType(Name, Context);
                          });
   }
 
-  /// \brief Create instances of all checks matching \p CheckRegexString and
-  /// store them in \p Checks.
+  /// Create instances of all checks that match the filter and store them.
   ///
   /// The caller takes ownership of the return \c ClangTidyChecks.
-  void createChecks(ClangTidyContext *Context,
+  void createChecks(llvm::function_ref<bool(StringRef)> Filter,
+                    ClangTidyContext *Context,
                     std::vector<std::unique_ptr<ClangTidyCheck>> &Checks);
 
   typedef std::map<std::string, CheckFactory> FactoryMap;
   FactoryMap::const_iterator begin() const { return Factories.begin(); }
   FactoryMap::const_iterator end() const { return Factories.end(); }
   bool empty() const { return Factories.empty(); }
 
 private:

clang-tidy/ClangTidyModule.cpp

 namespace tidy {
 
 void ClangTidyCheckFactories::registerCheckFactory(StringRef Name,
                                                    CheckFactory Factory) {
   Factories[Name] = std::move(Factory);
 }
 
 void ClangTidyCheckFactories::createChecks(
+    llvm::function_ref<bool(StringRef)> Filter,
     ClangTidyContext *Context,
     std::vector<std::unique_ptr<ClangTidyCheck>> &Checks) {
   for (const auto &Factory : Factories) {
-    if (Context->isCheckEnabled(Factory.first))
+    if (Filter(Factory.first))
       Checks.emplace_back(Factory.second(Factory.first, Context));
   }
 }
 
 ClangTidyOptions ClangTidyModule::getModuleOptions() {
   return ClangTidyOptions();
 }
 
 } // namespace tidy
 } // namespace clang

clang-tidy/fuchsia/RestrictSystemIncludesCheck.h

 //===--- RestrictSystemIncludesCheck.h - clang-tidy---------- ----*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_RESTRICTINCLUDESSCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_RESTRICTINCLUDESSCHECK_H
 
 #include "../ClangTidy.h"
+// FIXME: stop using GlobList, it's not part of the public interface.
 #include "../ClangTidyDiagnosticConsumer.h"
 #include "../utils/OptionsUtils.h"
 
 namespace clang {
 namespace tidy {
 namespace fuchsia {
 
 /// Checks for allowed includes and suggests removal of any others. If no

clang-tidy/google/TodoCommentCheck.cpp

 //===--- TodoCommentCheck.cpp - clang-tidy --------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TodoCommentCheck.h"
+// FIXME: provide some other way to get at the username.
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 namespace tidy {
 namespace google {
 namespace readability {
 

 
 private:
   TodoCommentCheck &Check;
   std::string User;
   llvm::Regex TodoMatch;
 };
 
 TodoCommentCheck::TodoCommentCheck(StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      Handler(llvm::make_unique<TodoCommentHandler>(
-          *this, Context->getOptions().User)) {}
+    : ClangTidyCheck(Name, Context) {
+  Handler =
+      llvm::make_unique<TodoCommentHandler>(*this, Context->getOptions().User);
+}
 
 void TodoCommentCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Compiler.getPreprocessor().addCommentHandler(Handler.get());
 }
 
 } // namespace readability
 } // namespace google
 } // namespace tidy
 } // namespace clang

clang-tidy/plugin/ClangTidyPlugin.cpp

 //===- ClangTidyPlugin.cpp - clang-tidy as a clang plugin -----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "../ClangTidyModule.h"
 #include "clang/Config/config.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/Frontend/MultiplexConsumer.h"
 
 namespace clang {
 namespace tidy {

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 "../ClangTidyDiagnosticConsumer.h"
 #include "clang/Config/config.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/TargetSelect.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
 using namespace clang::tooling;