[clang-tidy] Add -enable-alpha-checks command

[Prazek]

reformat

[aaron.ballman]

LGTM with a few minor nits, but in the future, please provide some summary of what your patch is going to do rather than leave it entirely blank. It makes it easier on the reviewers if they have an idea of what to expect and why it's needed. ;-)

Also, this should probably be added to the release notes.

clang-tidy/ClangTidyOptions.h

78

from static-analyzer -> from the static analyzer

[alexfh]

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

[alexfh]

clang-tidy/ClangTidy.cpp

296

This is the place where a small local change will enable alpha checkers, if needed. It specifically shouldn't be configurable by users. It might make sense to add a comment to this effect here.

[aaron.ballman]

In D28729#646548, @alexfh wrote:

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly, or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

[alexfh]

In D28729#646555, @aaron.ballman wrote:

In D28729#646548, @alexfh wrote:

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

[Prazek]

So the problem I got is that every time I want to check if there is already a feature in clang-tidy/static-analyzer that solves my issue, I either have to deal with static-analyzer command line, which is horrible, or I have to modify and recompile the source code.

The case I was looking at is a check to find

int *p = g():
*p = 42;
if(!p) {g(): }

I wanted to find null checks after the pointer was dereferenced, and indeed I found it in deadcode or unreachable code analysis in static-analyzer.

I am not sure what you are afraid of - as long as this feature is not turned by default everything should be fine. If someone would turn it on then as long as he doesn't see any false positives then everything is still fine, and it can always disable flag.

AFAIK there is a way to specify in cl::opt that the option is hidden, which could probably solve your concerns?

[aaron.ballman]

In D28729#646560, @alexfh wrote:

In D28729#646555, @aaron.ballman wrote:

In D28729#646548, @alexfh wrote:

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful.

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

Agreed.

[Prazek]

In D28729#646758, @aaron.ballman wrote:

In D28729#646560, @alexfh wrote:

In D28729#646555, @aaron.ballman wrote:

In D28729#646548, @alexfh wrote:

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful.

I don't quite understand. Are you suggesting:

• removing help text
• adding cl::hidden

What website are you talking about?

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

Agreed.

I don't agree that alpha checkers are that bad. When I was testing it year ago there were only few false positives, probably in 2 checkers.

[aaron.ballman]

In D28729#646876, @Prazek wrote:

In D28729#646758, @aaron.ballman wrote:

In D28729#646560, @alexfh wrote:

In D28729#646555, @aaron.ballman wrote:

In D28729#646548, @alexfh wrote:

As discussed with the Static Analyzer maintainers, alpha checkers are completely unsupported and are suitable for very early testing only. We had problems with them routinely, that's why I disabled alpha checkers in clang-tidy completely. I don't think there should be a user-visible way to enable them. Developers can locally change the code to get access to alpha checkers, but released binaries shouldn't provide this possibility.

That's good to know -- should it be documented a bit more explicitly,

Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you have other suggestions?

A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful.

I don't quite understand. Are you suggesting:

• removing help text
• adding cl::hidden

I'm suggesting removing help text.

What website are you talking about?

Things like this: http://clang-analyzer.llvm.org/alpha_checks.html

To me, there's a difference between "prevented from being on by default" and "completely unsupported". If they're completely unsupported, we probably shouldn't expose them more easily, but we may want to clarify this in user-facing documentation and help text.

or perhaps the alpha checks should be removed until they're fit for public consumption? Some of those alpha checks have been in the product for a long time, and if they're so unstable that we cannot expose them in a user-friendly fashion, perhaps they don't belong yet?

As discussed with SA folks, alpha checkers are convenient for them to develop new checks, but shouldn't be exposed to users. Some of these experimental checkers might deserve being moved out of alpha or removed completely, but that should be reported to and discussed with the SA maintainers.

Agreed.

I don't agree that alpha checkers are that bad. When I was testing it year ago there were only few false positives, probably in 2 checkers.

If the maintainers of the static analyzer think they shouldn't be exposed to users, I'm inclined to follow that advice.

[Prazek]

-enable-alpha-checks is now not visible for users, but it make
my life as clang-tidy developer much easier.

[Prazek]

Does solution like this works for you? We don't officially support alpha checkers, but it is much easier to check if something is already implemented in static analyzer easily

