[ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.

[hokein]

Update

[alexfh]

Nice! I like the idea, but we need to figure out a bunch of details (see the comments inline).

clang-tidy/ClangTidyOptions.cpp

268

ConfigFile.str() should also work.

clang-tidy/tool/ClangTidyMain.cpp

131

I would expect something different from an option named '-explain-checks', e.g. some kind of a short documentation on each check or the like. Something like explain-config or describe-config would be closer to what the option does, I think.

132

How about for each enabled check explains, where it is enabled, i.e. in clang-tidy binary, command line or a specific configuration file?

265

s/ClangTidy/clang-tidy/

279

s/argument 'checks'/option '-checks'/

297

s/argument 'config'/option '-config'/

334

I'm not sure I understand how this should work in case a check is:

1. enabled in binary (e.g. -*,a)
2. disabled in the config file (e.g. -a)
3. enabled again on the command line (e.g. a)

clang-tidy should say 'command line' in this case, but i'm not sure what your implementation will do, since it doesn't seem to keep the order of the configurations.

336

nit: Filter

You could also write if (GlobList(CheckSource.first).contains(Check)) {

338

s/comes from/is enabled in the/

[hokein]

Update.

[hokein]

Remove redundant code.

[hokein]

clang-tidy/tool/ClangTidyMain.cpp

334

I updated the patch and made sure the behavior keep the order of configuration.

But if the case like:

1. enabled in binary
2. enabled in config file
3. enabled in command-line option -checks

Only command-line option -checks will be diagnosed, since the processor order of configuration is binary -> config file -> command-line option -checks.

[alexfh]

clang-tidy/tool/ClangTidyMain.cpp

334

As discussed offline, the order of iteration matters here. And it looks like we should look at the complete Checks options without splitting them to components (in order to handle negative patterns).

[hokein]

Correct the processing orders.

[hokein]

More comments.

[alexfh]

clang-tidy/ClangTidyOptions.cpp

269

Can we use .emplace_back here?

clang-tidy/ClangTidyOptions.h

91

If it's from low to high, I'd use < instead of > to avoid confusion ;)

99

1. Use /// for doxygen comments.
2. Insert an empty comment line after this line to mark the end of the \brief block.
3. Mark the list items with *.

104

Can we store a single vector of string pairs instead of three different vectors? The second item of the pair already says enough about the source. And this being an array doesn't seem to be useful.

clang-tidy/tool/ClangTidyMain.cpp

281

Please pull this string literal to a constant: it's used more than once in the code. Maybe also pull the "clang-tidy binary" string too, for consistency.

[alexfh]

clang-tidy/tool/ClangTidyMain.cpp

281

Err, the strings are actually different, but I'd still make all three of them constants.

[hokein]

Use one vector storing checks details instead of three.

[alexfh]

I've just realized that the approach is artificially limited to just keeping track of the origin of the Checks option, while it could be easily extended to track the origin of all configuration items. What if instead of std::vector<StringPair> CheckSources; we introduce std::vector<std::pair<std::string, ClangTidyOptions>> Sources;? Then we could be able to find where a certain check option or an extra argument was introduced, for example. WDYT?

clang-tidy/tool/ClangTidyMain.cpp

188

One problem with this way of defining string constants is that they are not constants. CheckSourceTypeDefaultBinary = "oops"; will work just fine. Instead, I prefer to use constexpr char XXX[] = "..."; construct.

[hokein]

Keep track all the fields of ClangTidyOptions.

[hokein]

In D18694#396797, @alexfh wrote:

I've just realized that the approach is artificially limited to just keeping track of the origin of the Checks option, while it could be easily extended to track the origin of all configuration items. What if instead of std::vector<StringPair> CheckSources; we introduce std::vector<std::pair<std::string, ClangTidyOptions>> Sources;? Then we could be able to find where a certain check option or an extra argument was introduced, for example. WDYT?

Update the patch according to our offline discussion. One different thing is that we can't assume that Other in merge(CTO& Other) has empty HigherPriorityOptions because some usages break this assumption.

[hokein]

Don't modify unrelevant code.

[alexfh]

clang-tidy/ClangTidyOptions.h

99

As per offline discussion, there might be a more elegant and robust solution to this. For example, extending the ClangTidyOptionsProvider interface with a virtual getRawOptions method (ideas for a better name are welcome) returning an ordered list of ClangTidyOption objects accompanied by textual descriptions of their origin (defaults/configuration file name/command line option name). The virtual ClangTidyOptions getOptions(llvm::StringRef FileName); method can then be made non-virtual and serve as an interface for option consumers only (it would just merge all options returned by the former method).

[hokein]

Use a more elegant solution to track ClangTidyOptions.

[hokein]

Update.

[hokein]

Export SourceType.

[alexfh]

Looks better now, thanks!

clang-tidy/ClangTidyOptions.cpp

170

I'm not sure this will compile on VS2013.

186

Maybe emplace_back?

clang-tidy/ClangTidyOptions.h

124

Clang-format, please.

219

nit: Name should start with a lower-case letter.

clang-tidy/tool/ClangTidyMain.cpp

339

No need for parentheses around It->first.Checks.

[hokein]

Address code review comments.

[hokein]

clang-tidy/ClangTidyOptions.cpp

170

Yeah, you are right. VS2013 doesn't work with vector brace-initialization.

[hokein]

• VS2013 doesn't support constexpr.
• Don't make test read external .clang-tidy file.

[alexfh]

Sorry for the long delay. This is getting closer. A few more comments though.

clang-tidy/ClangTidyOptions.cpp

199

s/ = \s*OptionsSource//

207

This seems to be changing the caching logic. Consider this directory structure:

a/
   .clang-tidy
   b/
      c/

And consequtive getRawOptions calls with:

1. "a/some_file"
2. "a/b/c/some_file"
3. "a/b/some_file".

What would happen previously:

1. after call 1 CachedOptions would contain an entry for "a"
2. call 2 would find an entry for "a" and copy it for "a/b" and "a/b/c"
3. call 3 would just use the cache entry for "a/b"

Now step 2 doesn't copy the cache entry to "a/b" and "a/b/c".

Is there any specific reason to change this? This is benign given that the lookups happen in memory, but then the code needs to be consistent and avoid replicating cache entries to intermediate directories in all cases.

clang-tidy/ClangTidyOptions.h

115

As we figured out with another patch, constexpr is not supported by VS2013. Sorry for pushing you in a wrong direction.

122

s/from low to top/in order of increasing priority/

122

Let's move the implementation to the .cpp file. It's only used by a small fraction of translation units including this header, and it doesn't seem to be performance-critical to make this method inline.

124

This item should make it clear how a config file relates to the '-config' option in terms of priority. The easiest way to make the order clear is probably to split the item in two (and update the "3 types of the sources" above).

137

clang-format?

A few more instances below.

152

clang-format?

clang-tidy/tool/ClangTidyMain.cpp

333

"a more elegant way" seems to suggest that currently this already happens, but in a not elegant fashion. As far as I understand, we're not doing this at all ;)

[hokein]

Address comments.

[hokein]

clang-tidy/ClangTidyOptions.cpp

207

Oh, I add a break statement here accidently. Remove it, and keep the caching logic here now.

clang-tidy/ClangTidyOptions.h

124

Explaining the priority of config option and config file is't reasonable here since clang-tidy only takes one of them. If the config option is specified, clang-tidy just ignores the config file.

[alexfh]

Looks good with one nit.

Thank you for implementing this!

clang-tidy/tool/ClangTidyMain.cpp

333

Sorry for being not clear. We still need to find a way to display the source of other components of the configuration. So the FIXME was fine, just the wording was confusing. Please add the FIXME back, but just remove the "more elegant" part.

[alexfh]

One more nit.

clang-tidy/ClangTidyOptions.h

214

clang-format?

Consider making git clang-format handle it for you ;)

