[clang-tidy] Refactor InheritParentConfig from ClangTidyOptions

[njames93]

Summary

This option doesn't make much sense being in ClangTidyOptions as it is only used when reading configuration.
Instead of putting the option there, a new class ClangTidyInheritableOptions has been added, which extends ClangTidyOptions.
This should be used when reading configuration that is able to inherit from a parent.

[sammccall]

This solves the problem, but in a heavyweight way that seems out-of-proportion to the size of the problem.

If it doesn't make sense to change the Optional<bool> to bool = false and deal wit the merging in an ad-hoc way, I think I prefer the way it is now.

[njames93]

In D92199#2419863, @sammccall wrote:

This solves the problem, but in a heavyweight way that seems out-of-proportion to the size of the problem.

If it doesn't make sense to change the Optional<bool> to bool = false and deal wit the merging in an ad-hoc way, I think I prefer the way it is now.

It's not super heavyweight, but I agree its a little more involved than expected for the small problem it fixes

clang-tools-extra/clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
   typedef std::vector<std::string> ArgList;
 
   /// Add extra compilation arguments to the end of the list.
   llvm::Optional<ArgList> ExtraArgs;
 
   /// Add extra compilation arguments to the start of the list.
   llvm::Optional<ArgList> ExtraArgsBefore;
 
-  /// Only used in the FileOptionsProvider and ConfigOptionsProvider. If true
-  /// and using a FileOptionsProvider, it will take a configuration file in the
-  /// parent directory (if any exists) and apply this config file on top of the
-  /// parent one. IF true and using a ConfigOptionsProvider, it will apply this
-  /// config on top of any configuation file it finds in the directory using the
-  /// same logic as FileOptionsProvider. If false or missing, only this
-  /// configuration file will be used.
-  llvm::Optional<bool> InheritParentConfig;
-
   /// Use colors in diagnostics. If missing, it will be auto detected.
   llvm::Optional<bool> UseColor;
 };
 
