[clang-tidy] Add a new check group 'experimental-'

[whisperity]

Summary

As discussed in D69560 with @aaron.ballman, sometimes checks might not approximate the rule they target the best due to a variety of reasons. This new group should house checks that are working (i.e. no crashes and stupid outputs) but might not yet exactly match the rule we are trying to "sell them" as.

[njames93]

I'm not sold on this being necessary as there is nothing against simply putting the check in the right module to begin with, then changing the registerCheck call to experimenal-<name> as well as the docs.
Could even do some fancy trickery with the python scripts to add support in there for it.
python add_new_check.py <module> <name> --experimental
python rename_check.py --no-experimental <check_name> // Removes a check from experimental when ready for gen use

[aaron.ballman]

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own. Also, if we want to allow experimental checks, I think we should have some rough community consensus on 1) what are the criteria for something being experimental and included in clang-tidy (as opposed to in an out-of-tree repo) and what are the maintenance expectations for those checks, 2) what is the criteria for moving something *out* of experimental mode, and 3) how long should something be experimental without improvement before it's removed from clang-tidy?

I'm not opposed to the idea of experimental checks, but I don't want to see "experimental" become a dumping ground for low-quality checks that we then have to maintain indefinitely.

[whisperity]

In D76545#1935603, @aaron.ballman wrote:

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own.

For this, wouldn't the fact that telling Tidy to enable cppcoreguidelines-* (i.e. --checks='-*,cppcoreguidelines-*') also enables the cppcoreguidelines-experimental-* cause a problem? (This is the original reason why I tinkered in the "top-level" experimental- mode.)

In D76545#1935603, @aaron.ballman wrote:

I'm not opposed to the idea of experimental checks, but I don't want to see "experimental" become a dumping ground for low-quality checks that we then have to maintain indefinitely.

I think this could (in terms of development, support, etc.) work similarly to the alpha. group/package of Clang Static Analyser checkers. I'm not too knowledgeable about CSA - perhaps @Szelethus could help me out, - but their idea of alpha checkers is that they are mature in terms of the general idea, but clunky in terms of working. (The official stance is that anything out of alpha should not crash, anything within alpha might crash, and if you enable it and you crash, you are your own perpetrator.)

[whisperity]

@aaron.ballman @njames93 Should I write up a pitch mail to cfe-dev about this?

[aaron.ballman]

In D76545#1936389, @whisperity wrote:

In D76545#1935603, @aaron.ballman wrote:

I think we want to keep the experimental checks grouped to their parent module rather than being in a module of their own.

For this, wouldn't the fact that telling Tidy to enable cppcoreguidelines-* (i.e. --checks='-*,cppcoreguidelines-*') also enables the cppcoreguidelines-experimental-* cause a problem? (This is the original reason why I tinkered in the "top-level" experimental- mode.)

It will by default, but it doesn't have to. For instance, we could add an --experimental flag to clang-tidy that automatically enables the <module>-experimental-* checks in any otherwise-enabled module for the run.

In D76545#1935603, @aaron.ballman wrote:

I'm not opposed to the idea of experimental checks, but I don't want to see "experimental" become a dumping ground for low-quality checks that we then have to maintain indefinitely.

I think this could (in terms of development, support, etc.) work similarly to the alpha. group/package of Clang Static Analyser checkers. I'm not too knowledgeable about CSA - perhaps @Szelethus could help me out, - but their idea of alpha checkers is that they are mature in terms of the general idea, but clunky in terms of working. (The official stance is that anything out of alpha should not crash, anything within alpha might crash, and if you enable it and you crash, you are your own perpetrator.)

The alpha checks from CSA was the situation I was hoping to avoid -- my experience has been that checks go into there and rarely come out of alpha stage but continue to require periodic maintenance.

@aaron.ballman @njames93 Should I write up a pitch mail to cfe-dev about this?

I think that would be a reasonable next step.

[whisperity]