[hokein]

Fix remaining nits.

[hokein]

clang-tidy/ClangTidyOptions.h

214

Done. Format the whole patch now.

clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
   llvm::Optional<std::string> User;
 
   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;
 
+  enum CheckSourceType {
+    DefaultBinary,
+    ConfigCommandlineOptionOrFile,
+    ChecksCommandlineOption,
+    CheckSourceTypeEnd
+  };
+  // \brief Stores each check filter and its source for every check source type.
+  // clang-tidy has 3 types of check sources:
+  //    clang-tidy binary
+  //    '-config' commandline option or a specific configuration file
+  //    '-checks' commandline option
+  std::vector<StringPair> CheckSources[CheckSourceTypeEnd];
+
   typedef std::vector<std::string> ArgList;
 
   /// \brief Add extra compilation arguments to the end of the list.
   llvm::Optional<ArgList> ExtraArgs;
 
   /// \brief Add extra compilation arguments to the start of the list.
   llvm::Optional<ArgList> ExtraArgsBefore;
 };
 
 /// \brief Abstract interface for retrieving various ClangTidy options.
 class ClangTidyOptionsProvider {
 public:
   virtual ~ClangTidyOptionsProvider() {}
 
   /// \brief Returns global options, which are independent of the file.
   virtual const ClangTidyGlobalOptions &getGlobalOptions() = 0;
 
   /// \brief Returns options applying to a specific translation unit with the
   /// specified \p FileName.
   virtual ClangTidyOptions getOptions(llvm::StringRef FileName) = 0;
 };
 
 /// \brief Implementation of the \c ClangTidyOptionsProvider interface, which
 /// returns the same options for all files.
 class DefaultOptionsProvider : public ClangTidyOptionsProvider {
 public:
   DefaultOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
                          const ClangTidyOptions &Options)
       : GlobalOptions(GlobalOptions), DefaultOptions(Options) {}
   const ClangTidyGlobalOptions &getGlobalOptions() override {
     return GlobalOptions;
   }
   ClangTidyOptions getOptions(llvm::StringRef /*FileName*/) override {
     return DefaultOptions;
   }
 
 private:
   ClangTidyGlobalOptions GlobalOptions;
   ClangTidyOptions DefaultOptions;
 };
 
 /// \brief Implementation of the \c ClangTidyOptionsProvider interface, which
 /// tries to find a configuration file in the closest parent directory of each
 /// source file.
 ///
 /// By default, files named ".clang-tidy" will be considered, and the
 /// \c clang::tidy::parseConfiguration function will be used for parsing, but a
 /// custom set of configuration file names and parsing functions can be
 /// specified using the appropriate constructor.
 class FileOptionsProvider : public DefaultOptionsProvider {
 public:
   // \brief A pair of configuration file base name and a function parsing
   // configuration from text in the corresponding format.
   typedef std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions>(
                                      llvm::StringRef)>> ConfigFileHandler;
 
   /// \brief Configuration file handlers listed in the order of priority.
   ///
public:
   /// that can parse configuration from this file type. The configuration files
   /// in each directory are searched for in the order of appearance in
   /// \p ConfigHandlers.
   FileOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
                       const ClangTidyOptions &DefaultOptions,
                       const ClangTidyOptions &OverrideOptions,
                       const ConfigFileHandlers &ConfigHandlers);
 
   ClangTidyOptions getOptions(llvm::StringRef FileName) override;
 
 protected:
   /// \brief Try to read configuration files from \p Directory using registered
   /// \c ConfigHandlers.
   llvm::Optional<ClangTidyOptions> TryReadConfigFile(llvm::StringRef Directory);
 
   llvm::StringMap<ClangTidyOptions> CachedOptions;
   ClangTidyOptions OverrideOptions;
   ConfigFileHandlers ConfigHandlers;
 };
 
 /// \brief Parses LineFilter from JSON and stores it to the \p Options.
 std::error_code parseLineFilter(llvm::StringRef LineFilter,

clang-tidy/ClangTidyOptions.cpp

ClangTidyOptions ClangTidyOptions::getDefaults() {
   return Options;
 }
 
 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)