[alexfh]

In D28729#647250, @Prazek wrote:

Does solution like this works for you? We don't officially support alpha checkers, but it is much easier to check if something is already implemented in static analyzer easily

Is it the only problem you're trying to solve by this patch? If so, it might be better to add an alias/script to run clang --analyze -Xclang -analyzer-checker=alpha or even use gcc.godbolt.org (e.g. https://godbolt.org/g/msQigz).

My concern is that if there's a user-visible option (undocumented, unlisted, named --no-no-no-please-dont-use-this-option or otherwise discouraged), people will use it (sometimes unintentionally, because, well, they changed their config once to give it a try and then forgot about it for a year), rely on it, complain about the tool, etc. If clang-tidy is used in some sort of a server-side manner (e.g. integrated with code review), misbehaving checkers may cause crashes, and this is problematic, when maintainers have no intention to fix the bugs and well, they explicitly asked not to expose experimental checkers (see http://llvm.org/PR26855, for example).

[Prazek]

Ok, I didn't know that it is this easy to run alpha checks from clang.

clang-tidy/ClangTidy.cpp

for (const auto &Opt : Opts.CheckOptions) {
     if (!OptName.startswith(AnalyzerPrefix))
       continue;
     AnalyzerOptions->Config[OptName.substr(AnalyzerPrefix.size())] = Opt.second;
   }
 }
 
 typedef std::vector<std::pair<std::string, bool>> CheckersList;
 
-static CheckersList getCheckersControlList(GlobList &Filter) {
+static CheckersList getCheckersControlList(GlobList &Filter,
+                                           bool IncludeExperimental) {
   CheckersList List;
 
   const auto &RegisteredCheckers =
-      AnalyzerOptions::getRegisteredCheckers(/*IncludeExperimental=*/false);
+      AnalyzerOptions::getRegisteredCheckers(IncludeExperimental);
   bool AnalyzerChecksEnabled = false;
   for (StringRef CheckName : RegisteredCheckers) {
     std::string ClangTidyCheckName((AnalyzerCheckNamePrefix + CheckName).str());
     AnalyzerChecksEnabled |= Filter.contains(ClangTidyCheckName);
   }
 
   if (!AnalyzerChecksEnabled)
     return List;
ClangTidyASTConsumerFactory::CreateASTConsumer(
 
   AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts();
   // FIXME: Remove this option once clang's cfg-temporary-dtors option defaults
   // to true.
   AnalyzerOptions->Config["cfg-temporary-dtors"] =
       Context.getOptions().AnalyzeTemporaryDtors ? "true" : "false";
 
   GlobList &Filter = Context.getChecksFilter();
-  AnalyzerOptions->CheckersControlList = getCheckersControlList(Filter);
+  AnalyzerOptions->CheckersControlList =
+      getCheckersControlList(Filter, *Context.getOptions().EnableAlphaChecks);
   if (!AnalyzerOptions->CheckersControlList.empty()) {
     setStaticAnalyzerCheckerOpts(Context.getOptions(), AnalyzerOptions);
     AnalyzerOptions->AnalysisStoreOpt = RegionStoreModel;
     AnalyzerOptions->AnalysisDiagOpt = PD_NONE;
     AnalyzerOptions->AnalyzeNestedBlocks = true;
     AnalyzerOptions->eagerlyAssumeBinOpBifurcation = true;
     std::unique_ptr<ento::AnalysisASTConsumer> AnalysisConsumer =
         ento::CreateAnalysisConsumer(Compiler);
     AnalysisConsumer->AddDiagnosticConsumer(
         new AnalyzerDiagnosticConsumer(Context));
     Consumers.push_back(std::move(AnalysisConsumer));
   }
   return llvm::make_unique<ClangTidyASTConsumer>(
       std::move(Consumers), std::move(Finder), std::move(Checks));
 }
 
 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 &AnalyzerCheck : getCheckersControlList(Filter))
+  for (const auto &AnalyzerCheck :
+       getCheckersControlList(Filter, *Context.getOptions().EnableAlphaChecks))
     CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 
   std::sort(CheckNames.begin(), CheckNames.end());
   return CheckNames;
 }
 
 ClangTidyOptions::OptionMap ClangTidyASTConsumerFactory::getCheckOptions() {
   ClangTidyOptions::OptionMap Options;

clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
   llvm::Optional<std::string> HeaderFilterRegex;
 
   /// \brief Output warnings from system headers matching \c HeaderFilterRegex.
   llvm::Optional<bool> SystemHeaders;
 
   /// \brief Turns on temporary destructor-based analysis.
   llvm::Optional<bool> AnalyzeTemporaryDtors;
 
+  /// \brief Turns on experimental alpha checkers from the static analyzer.
+  llvm::Optional<bool> EnableAlphaChecks;
+
   /// \brief Specifies the name or e-mail of the user running clang-tidy.
   ///
   /// This option is used, for example, to place the correct user name in TODO()
   /// comments in the relevant check.
   llvm::Optional<std::string> User;
 
   typedef std::pair<std::string, std::string> StringPair;
   typedef std::map<std::string, std::string> OptionMap;

clang-tidy/ClangTidyOptions.cpp

 template <> struct MappingTraits<ClangTidyOptions> {
   static void mapping(IO &IO, ClangTidyOptions &Options) {
     MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
         IO, Options.CheckOptions);
     IO.mapOptional("Checks", Options.Checks);
     IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
+    IO.mapOptional("EnableAlphaChecks", Options.EnableAlphaChecks);
     IO.mapOptional("User", Options.User);
     IO.mapOptional("CheckOptions", NOpts->Options);
     IO.mapOptional("ExtraArgs", Options.ExtraArgs);
     IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
 
 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
   Options.Checks = "";
   Options.WarningsAsErrors = "";
   Options.HeaderFilterRegex = "";
   Options.SystemHeaders = false;
   Options.AnalyzeTemporaryDtors = false;
+  Options.EnableAlphaChecks = false;
   Options.User = llvm::None;
   for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(),
                                          E = ClangTidyModuleRegistry::end();
        I != E; ++I)
     Options = Options.mergeWith(I->instantiate()->getModuleOptions());
   return Options;
 }
 

 ClangTidyOptions::mergeWith(const ClangTidyOptions &Other) const {
   ClangTidyOptions Result = *this;
 
   mergeCommaSeparatedLists(Result.Checks, Other.Checks);
   mergeCommaSeparatedLists(Result.WarningsAsErrors, Other.WarningsAsErrors);
   overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex);
   overrideValue(Result.SystemHeaders, Other.SystemHeaders);
   overrideValue(Result.AnalyzeTemporaryDtors, Other.AnalyzeTemporaryDtors);
