[clang-tidy] Clang tidy is now alias aware

[njames93]

Summary

Clang-tidy has had an on growing issue with blindly adding all checks from a module using the -checks=somemodule-* option. If somemodule alias' another enabled check. the check will get ran twice giving duplicated warning and fix options. This can lead to fixes overlapping preventing them from being applied or worse still applying twice resulting in malformed code. There are other issues with ConfigOptions being different in the 2 different instances so one instance of the check may be running a different set of options to the other.

This patch solves these issue by making clang-tidy only run 1 instance of an aliased check even if it has been turned on with multiple names. Warning messages are appended with the original check name if it has been enabled(via cmd line, or config file). If the original check hasn't been enabled then the name will be the first enabled alias that was registered.

Config options are also handled in a similar fashion, see example

[{ key: MyAliasedName.Option1, value: 0 }.
{ key: OriginalCheckName.Option1, value: 1 }]

If the check original-check-name was turned on, then Option1 will be parsed as being 1, otherwise it ignores OriginalCheckName.Option1 key and falls back to the first enabled alias check that declares Option1

To declare a check as an alias when you register then check, just add the check this alias' to as a second parameter

CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
    "hicpp-braces-around-statements");
// Update to
CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
    "hicpp-braces-around-statements", "readability-braces-around-statements");

I haven't added in all the alias declarations in just yet (If you want me to just ask), nor have I added test cases. However once all the alias declarations are added in, the tests fall nicely in line with the current test infrastructure.
Adding this line to the readability-braces-around-statements checks causes no errors, no multiple fix attempts or warning messages.

// RUN: %check_clang_tidy %s readability-braces-around-statements,hicpp-readability-braces-around-statements %t

Likewise changing it to

// RUN: %check_clang_tidy %s hicpp-readability-braces-around-statements %t

