[clang-tidy] Avoid running aliased checks twice

[malcolm.parsons]

Summary

It is easy to configure clang-tidy with both aliased and original
checks enabled. This causes the check to be appear under both names
in -list-checks and -dump-config, and the check is run twice.

This changeset tells the CheckFactory about aliases so that an alias
is only enabled when the original is not enabled.

[malcolm.parsons]

ping.

[aaron.ballman]

I *really* like this idea, thank you for working on this! A few things:

(0) I'm uncomfortable with the lack of tests in the patch. I'm not certain of a good way to test this, however. @alexfh, do you have ideas?
(1) I think that the aliases and the originals should be listed with -list-checks, because these are names under which the checks may be run (and sometimes the name may even imply different semantics for the check).
(2) I'm not as certain about -dump-config, since I am not really familiar with that option, but I think we want to list the alias and the original under that as well because different check names may have different configuration options.

[malcolm.parsons]

In D25659#584986, @aaron.ballman wrote:

(1) I think that the aliases and the originals should be listed with -list-checks, because these are names under which the checks may be run (and sometimes the name may even imply different semantics for the check).

I've always found it odd that -list-checks only lists the enabled checks.
I think a list of enabled checks shouldn't report aliases that will not be used.

(2) I'm not as certain about -dump-config, since I am not really familiar with that option, but I think we want to list the alias and the original under that as well because different check names may have different configuration options.

It was because of -dump-config that I noticed the checks were running twice.
I had to configure a check in two places to be sure it was configured how I wanted.
I don't think there is a use case for running the same check with different options at the same time.
Dumping options that won't be used isn't good either.

I'd like to remove cert-oop11-cpp's UseCERTSemantics option - see my comment on D12839.

An alternative to clang-tidy preferring the original check would be to complain if both are enabled.

[aaron.ballman]

In D25659#585154, @malcolm.parsons wrote:

In D25659#584986, @aaron.ballman wrote:

(1) I think that the aliases and the originals should be listed with -list-checks, because these are names under which the checks may be run (and sometimes the name may even imply different semantics for the check).

I've always found it odd that -list-checks only lists the enabled checks.
I think a list of enabled checks shouldn't report aliases that will not be used.

Okay, that's a good point, I forgot that -list-checks only lists the enabled checks, not all of them. With that in mind, here's what I would expect to see:

(0) -checks=* -list-checks
(1) -checks=-*, cert-* -list-checks
(2) -checks=*, -cert-* -list checks

In (0), I would expect all checks to be listed, even aliases. Aliases may have semantic distinctions from other checks and the user said "run everything." However, for aliases which have no semantic distinction, there's no point to running the check twice even if the user said to run everything. Leaving aliases in or out results in an unsatisfying situation either way, unless we know whether an alias supplies different option values.
In (1), I would expect only the cert checks to be listed, even if they alias to something outside of the cert module. It would be strange to use those options and see something outside of the cert module listed, even if it had no semantic distinction, just because of cert rules that are an alias to something else.
In (2), I would expect to see every check *except* cert checks listed, regardless of aliasing in or out of the cert module. Similar rationale as (1).

If I understand the output from this patch, only (0) is modified to not list the alias name because aliases are never enabled with -checks=* in this patch, but (1) and (2) do not change behavior?

(2) I'm not as certain about -dump-config, since I am not really familiar with that option, but I think we want to list the alias and the original under that as well because different check names may have different configuration options.

It was because of -dump-config that I noticed the checks were running twice.
I had to configure a check in two places to be sure it was configured how I wanted.
I don't think there is a use case for running the same check with different options at the same time.

Does -dump-config only list enabled checks as well, or does it list the configuration for all checks? If it's all checks, then the alias should be present because it may be configured differently. If it's only enabled checks, then I agree that it should only list what is being used.

Dumping options that won't be used isn't good either.

I'd like to remove cert-oop11-cpp's UseCERTSemantics option - see my comment on D12839.

Removing that option may be possible so long as we still get the correct semantics when running cert-oop11-cpp.

An alternative to clang-tidy preferring the original check would be to complain if both are enabled.

I don't think that preferring the original check is bad so long as we get the semantic options correct; the ability for an alias to be defined as "that check with these options being set this way" is important.

[malcolm.parsons]

In D25659#585234, @aaron.ballman wrote:

In D25659#585154, @malcolm.parsons wrote:

In D25659#584986, @aaron.ballman wrote:

(1) I think that the aliases and the originals should be listed with -list-checks, because these are names under which the checks may be run (and sometimes the name may even imply different semantics for the check).

I've always found it odd that -list-checks only lists the enabled checks.
I think a list of enabled checks shouldn't report aliases that will not be used.

Okay, that's a good point, I forgot that -list-checks only lists the enabled checks, not all of them. With that in mind, here's what I would expect to see:

(0) -checks=* -list-checks
(1) -checks=-*, cert-* -list-checks
(2) -checks=*, -cert-* -list checks

In (0), I would expect all checks to be listed, even aliases. Aliases may have semantic distinctions from other checks and the user said "run everything." However, for aliases which have no semantic distinction, there's no point to running the check twice even if the user said to run everything. Leaving aliases in or out results in an unsatisfying situation either way, unless we know whether an alias supplies different option values.
In (1), I would expect only the cert checks to be listed, even if they alias to something outside of the cert module. It would be strange to use those options and see something outside of the cert module listed, even if it had no semantic distinction, just because of cert rules that are an alias to something else.
In (2), I would expect to see every check *except* cert checks listed, regardless of aliasing in or out of the cert module. Similar rationale as (1).

If I understand the output from this patch, only (0) is modified to not list the alias name because aliases are never enabled with -checks=* in this patch, but (1) and (2) do not change behavior?

0) doesn't list aliases. I'd like a -list-all-checks option.

1. lists all the cert checks, as they aren't aliases for other cert checks.
2. doesn't list any cert checks, as none are enabled.

Does -dump-config only list enabled checks as well, or does it list the configuration for all checks? If it's all checks, then the alias should be present because it may be configured differently. If it's only enabled checks, then I agree that it should only list what is being used.

The config is only dumped for the enabled checks.

I don't think that preferring the original check is bad so long as we get the semantic options correct; the ability for an alias to be defined as "that check with these options being set this way" is important.

That might mean preferring the alias. But there might be more than one.

[alexfh]

I think, silently choosing one of the checks may be confusing and counter-intuitive. Should we just warn in case we see the same check enabled by multiple aliases?

[alexfh]

In D25659#588658, @alexfh wrote:

I think, silently choosing one of the checks may be confusing and counter-intuitive. Should we just warn in case we see the same check enabled by multiple aliases?

And by "aliases" I mean different names under which the check is registered. I'm not sure I see the benefit of introducing a separate concept and a separate API for registering checks under multiple names. This introduces another sort of dynamic binding, which we have to have good reasons for.

[malcolm.parsons]

In D25659#588663, @alexfh wrote:

In D25659#588658, @alexfh wrote:

I think, silently choosing one of the checks may be confusing and counter-intuitive. Should we just warn in case we see the same check enabled by multiple aliases?

And by "aliases" I mean different names under which the check is registered. I'm not sure I see the benefit of introducing a separate concept and a separate API for registering checks under multiple names. This introduces another sort of dynamic binding, which we have to have good reasons for.

Is using typeid() on the constructed checks OK?

[alexfh]

In D25659#589081, @malcolm.parsons wrote:

In D25659#588663, @alexfh wrote:

In D25659#588658, @alexfh wrote:

I think, silently choosing one of the checks may be confusing and counter-intuitive. Should we just warn in case we see the same check enabled by multiple aliases?

And by "aliases" I mean different names under which the check is registered. I'm not sure I see the benefit of introducing a separate concept and a separate API for registering checks under multiple names. This introduces another sort of dynamic binding, which we have to have good reasons for.

Is using typeid() on the constructed checks OK?

I don't think rtti is allowed in LLVM. But we can find a solution, I believe. The important thing is to decide what behavior is desired.

[malcolm.parsons]

In D25659#588658, @alexfh wrote:

I think, silently choosing one of the checks may be confusing and counter-intuitive. Should we just warn in case we see the same check enabled by multiple aliases?

A warning is useful if the checks have different settings, e.g. google-readability-braces-around-statements and readability-braces-around-statements.

But a warning is just noise for other aliases, e.g. cert-dcl54-cpp and misc-new-delete-overloads.

[aaron.ballman]

In D25659#589386, @malcolm.parsons wrote:

In D25659#588658, @alexfh wrote:

I think, silently choosing one of the checks may be confusing and counter-intuitive. Should we just warn in case we see the same check enabled by multiple aliases?

A warning is useful if the checks have different settings, e.g. google-readability-braces-around-statements and readability-braces-around-statements.