+  if (Other.Checks) {
     Result.Checks =
         (Result.Checks && !Result.Checks->empty() ? *Result.Checks + "," : "") +
         *Other.Checks;
+
+    for (auto SourceType : {ClangTidyOptions::ChecksCommandlineOption,
+                            ClangTidyOptions::ConfigCommandlineOptionOrFile,
+                            ClangTidyOptions::DefaultBinary}) {
+      for (const auto &CheckSource : Other.CheckSources[SourceType])
+        Result.CheckSources[SourceType].push_back(CheckSource);
+    }
+  }
   if (Other.WarningsAsErrors)
     Result.WarningsAsErrors =
         (Result.WarningsAsErrors && !Result.WarningsAsErrors->empty()
              ? *Result.WarningsAsErrors + ","
              : "") +
         *Other.WarningsAsErrors;
 
   if (Other.HeaderFilterRegex)

 FileOptionsProvider::FileOptionsProvider(
     const ClangTidyGlobalOptions &GlobalOptions,
     const ClangTidyOptions &DefaultOptions,
     const ClangTidyOptions &OverrideOptions)
     : DefaultOptionsProvider(GlobalOptions, DefaultOptions),
       OverrideOptions(OverrideOptions) {
   ConfigHandlers.emplace_back(".clang-tidy", parseConfiguration);
   CachedOptions[""] = DefaultOptions.mergeWith(OverrideOptions);
 }
 
 FileOptionsProvider::FileOptionsProvider(
     const ClangTidyGlobalOptions &GlobalOptions,
     const ClangTidyOptions &DefaultOptions,
     const ClangTidyOptions &OverrideOptions,
     const FileOptionsProvider::ConfigFileHandlers &ConfigHandlers)
     : DefaultOptionsProvider(GlobalOptions, DefaultOptions),
       OverrideOptions(OverrideOptions), ConfigHandlers(ConfigHandlers) {
   CachedOptions[""] = DefaultOptions.mergeWith(OverrideOptions);
 }
 
 // FIXME: This method has some common logic with clang::format::getStyle().
 // Consider pulling out common bits to a findParentFileWithName function or
 // similar.
 ClangTidyOptions FileOptionsProvider::getOptions(StringRef FileName) {
   DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n");
   SmallString<256> FilePath(FileName);
 
   if (std::error_code EC = llvm::sys::fs::make_absolute(FilePath)) {
     llvm::errs() << "Can't make absolute path from " << FileName << ": "
                  << EC.message() << "\n";
     // FIXME: Figure out what to do.
   } else {
     FileName = FilePath;
   }
 
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef Path = llvm::sys::path::parent_path(FileName);
   for (StringRef CurrentPath = Path;;
        CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
     llvm::Optional<ClangTidyOptions> Result;
 
     auto Iter = CachedOptions.find(CurrentPath);
     if (Iter != CachedOptions.end())
       Result = Iter->second;
 
     if (!Result)
       Result = TryReadConfigFile(CurrentPath);
 
     if (Result) {
       // Store cached value for all intermediate directories.
       while (Path != CurrentPath) {
         DEBUG(llvm::dbgs() << "Caching configuration for path " << Path
                            << ".\n");
for (const ConfigFileHandler &ConfigHandler : ConfigHandlers) {
     llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
         ConfigHandler.second((*Text)->getBuffer());
     if (!ParsedOptions) {
       if (ParsedOptions.getError())
         llvm::errs() << "Error parsing " << ConfigFile << ": "
                      << ParsedOptions.getError().message() << "\n";
       continue;
     }
-
+    if (ParsedOptions->Checks) {
+      ParsedOptions
+          ->CheckSources[ClangTidyOptions::ConfigCommandlineOptionOrFile]
+          .push_back(ClangTidyOptions::StringPair(*ParsedOptions->Checks,
+                                                  ConfigFile.c_str()));
+    }
     return DefaultOptionsProvider::getOptions(Directory)
         .mergeWith(*ParsedOptions)
         .mergeWith(OverrideOptions);
   }
   return llvm::None;
 }
 
 /// \brief Parses -line-filter option and stores it to the \c Options.

clang-tidy/tool/ClangTidyMain.cpp

)"),
                                cl::init(false), cl::cat(ClangTidyCategory));
 
 static cl::opt<bool> ListChecks("list-checks", cl::desc(R"(
 List all enabled checks and exit. Use with
 -checks=* to list all available checks.
 )"),
                                 cl::init(false), cl::cat(ClangTidyCategory));
 
+static cl::opt<bool> ExplainConfig("explain-config", cl::desc(R"(
+for each enabled check explains, where it is enabled, i.e. in clang-tidy binary,
+command line or a specific configuration file.
+)"),
+                                   cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt<std::string> Config("config", cl::desc(R"(
 Specifies a configuration in YAML/JSON format:
   -config="{Checks: '*',
             CheckOptions: [{key: x,
                             value: y}]}"
 When the value is empty, clang-tidy will
 attempt to find a file named .clang-tidy for
 each source file in its parent directories.

 code with clang-apply-replacements.
 )"),
                                         cl::value_desc("filename"),
                                         cl::cat(ClangTidyCategory));
 
 namespace clang {
 namespace tidy {
 
 static void printStats(const ClangTidyStats &Stats) {
   if (Stats.errorsIgnored()) {
     llvm::errs() << "Suppressed " << Stats.errorsIgnored() << " warnings (";
     StringRef Separator = "";
     if (Stats.ErrorsIgnoredNonUserCode) {
       llvm::errs() << Stats.ErrorsIgnoredNonUserCode << " in non-user code";
       Separator = ", ";
     }
     if (Stats.ErrorsIgnoredLineFilter) {
static std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider() {
   if (std::error_code Err = parseLineFilter(LineFilter, GlobalOptions)) {
     llvm::errs() << "Invalid LineFilter: " << Err.message() << "\n\nUsage:\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return nullptr;
   }
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.CheckSources[ClangTidyOptions::DefaultBinary].push_back(
+      ClangTidyOptions::StringPair(DefaultChecks, "clang-tidy binary"));
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
   // USERNAME is used on Windows.
   if (!DefaultOptions.User)
     DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
 
   ClangTidyOptions OverrideOptions;
-  if (Checks.getNumOccurrences() > 0)
+  if (Checks.getNumOccurrences() > 0) {
     OverrideOptions.Checks = Checks;
+    OverrideOptions.CheckSources[ClangTidyOptions::ChecksCommandlineOption]
+        .push_back(ClangTidyOptions::StringPair(
+            Checks, "command-line option '-checks'"));
+  }
   if (WarningsAsErrors.getNumOccurrences() > 0)
     OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (SystemHeaders.getNumOccurrences() > 0)
     OverrideOptions.SystemHeaders = SystemHeaders;
   if (AnalyzeTemporaryDtors.getNumOccurrences() > 0)
     OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
 
   if (!Config.empty()) {
     if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
             parseConfiguration(Config)) {
+      if (ParsedConfig->Checks) {
+        ParsedConfig
+            ->CheckSources[ClangTidyOptions::ConfigCommandlineOptionOrFile]
+            .push_back(ClangTidyOptions::StringPair(
+                *ParsedConfig->Checks, "command-line option '-config'"));
+      }
       return llvm::make_unique<DefaultOptionsProvider>(
           GlobalOptions, ClangTidyOptions::getDefaults()
                              .mergeWith(DefaultOptions)
                              .mergeWith(*ParsedConfig)
                              .mergeWith(OverrideOptions));
     } else {
       llvm::errs() << "Error: invalid configuration specified.\n"
                    << ParsedConfig.getError().message() << "\n";
static int clangTidyMain(int argc, const char **argv) {
   StringRef FileName("dummy");
   auto PathList = OptionsParser.getSourcePathList();
   if (!PathList.empty()) {
     FileName = PathList.front();
   }
   ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FileName);
   std::vector<std::string> EnabledChecks = getCheckNames(EffectiveOptions);
 
+  if (ExplainConfig) {
+    for (const std::string& Check : EnabledChecks) {
+      // Keeps the processing order of different check sources.
+      for (auto SourceType : {ClangTidyOptions::ChecksCommandlineOption,
+                              ClangTidyOptions::ConfigCommandlineOptionOrFile,
+                              ClangTidyOptions::DefaultBinary}) {
+        bool FindMatch = false;
+        for (auto It = EffectiveOptions.CheckSources[SourceType].rbegin();
+             It != EffectiveOptions.CheckSources[SourceType].rend(); ++It) {
+          if (GlobList(It->first).contains(Check)) {
+            llvm::outs() << "'" << Check << "' is enabled in the "
+                         << It->second << ".\n";
+            FindMatch = true;
+            break;
+          }
+          if (FindMatch)
+            break;
+        }
+      }
+    }
+    return 0;
+  }
+
   if (ListChecks) {
     llvm::outs() << "Enabled checks:";
     for (auto CheckName : EnabledChecks)
       llvm::outs() << "\n    " << CheckName;
     llvm::outs() << "\n\n";
     return 0;
   }
 

test/clang-tidy/Inputs/explain-config/.clang-tidy

+Checks: '-*,modernize-use-nullptr'

test/clang-tidy/explain-checks.cpp

+// RUN: clang-tidy -checks=-*,modernize-use-nullptr -explain-config | FileCheck --check-prefix=CHECK-MESSAGE1 %s
+// RUN: clang-tidy -config="{Checks: '-*,modernize-use-nullptr'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE2 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,modernize-use-nullptr'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE3 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,-modernize-use-nullptr'}" %S/Inputs/explain-config/a.cc -explain-config | FileCheck --check-prefix=CHECK-MESSAGE4 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,modernize-*'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE5 %s
+// RUN: clang-tidy -explain-config %S/Inputs/explain-config/a.cc | grep "'modernize-use-nullptr' is enabled in the %S/Inputs/explain-config/.clang-tidy."
+
+// CHECK-MESSAGE1: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+// CHECK-MESSAGE2: 'modernize-use-nullptr' is enabled in the command-line option '-config'.
+// CHECK-MESSAGE3: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+// CHECK-MESSAGE4: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+// CHECK-MESSAGE5: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.