Obsolete. There was no community consensus, or even a large enough discussion about this. What's more, the only dependee of this patch has superseded in ellegance, dropping the need for this group.

clang-tools-extra/clang-tidy/CMakeLists.txt

 # If you add a check, also add it to ClangTidyForceLinker.h in this directory.
 add_subdirectory(android)
 add_subdirectory(abseil)
 add_subdirectory(boost)
 add_subdirectory(bugprone)
 add_subdirectory(cert)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(darwin)
+add_subdirectory(experimental)
 add_subdirectory(fuchsia)
 add_subdirectory(google)
 add_subdirectory(hicpp)
 add_subdirectory(linuxkernel)
 add_subdirectory(llvm)
 add_subdirectory(llvmlibc)
 add_subdirectory(misc)
 add_subdirectory(modernize)

 set(ALL_CLANG_TIDY_CHECKS
   clangTidyAndroidModule
   clangTidyAbseilModule
   clangTidyBoostModule
   clangTidyBugproneModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyDarwinModule
+  clangTidyExperimentalModule
   clangTidyFuchsiaModule
   clangTidyGoogleModule
   clangTidyHICPPModule
   clangTidyLinuxKernelModule
   clangTidyLLVMModule
   clangTidyLLVMLibcModule
   clangTidyMiscModule
   clangTidyModernizeModule

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

 static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
     CppCoreGuidelinesModuleAnchorSource;
 
 // This anchor is used to force the linker to link the DarwinModule.
 extern volatile int DarwinModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED DarwinModuleAnchorDestination =
     DarwinModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ExperimentalModule.
+extern volatile int ExperimentalModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ExperimentalModuleAnchorDestination =
+    ExperimentalModuleAnchorSource;
+
 // This anchor is used to force the linker to link the FuchsiaModule.
 extern volatile int FuchsiaModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED FuchsiaModuleAnchorDestination =
     FuchsiaModuleAnchorSource;
 
 // This anchor is used to force the linker to link the GoogleModule.
 extern volatile int GoogleModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED GoogleModuleAnchorDestination =

clang-tools-extra/clang-tidy/experimental/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyExperimentalModule
+  ExperimentalTidyModule.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  clangTooling
+  )

clang-tools-extra/clang-tidy/experimental/ExperimentalTidyModule.cpp

+//===-- ExperimentalTidyModule.cpp - clang-tidy ---------------------------===//
+//
+// 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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+
+namespace clang {
+namespace tidy {
+namespace experimental {
+
+/// A module containing checks that are experimental versions of future checks
+/// that belong to other modules.
+class ExperimentalModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+
+  }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+
+    return Options;
+  }
+};
+
+// Register the LLVMTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<ExperimentalModule>
+    X("experimental-module",
+      "Adds checks for that are experimental versions of future checks.");
+
+} // namespace experimental
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the ExperimentalModule.
+volatile int ExperimentalModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/clang-tidy/index.rst

 ``abseil-``            Checks related to Abseil library.
 ``android-``           Checks related to Android.
 ``boost-``             Checks related to Boost library.
 ``bugprone-``          Checks that target bugprone code constructs.
 ``cert-``              Checks related to CERT Secure Coding Guidelines.
 ``clang-analyzer-``    Clang Static Analyzer checks.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``darwin-``            Checks related to Darwin coding conventions.
+``experimental-``      Experimental checks that will eventually move to other
+                       groups.
 ``fuchsia-``           Checks related to Fuchsia coding conventions.
 ``google-``            Checks related to Google coding conventions.
 ``hicpp-``             Checks related to High Integrity C++ Coding Standard.
 ``linuxkernel-``       Checks related to the Linux Kernel coding conventions.
 ``llvm-``              Checks related to the LLVM coding conventions.
 ``llvmlibc-``          Checks related to the LLVM-libc coding standards.
 ``misc-``              Checks that we didn't have a better category for.
 ``modernize-``         Checks that advocate usage of modern (currently "modern"