But a warning is just noise for other aliases, e.g. cert-dcl54-cpp and misc-new-delete-overloads.

That's why I don't think there's a good default behavior without capturing further information about whether an alias is an identical copy under a different name or a partial copy with differing semantics. If we have that information, then my preference is for warning the user about enabling the (effectively) same check twice.

[malcolm.parsons]

Abandon until we have a plan.

clang-tidy/ClangTidy.cpp

 std::vector<std::string> ClangTidyASTConsumerFactory::getCheckNames() {
   std::vector<std::string> CheckNames;
   GlobList &Filter = Context.getChecksFilter();
   for (const auto &CheckFactory : *CheckFactories) {
     if (Filter.contains(CheckFactory.first))
       CheckNames.push_back(CheckFactory.first);
   }
 
+  for (const auto &Alias : CheckFactories->aliases()) {
+    if (Filter.contains(Alias.first) && !Filter.contains(Alias.second))
+      CheckNames.push_back(Alias.first);
+  }
+
   for (const auto &AnalyzerCheck : getCheckersControlList(Filter))
     CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 
   std::sort(CheckNames.begin(), CheckNames.end());
   return CheckNames;
 }
 
 ClangTidyOptions::OptionMap ClangTidyASTConsumerFactory::getCheckOptions() {

clang-tidy/ClangTidyModule.h

public:
   /// \endcode
   template <typename CheckType> void registerCheck(StringRef CheckName) {
     registerCheckFactory(CheckName,
                          [](StringRef Name, ClangTidyContext *Context) {
       return new CheckType(Name, Context);
     });
   }
 
+  /// Register an alias for another check
+  void registerAlias(StringRef AliasName, StringRef CheckName) {
+    Aliases[AliasName] = CheckName;
+  }
+
   /// \brief Create instances of all checks matching \p CheckRegexString and
   /// store them in \p Checks.
   ///
   /// The caller takes ownership of the return \c ClangTidyChecks.
   void createChecks(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(); }
 
+  typedef std::map<std::string, std::string> AliasMap;
+  const AliasMap& aliases() const { return Aliases; }
+  AliasMap::const_iterator aliases_begin() const { return Aliases.begin(); }
+  AliasMap::const_iterator aliases_end() const { return Aliases.end(); }
+  bool aliases_empty() const { return Aliases.empty(); }
+
 private:
   FactoryMap Factories;
+  AliasMap Aliases;
 };
 
 /// \brief A clang-tidy module groups a number of \c ClangTidyChecks and gives
 /// them a prefixed name.
 class ClangTidyModule {
 public:
   virtual ~ClangTidyModule() {}
 

clang-tidy/ClangTidyModule.cpp

 void ClangTidyCheckFactories::createChecks(
     ClangTidyContext *Context,
     std::vector<std::unique_ptr<ClangTidyCheck>> &Checks) {
   GlobList &Filter = Context->getChecksFilter();
   for (const auto &Factory : Factories) {
     if (Filter.contains(Factory.first))
       Checks.emplace_back(Factory.second(Factory.first, Context));
   }
+
+  for (const auto &Alias : Aliases) {
+    assert(Factories.count(Alias.second) && "Aliased check does not exist");
+    if (Filter.contains(Alias.first) && !Filter.contains(Alias.second))
+      Checks.emplace_back(Factories[Alias.second](Alias.first, Context));
+  }
 }
 
 ClangTidyOptions ClangTidyModule::getModuleOptions() {
   return ClangTidyOptions();
 }
 
 } // namespace tidy
 } // namespace clang

clang-tidy/cert/CERTTidyModule.cpp

 //===--- CERTTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "../google/UnnamedNamespaceInHeaderCheck.h"
-#include "../misc/MoveConstructorInitCheck.h"
-#include "../misc/NewDeleteOverloadsCheck.h"
-#include "../misc/NonCopyableObjects.h"
-#include "../misc/StaticAssertCheck.h"
-#include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "CommandProcessorCheck.h"
 #include "FloatLoopCounter.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
 
 class CERTModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // C++ checkers
     // DCL
     CheckFactories.registerCheck<VariadicFunctionDefCheck>(
         "cert-dcl50-cpp");
-    CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
-        "cert-dcl54-cpp");
-    CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
-        "cert-dcl59-cpp");
+    CheckFactories.registerAlias("cert-dcl54-cpp", "misc-new-delete-overloads");
+    CheckFactories.registerAlias("cert-dcl59-cpp", "google-build-namespaces");
     // OOP