Also runs without a hitch (obviously warning messages are appended with [hicpp-readability-braces-around-statements] instead of [readability-braces-around-statements] as above.

[merge_guards_bot]

Unit tests: pass. 61774 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

should warning be printed with all the enabled checks, or just the primary name?

warning: statement should be inside braces [readability-braces-around-statements]

vs

warning: statement should be inside braces [readability-braces-around-statements, hicpp-braces-around-statements]
warning: statement should be inside braces [readability-braces-around-statements], [hicpp-braces-around-statements]

[lebedev.ri]

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

To be noted, this will make adding of new aliases in non-up-stream modules more involved,
since now the aliases will no longer be confined to the new module, but to the original modules.

Also, there was previously some attempts at this:
https://lists.llvm.org/pipermail/cfe-dev/2017-September/055589.html
D25659
D38171
???

If we proceed as-is, i think we also need one more config switch "CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.

[njames93]

In D72566#1815904, @lebedev.ri wrote:

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

If you're expecting a test to run twice usually something has gone wrong

To be noted, this will make adding of new aliases in non-up-stream modules more involved,
since now the aliases will no longer be confined to the new module, but to the original modules.

That's how it currently is for adding new alias definitions.
When you create an alias definition now you need the implementation files available.
However this approach doesn't need to modify the original module and appears mostly transparent to it.
Its also made in such a way that if you set a check as aliasing to a now existent name, it would just give you another way to invoke said check

If we proceed as-is, i think we also need one more config switch "CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.

That does sound like something I can get behind

There is one major nit I can think of and that is if you create an alias to the wrong check it would overwrite said check. I may look into adjusting that

[lebedev.ri]

In D72566#1815913, @njames93 wrote:

In D72566#1815904, @lebedev.ri wrote:

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

If you're expecting a test to run twice usually something has gone wrong

That is not what i'm talking about.
I'm mainly worried about checks with custom config options:
It is likely that said options were specified only for one check,
not all of it's aliases too. So now the fact whether or not
these options will be used will depend on the registering order of checks.

To be noted, this will make adding of new aliases in non-up-stream modules more involved,
since now the aliases will no longer be confined to the new module, but to the original modules.

That's how it currently is for adding new alias definitions.
When you create an alias definition now you need the implementation files available.
However this approach doesn't need to modify the original module and appears mostly transparent to it.
Its also made in such a way that if you set a check as aliasing to a now existent name, it would just give you another way to invoke said check

If we proceed as-is, i think we also need one more config switch "CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.

That does sound like something I can get behind

There is one major nit I can think of and that is if you create an alias to the wrong check it would overwrite said check. I may look into adjusting that

[aaron.ballman]

In D72566#1815913, @njames93 wrote:

In D72566#1815904, @lebedev.ri wrote:

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

If you're expecting a test to run twice usually something has gone wrong

I'm not certain I agree. Many of the aliases have different sets of default checking options from the primary check. Running the "same" check twice in these cases produces different output. How do you intend to handle situations like that? If the answer is "disable the aliases", then this has the potential to lose valuable coverage for some folks who are asking for all checks to be enabled.

[njames93]

How does this sound?
I'll put in a config option in there to disable the new alias behaviour.
Personally I'd say default to the new behaviour. Then if an organisation
relies on the old behaviour then can revert back with a command line option
or .clang-tidy file edit

[njames93]

Another potential solution is leave is as is by default. Then emit a warning if a check is ran twice. The warning could be suppressed by passing a option to say what behaviour you want?

[lebedev.ri]

I think the fact that this is a fourth (?) different incarnation of a patch
trying to solve the same *major* *ugly* problem, it may be an evidence that
perhaps this problem should not be approached from the 'let's hack it together'
approach, but with a concrete plan sent as an RFC to cfe-dev :/

[aaron.ballman]

In D72566#1816109, @lebedev.ri wrote:

I think the fact that this is a fourth (?) different incarnation of a patch
trying to solve the same *major* *ugly* problem, it may be an evidence that
perhaps this problem should not be approached from the 'let's hack it together'
approach, but with a concrete plan sent as an RFC to cfe-dev :/

+1

This is a pretty big part of the user interface for clang-tidy, so I think we need community buy-in on how to proceed. It's not that we think the current interface is perfect, it's more that there are complexities we need to better understand before deciding on a solution. I'd rather get a big picture idea of the design for aliases, how they interact with the command line, documentation, diagnostics, etc to make sure we're hitting all of the important bits. Also, we should make sure that whatever we do is at least someone comparable with the clang static analyzer (I don't know if they have aliases there or not) so that we don't design two totally different approaches to solving the same problem.

[MartinG]

In D72566#1815917, @aaron.ballman wrote:

I'm not certain I agree. Many of the aliases have different sets of default checking options from the primary check.

And that's precisely the problem. Aliases should be just that: aliases. If two checks check for the same thing but have different options, they should be merged into one.

[carlosgalvezp]

Hi!

I've been following this issue about aliases in clang-tidy through different bug reports and commit attempts. I see that the author abandoned this commit without an explanation. What's the reason? Where can I follow the continuation of this discussion, is there a email list or something?

I believe making sure that clang-tidy only runs once instance of the checks is important, it can dramatically reduce the runtime. This can have a significant impact in CI working with large codebases. It also doesn't make sense that alias checks aren't identical and don't detect the same things. As it has been mentioned above, users should not carry the burden of knowing and maintaining what is aliasing what at any point in time, that's an internal implementation detail of clang-tidy.

[lebedev.ri]

I think this should be as simple as having an opt-in "DisableAllAliaseeChecks" option in .clang-tidy, that force-disables all the checks that are actually aliases and not proper checks.

[carlosgalvezp]

Fully agree. Such an opt-in feature shouldn't break things. I'd be happy to implement it if people think it's a good idea! Would need some hand-holding though, never touched the LLVM codebase before :)

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

 }
 
 std::vector<std::string> ClangTidyASTConsumerFactory::getCheckNames() {
   std::vector<std::string> CheckNames;
   for (const auto &CheckFactory : *CheckFactories) {
     if (Context.isCheckEnabled(CheckFactory.first))
       CheckNames.push_back(CheckFactory.first);
   }
+  for (const auto &AliasCheckFactory : CheckFactories->aliases()) {
+    if (Context.isCheckEnabled(AliasCheckFactory.first))
+      CheckNames.push_back(AliasCheckFactory.first);
+    for (auto StringRefPair =
+             StringRef(AliasCheckFactory.second.first).split(';');
+         !StringRefPair.first.empty();
+         StringRefPair = StringRefPair.second.split(';')) {
+      if (Context.isCheckEnabled(StringRefPair.first))
+        CheckNames.push_back(StringRefPair.first);
+    }
+  }
 
 #if CLANG_ENABLE_STATIC_ANALYZER
   for (const auto &AnalyzerCheck : getAnalyzerCheckersAndPackages(
            Context, Context.canEnableAnalyzerAlphaCheckers()))
     CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 #endif // CLANG_ENABLE_STATIC_ANALYZER
 
   std::sort(CheckNames.begin(), CheckNames.end());

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

void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
                StringRef Value) const;
 
     /// Stores an option with the check-local name \p LocalName with
     /// ``int64_t`` value \p Value to \p Options.
     void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
                int64_t Value) const;
 
   private:
-    std::string NamePrefix;
+    SmallVector<std::string, 2> NamePrefixes;
     const ClangTidyOptions::OptionMap &CheckOptions;
   };
 
 private:
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
   StringRef getID() const override { return CheckName; }
   std::string CheckName;
   ClangTidyContext *Context;

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

 //===--- ClangTidyCheck.cpp - clang-tidy ------------------------*- C++ -*-===//
 //
 // 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 "ClangTidyCheck.h"
+#include "llvm/ADT/SmallVector.h"
 
 namespace clang {
 namespace tidy {
 
 ClangTidyCheck::ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context)
-    : CheckName(CheckName), Context(Context),
+    : CheckName(CheckName.split(';').first), 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) {
   // For historical reasons, checks don't implement the MatchFinder run()
   // callback directly. We keep the run()/check() distinction to avoid interface
   // churn, and to allow us to add cross-cutting logic in the future.
   check(Result);
 }
 
-ClangTidyCheck::OptionsView::OptionsView(StringRef CheckName,
-                         const ClangTidyOptions::OptionMap &CheckOptions)
-    : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions) {}
+ClangTidyCheck::OptionsView::OptionsView(
+    StringRef CheckName, const ClangTidyOptions::OptionMap &CheckOptions)
+    : CheckOptions(CheckOptions) {
+  llvm::SmallVector<StringRef, 4> CheckNames;
+  CheckName.split(CheckNames, ';', -1, false);
+  for (const auto &Name : CheckNames) {
+    NamePrefixes.emplace_back((Name + ".").str());
+  }
+  assert(!NamePrefixes.empty());
+}
 
 std::string ClangTidyCheck::OptionsView::get(StringRef LocalName,
                                              StringRef Default) const {
+  for (const auto &NamePrefix : NamePrefixes) {
     const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
     if (Iter != CheckOptions.end())
       return Iter->second;
+  }
   return Default;
 }
 
 std::string
 ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName,
                                               StringRef Default) const {
+  for (const auto &NamePrefix : NamePrefixes) {
     auto Iter = CheckOptions.find(NamePrefix + LocalName.str());
     if (Iter != CheckOptions.end())
       return Iter->second;
     // Fallback to global setting, if present.
     Iter = CheckOptions.find(LocalName.str());
     if (Iter != CheckOptions.end())
       return Iter->second;
+  }
   return Default;
 }
 
 void ClangTidyCheck::OptionsView::store(ClangTidyOptions::OptionMap &Options,
                                         StringRef LocalName,
                                         StringRef Value) const {
-  Options[NamePrefix + LocalName.str()] = Value;
+  Options[NamePrefixes.front() + LocalName.str()] = Value;
 }
 
 void ClangTidyCheck::OptionsView::store(ClangTidyOptions::OptionMap &Options,
                                         StringRef LocalName,
                                         int64_t Value) const {
   store(Options, LocalName, llvm::itostr(Value));
 }
 
 } // namespace tidy
 } // namespace clang

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

 //===--- ClangTidyModule.h - clang-tidy -------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULE_H
 
 #include "ClangTidy.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
 #include <functional>
 #include <map>
 #include <memory>
 #include <string>
 #include <utility>
 
 namespace clang {
 namespace tidy {
 
 /// A collection of \c ClangTidyCheckFactory instances.
 ///
 /// All clang-tidy modules register their check factories with an instance of
 /// this object.
 class ClangTidyCheckFactories {
 public:
   using CheckFactory = std::function<std::unique_ptr<ClangTidyCheck>(
       StringRef Name, ClangTidyContext *Context)>;
 
   /// Registers check \p Factory with name \p Name.
   ///
   /// For all checks that have default constructors, use \c registerCheck.
   void registerCheckFactory(StringRef Name, CheckFactory Factory);
 
+  /// Registers check \p Factory with name \p Name that is an alias to
+  /// another check called \p AliasToName.
+  ///
+  /// For all checks that have default constructors, use \c registerCheck.
+  void registerCheckFactory(StringRef Name, StringRef AliasToName,
+                            CheckFactory Factory);
+
   /// Registers the \c CheckType with the name \p Name.
   ///
   /// This method should be used for all \c ClangTidyChecks that don't require
   /// constructor parameters.
   ///
   /// For example, if have a clang-tidy check like:
   /// \code
   /// class MyTidyCheck : public ClangTidyCheck {
public:
   /// \endcode
   template <typename CheckType> void registerCheck(StringRef CheckName) {
     registerCheckFactory(CheckName,
                          [](StringRef Name, ClangTidyContext *Context) {
                            return std::make_unique<CheckType>(Name, Context);
                          });
   }
 
+  /// Registers the \c CheckType with the name \p Name that is an alias to
+  /// another check called \p AliasToName.
+  template <typename CheckType>
+  void registerCheck(StringRef CheckName, StringRef AliasToName) {
+    registerCheckFactory(CheckName, AliasToName,
+                         [](StringRef Name, ClangTidyContext *Context) {
+                           return std::make_unique<CheckType>(Name, Context);
+                         });
+  }
+
   /// Create instances of checks that are enabled.
   std::vector<std::unique_ptr<ClangTidyCheck>>
   createChecks(ClangTidyContext *Context);
 
   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(); }
+  typedef std::map<std::string, std::pair<std::string, CheckFactory>>
+      AliasFactoryMap;
+  AliasFactoryMap::const_iterator alias_begin() const {
+    return AliasFactories.begin();
+  }
+  AliasFactoryMap::const_iterator alias_end() const {
+    return AliasFactories.end();
+  }
+  llvm::iterator_range<AliasFactoryMap::const_iterator> aliases() const {
+    return {alias_begin(), alias_end()};
+  }
+  bool empty() const { return Factories.empty() && AliasFactories.empty(); }
 
 private:
   FactoryMap Factories;
+  AliasFactoryMap AliasFactories;
 };
 
 /// A clang-tidy module groups a number of \c ClangTidyChecks and gives
 /// them a prefixed name.
 class ClangTidyModule {
 public:
   virtual ~ClangTidyModule() {}
 

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

 //===--- tools/extra/clang-tidy/ClangTidyModule.cpp - Clang tidy tool -----===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 ///
 ///  \file Implements classes required to build clang-tidy modules.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidyModule.h"
+#include "llvm/Support/raw_ostream.h"
+#include <utility>
 
 namespace clang {
 namespace tidy {
 
 void ClangTidyCheckFactories::registerCheckFactory(StringRef Name,
                                                    CheckFactory Factory) {
+  if (AliasFactories.count(Name))
+    return; // Check already registered as alias
   Factories[Name] = std::move(Factory);
 }
 
+void ClangTidyCheckFactories::registerCheckFactory(StringRef Name,
+                                                   StringRef AliasToName,
+                                                   CheckFactory Factory) {
+  Factories.erase(AliasToName);
+  auto &Current = AliasFactories[AliasToName];
+  if (Current.second) // Check if already declared
+    Current.first.append((";" + Name).str());
+  else
+    Current = std::make_pair(Name, std::move(Factory));
+  assert(AliasFactories.count(AliasToName) != 0);
+}
+
 std::vector<std::unique_ptr<ClangTidyCheck>>
 ClangTidyCheckFactories::createChecks(ClangTidyContext *Context) {
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
   for (const auto &Factory : Factories) {
     if (Context->isCheckEnabled(Factory.first))
       Checks.emplace_back(Factory.second(Factory.first, Context));
   }
+  for (const auto &AliasFactory : AliasFactories) {
+    SmallString<256> Buffer;
+    llvm::raw_svector_ostream Builder(Buffer);
+    if (Context->isCheckEnabled(AliasFactory.first))
+      Builder << AliasFactory.first << ';';
+    SmallVector<StringRef, 4> Split;
+    StringRef AliasNames = AliasFactory.second.first;
+    AliasNames.split(Split, ';', -1, false);
+    for (StringRef Aliases : Split) {
+      if (Context->isCheckEnabled(Aliases))
+        Builder << Aliases << ';';
+    }
+    if (!Buffer.empty()) {
+      Buffer.pop_back(); // Remove the last ';'
+      Checks.emplace_back(AliasFactory.second.second(Buffer, Context));
+    }
+  }
   return Checks;
 }
 
 ClangTidyOptions ClangTidyModule::getModuleOptions() {
   return ClangTidyOptions();
 }
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- Clang tidy is now aware of alias checks and will only run one instance of
+  an alias check that's enabled by different names.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-bad-signal-to-kill-thread
   <clang-tidy/checks/bugprone-bad-signal-to-kill-thread>` check.
 
   Finds ``pthread_kill`` function calls when a thread is terminated by 
   raising ``SIGTERM`` signal.