+struct ClangTidyInheritableOptions : public ClangTidyOptions {
+  bool Inherit = false;
+};
+
 /// Abstract interface for retrieving various ClangTidy options.
 class ClangTidyOptionsProvider {
 public:
   static const char OptionsSourceTypeDefaultBinary[];
   static const char OptionsSourceTypeCheckCommandLineOption[];
   static const char OptionsSourceTypeConfigCommandLineOption[];
 
   virtual ~ClangTidyOptionsProvider() {}
private:
   ClangTidyGlobalOptions GlobalOptions;
   ClangTidyOptions DefaultOptions;
 };
 
 class FileOptionsBaseProvider : public DefaultOptionsProvider {
 public:
   // 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)>>
+  typedef std::pair<std::string,
+                    std::function<llvm::ErrorOr<ClangTidyInheritableOptions>(
       ConfigFileHandler;
 
+  using InheritableOptionsSource =
+      std::pair<ClangTidyInheritableOptions, std::string>;
+
   /// Configuration file handlers listed in the order of priority.
   ///
   /// Custom configuration file formats can be supported by constructing the
   /// list of handlers and passing it to the appropriate \c FileOptionsProvider
   /// constructor. E.g. initialization of a \c FileOptionsProvider with support
   /// of a custom configuration file format for files named ".my-tidy-config"
   /// could look similar to this:
   /// \code
FileOptionsBaseProvider(const ClangTidyGlobalOptions &GlobalOptions,
                           const ConfigFileHandlers &ConfigHandlers);
 
 protected:
   void addRawFileOptions(llvm::StringRef AbsolutePath,
                          std::vector<OptionsSource> &CurOptions);
 
   /// Try to read configuration files from \p Directory using registered
   /// \c ConfigHandlers.
-  llvm::Optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
+  llvm::Optional<InheritableOptionsSource>
+  tryReadConfigFile(llvm::StringRef Directory);
 
-  llvm::StringMap<OptionsSource> CachedOptions;
+  llvm::StringMap<InheritableOptionsSource> CachedOptions;
   ClangTidyOptions OverrideOptions;
   ConfigFileHandlers ConfigHandlers;
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
 };
 
 /// Implementation of ClangTidyOptions interface, which is used for
 /// '-config' command-line option.
 class ConfigOptionsProvider : public FileOptionsBaseProvider {
 public:
   ConfigOptionsProvider(
       const ClangTidyGlobalOptions &GlobalOptions,
       const ClangTidyOptions &DefaultOptions,
-      const ClangTidyOptions &ConfigOptions,
+      const ClangTidyInheritableOptions &ConfigOptions,
       const ClangTidyOptions &OverrideOptions,
       llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
   std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
 
 private:
-  ClangTidyOptions ConfigOptions;
+  ClangTidyInheritableOptions ConfigOptions;
 };
 
 /// 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
public:
 
   std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
 };
 
 /// Parses LineFilter from JSON and stores it to the \p Options.
 std::error_code parseLineFilter(llvm::StringRef LineFilter,
                                 ClangTidyGlobalOptions &Options);
 
-/// Parses configuration from JSON and returns \c ClangTidyOptions or an
-/// error.
-llvm::ErrorOr<ClangTidyOptions> parseConfiguration(llvm::StringRef Config);
+/// Parses configuration from JSON and returns \c ClangTidyOptions and a bool
+/// indicating if we should inherit from a parent config or an error.
+llvm::ErrorOr<ClangTidyInheritableOptions>
+parseConfiguration(llvm::StringRef Config);
 
 /// Serializes configuration to a YAML-encoded string.
 std::string configurationAsText(const ClangTidyOptions &Options);
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

static void mapping(IO &IO, ClangTidyOptions &Options) {
     IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // legacy compatibility
     IO.mapOptional("FormatStyle", Options.FormatStyle);
     IO.mapOptional("User", Options.User);
     IO.mapOptional("CheckOptions", NOpts->Options);
     IO.mapOptional("ExtraArgs", Options.ExtraArgs);
     IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
-    IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
     IO.mapOptional("UseColor", Options.UseColor);
   }
 };
 