-    CheckFactories.registerCheck<misc::MoveConstructorInitCheck>(
-        "cert-oop11-cpp");
+    CheckFactories.registerAlias("cert-oop11-cpp", "misc-move-constructor-init");
     // ERR
     CheckFactories.registerCheck<SetLongJmpCheck>(
         "cert-err52-cpp");
     CheckFactories.registerCheck<StaticObjectExceptionCheck>(
         "cert-err58-cpp");
     CheckFactories.registerCheck<ThrownExceptionTypeCheck>(
         "cert-err60-cpp");
-    CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
-        "cert-err61-cpp");
+    CheckFactories.registerAlias("cert-err61-cpp",
+                                 "misc-throw-by-value-catch-by-reference");
 
     // C checkers
     // DCL
-    CheckFactories.registerCheck<misc::StaticAssertCheck>(
-        "cert-dcl03-c");
+    CheckFactories.registerAlias("cert-dcl03-c", "misc-static-assert");
     // ENV
     CheckFactories.registerCheck<CommandProcessorCheck>(
         "cert-env33-c");
     // FLP
     CheckFactories.registerCheck<FloatLoopCounter>(
         "cert-flp30-c");
     // FIO
-    CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>(
-        "cert-fio38-c");
+    CheckFactories.registerAlias("cert-fio38-c", "misc-non-copyable-objects");
     // ERR
     CheckFactories.registerCheck<StrToNumCheck>(
         "cert-err34-c");
   }
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     Options.CheckOptions["cert-oop11-cpp.UseCERTSemantics"] = "1";
     return Options;

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 #include "SpecialMemberFunctionsCheck.h"
 #include "SlicingCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerAlias(
+        "cppcoreguidelines-c-copy-assignment-signature",
+        "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
     CheckFactories.registerCheck<SpecialMemberFunctionsCheck>(
         "cppcoreguidelines-special-member-functions");
     CheckFactories.registerCheck<SlicingCheck>(
         "cppcoreguidelines-slicing");
-    CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
-        "cppcoreguidelines-c-copy-assignment-signature");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
 
 } // namespace cppcoreguidelines
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CppCoreGuidelinesModule.
 volatile int CppCoreGuidelinesModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "../readability/BracesAroundStatementsCheck.h"
-#include "../readability/FunctionSizeCheck.h"
-#include "../readability/NamespaceCommentCheck.h"
-#include "../readability/RedundantSmartptrGetCheck.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "IntegerTypesCheck.h"
 #include "MemsetZeroLengthCheck.h"
 #include "OverloadedUnaryAndCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<runtime::StringReferenceMemberCheck>(
         "google-runtime-member-string-references");
     CheckFactories.registerCheck<runtime::MemsetZeroLengthCheck>(
         "google-runtime-memset");
     CheckFactories.registerCheck<readability::AvoidCStyleCastsCheck>(
         "google-readability-casting");
     CheckFactories.registerCheck<readability::TodoCommentCheck>(
         "google-readability-todo");
-    CheckFactories
-        .registerCheck<clang::tidy::readability::BracesAroundStatementsCheck>(
-            "google-readability-braces-around-statements");
+    CheckFactories.registerAlias("google-readability-braces-around-statements",
+                                 "readability-braces-around-statements");
     CheckFactories.registerCheck<readability::GlobalNamesInHeadersCheck>(
         "google-global-names-in-headers");
-    CheckFactories.registerCheck<clang::tidy::readability::FunctionSizeCheck>(
-        "google-readability-function-size");
-    CheckFactories
-        .registerCheck<clang::tidy::readability::NamespaceCommentCheck>(
-            "google-readability-namespace-comments");
-    CheckFactories
-        .registerCheck<clang::tidy::readability::RedundantSmartptrGetCheck>(
-            "google-readability-redundant-smartptr-get");
+    CheckFactories.registerAlias("google-readability-function-size",
+                                 "readability-function-size");
+    CheckFactories.registerAlias("google-readability-namespace-comments",
+                                 "llvm-namespace-comment");
+    CheckFactories.registerAlias("google-readability-redundant-smartptr-get",
+                                 "readability-redundant-smartptr-get");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;
     Opts["google-readability-braces-around-statements.ShortStatementLines"] =
         "1";
     Opts["google-readability-function-size.StatementThreshold"] = "800";