+  overrideValue(Result.EnableAlphaChecks, Other.EnableAlphaChecks);
   overrideValue(Result.User, Other.User);
   mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
   mergeVectors(Result.ExtraArgsBefore, Other.ExtraArgsBefore);
 
   for (const auto &KeyValue : Other.CheckOptions)
     Result.CheckOptions[KeyValue.first] = KeyValue.second;
 
   return Result;

clang-tidy/tool/ClangTidyMain.cpp

 Enable temporary destructor-aware analysis in
 clang-analyzer- checks.
 This option overrides the value read from a
 .clang-tidy file.
 )"),
                                            cl::init(false),
                                            cl::cat(ClangTidyCategory));
 
+/// This option Enables alpha checkers from the static analyzer, that are
+/// experimental. This option is set to false and not visible in help, because
+/// it is highly not recommended for users.
+static cl::opt<bool> EnableAlphaChecks("enable-alpha-checks",
+                                       cl::init(false), cl::Hidden,
+                                       cl::cat(ClangTidyCategory));
+
 static cl::opt<std::string> ExportFixes("export-fixes", cl::desc(R"(
 YAML file to store suggested fixes in. The
 stored fixes can be applied to the input source
 code with clang-apply-replacements.
 )"),
                                         cl::value_desc("filename"),
                                         cl::cat(ClangTidyCategory));
 
static std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider() {
   }
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
+  DefaultOptions.EnableAlphaChecks = EnableAlphaChecks;
   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)
     OverrideOptions.Checks = Checks;

test/clang-tidy/enable-alpha-checks.cpp

+// Check if '-enable-alpha-checks' is visible for users
+// RUN: clang-tidy -help | not grep 'enable-alpha-checks'
+
+// Check if '-enable-alpha-checks' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -enable-alpha-checks | grep 'clang-analyzer-alpha'