+template <> struct MappingTraits<clang::tidy::ClangTidyInheritableOptions> {
+  static void mapping(IO &IO, clang::tidy::ClangTidyInheritableOptions &Opts) {
+    MappingTraits<ClangTidyOptions>::mapping(IO, Opts);
+    IO.mapOptional("InheritParentConfig", Opts.Inherit, false);
+  }
+};
+
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
 
 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector<OptionsSource> Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
   return Result;
 }
 
 ConfigOptionsProvider::ConfigOptionsProvider(
     const ClangTidyGlobalOptions &GlobalOptions,
     const ClangTidyOptions &DefaultOptions,
-    const ClangTidyOptions &ConfigOptions,
+    const ClangTidyInheritableOptions &ConfigOptions,
     const ClangTidyOptions &OverrideOptions,
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
     : FileOptionsBaseProvider(GlobalOptions, DefaultOptions, OverrideOptions,
                               FS),
       ConfigOptions(ConfigOptions) {}
 
 std::vector<OptionsSource>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector<OptionsSource> RawOptions =
       DefaultOptionsProvider::getRawOptions(FileName);
-  if (ConfigOptions.InheritParentConfig.getValueOr(false)) {
+  if (ConfigOptions.Inherit) {
     LLVM_DEBUG(llvm::dbgs()
                << "Getting options for file " << FileName << "...\n");
     assert(FS && "FS must be set.");
 
     llvm::SmallString<128> AbsoluteFilePath(FileName);
 
     if (!FS->makeAbsolute(AbsoluteFilePath)) {
       addRawFileOptions(AbsoluteFilePath, RawOptions);
void FileOptionsBaseProvider::addRawFileOptions(
     llvm::StringRef AbsolutePath, std::vector<OptionsSource> &CurOptions) {
   auto CurSize = CurOptions.size();
 
   // 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(AbsolutePath);
   for (StringRef CurrentPath = Path; !CurrentPath.empty();
        CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
-    llvm::Optional<OptionsSource> Result;
+    llvm::Optional<InheritableOptionsSource> 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) {
         LLVM_DEBUG(llvm::dbgs()
                    << "Caching configuration for path " << Path << ".\n");
         if (!CachedOptions.count(Path))
           CachedOptions[Path] = *Result;
         Path = llvm::sys::path::parent_path(Path);
       }
       CachedOptions[Path] = *Result;
 
-      CurOptions.push_back(*Result);
-      if (!Result->first.InheritParentConfig.getValueOr(false))
+      bool Inherit = Result->first.Inherit;
+
+      CurOptions.push_back(std::move(*Result));
+      if (!Inherit)
         break;
     }
   }
   // Reverse order of file configs because closer configs should have higher
   // priority.
   std::reverse(CurOptions.begin() + CurSize, CurOptions.end());
 }
 
FileOptionsProvider::getRawOptions(StringRef FileName) {
   addRawFileOptions(AbsoluteFilePath, RawOptions);
   OptionsSource CommandLineOptions(OverrideOptions,
                                    OptionsSourceTypeCheckCommandLineOption);
 
   RawOptions.push_back(CommandLineOptions);
   return RawOptions;
 }
 
-llvm::Optional<OptionsSource>
+llvm::Optional<FileOptionsBaseProvider::InheritableOptionsSource>
 FileOptionsBaseProvider::tryReadConfigFile(StringRef Directory) {
   assert(!Directory.empty());
 
   llvm::ErrorOr<llvm::vfs::Status> DirectoryStatus = FS->status(Directory);
 
   if (!DirectoryStatus || !DirectoryStatus->isDirectory()) {
     llvm::errs() << "Error reading configuration from " << Directory
                  << ": directory doesn't exist.\n";
if (std::error_code EC = Text.getError()) {
                    << "\n";
       continue;
     }
 
     // Skip empty files, e.g. files opened for writing via shell output
     // redirection.
     if ((*Text)->getBuffer().empty())
       continue;
-    llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
+    llvm::ErrorOr<ClangTidyInheritableOptions> ParsedOptions =
         ConfigHandler.second((*Text)->getBuffer());
     if (!ParsedOptions) {
       if (ParsedOptions.getError())
         llvm::errs() << "Error parsing " << ConfigFile << ": "
                      << ParsedOptions.getError().message() << "\n";
       continue;
     }
-    return OptionsSource(*ParsedOptions, std::string(ConfigFile));
+    return InheritableOptionsSource(std::move(*ParsedOptions),
+                                    std::string(ConfigFile));
   }
   return llvm::None;
 }
 
 /// Parses -line-filter option and stores it to the \c Options.
 std::error_code parseLineFilter(StringRef LineFilter,
                                 clang::tidy::ClangTidyGlobalOptions &Options) {
   llvm::yaml::Input Input(LineFilter);
   Input >> Options.LineFilter;
   return Input.error();
 }
 
-llvm::ErrorOr<ClangTidyOptions> parseConfiguration(StringRef Config) {
+llvm::ErrorOr<ClangTidyInheritableOptions>
+parseConfiguration(StringRef Config) {
   llvm::yaml::Input Input(Config);
-  ClangTidyOptions Options;
+  ClangTidyInheritableOptions Options;
   Input >> Options;
   if (Input.error())
     return Input.error();
   return Options;
 }
 
 std::string configurationAsText(const ClangTidyOptions &Options) {
   std::string Text;

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

if (SystemHeaders.getNumOccurrences() > 0)
     OverrideOptions.SystemHeaders = SystemHeaders;
   if (FormatStyle.getNumOccurrences() > 0)
     OverrideOptions.FormatStyle = FormatStyle;
   if (UseColor.getNumOccurrences() > 0)
     OverrideOptions.UseColor = UseColor;
 
   auto LoadConfig = [&](StringRef Configuration)
       -> std::unique_ptr<ClangTidyOptionsProvider> {
-    llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
+    llvm::ErrorOr<ClangTidyInheritableOptions> ParsedConfig =
         parseConfiguration(Configuration);
     if (ParsedConfig)
       return std::make_unique<ConfigOptionsProvider>(
           GlobalOptions,
           ClangTidyOptions::getDefaults().merge(DefaultOptions, 0),
           *ParsedConfig, OverrideOptions, std::move(FS));
     llvm::errs() << "Error: invalid configuration specified.\n"
                  << ParsedConfig.getError().message() << "\n";

clang-tools-extra/clangd/TidyProvider.cpp

if (List.empty())
     return;
   if (!Checks || Checks->empty()) {
     Checks.emplace(List);
     return;
   }
   *Checks = llvm::join_items(",", *Checks, List);
 }
 
-static llvm::Optional<tidy::ClangTidyOptions>
+static llvm::Optional<tidy::ClangTidyInheritableOptions>
 tryReadConfigFile(llvm::vfs::FileSystem *FS, llvm::StringRef Directory) {
   assert(!Directory.empty());
   // We guaranteed that child directories of Directory exist, so this assert
   // should hopefully never fail.
   assert(FS->exists(Directory));
 
   llvm::SmallString<128> ConfigFile(Directory);
   llvm::sys::path::append(ConfigFile, ".clang-tidy");
if (std::error_code EC = Text.getError()) {
     elog("Can't read '{0}': {1}", ConfigFile, EC.message());
     return llvm::None;
   }
 
   // Skip empty files, e.g. files opened for writing via shell output
   // redirection.
   if ((*Text)->getBuffer().empty())
     return llvm::None;
-  llvm::ErrorOr<tidy::ClangTidyOptions> ParsedOptions =
+  llvm::ErrorOr<tidy::ClangTidyInheritableOptions> ParsedOptions =
       tidy::parseConfiguration((*Text)->getBuffer());
   if (!ParsedOptions) {
     if (ParsedOptions.getError())
       elog("Error parsing clang-tidy configuration in '{0}': {1}", ConfigFile,
            ParsedOptions.getError().message());
     return llvm::None;
   }
   return std::move(*ParsedOptions);
for (const auto &CheckOption : CurTidyConfig.CheckOptions)
       Opts.CheckOptions.insert_or_assign(CheckOption.getKey(),
                                          tidy::ClangTidyOptions::ClangTidyValue(
                                              CheckOption.getValue(), 10000U));
   };
 }
 
 TidyProvider provideClangTidyFiles(ThreadsafeFS &TFS) {
   return [&TFS](tidy::ClangTidyOptions &Opts, llvm::StringRef Filename) {
-    llvm::SmallVector<tidy::ClangTidyOptions, 4> OptionStack;
+    llvm::SmallVector<tidy::ClangTidyInheritableOptions, 4> OptionStack;
     auto FS(TFS.view(llvm::None));
     llvm::SmallString<256> AbsolutePath(Filename);
 
     assert(llvm::sys::path::is_absolute(AbsolutePath));
 
     llvm::sys::path::remove_dots(AbsolutePath, true);
     llvm::StringRef Directory = llvm::sys::path::parent_path(AbsolutePath);
     {
return [&TFS](tidy::ClangTidyOptions &Opts, llvm::StringRef Filename) {
     for (llvm::StringRef CurrentDirectory = Directory;
          !CurrentDirectory.empty();
          CurrentDirectory = llvm::sys::path::parent_path(CurrentDirectory)) {
       auto ConfigFile = tryReadConfigFile(FS.get(), CurrentDirectory);
       if (!ConfigFile)
         continue;
       OptionStack.push_back(std::move(*ConfigFile));
       // Should we search for a parent config to merge
-      if (!OptionStack.back().InheritParentConfig.getValueOr(false))
+      if (!OptionStack.back().Inherit)
         break;
     }
     unsigned Order = 1U;
     for (auto &Option : llvm::reverse(OptionStack))
       Opts.mergeWith(Option, Order++);
   };
 }