[clang-tidy] Add a flag to enable alpha checkers

[pfultz2]

Summary

The alpha checkers can already be enabled using the clang driver, this allows them to be enabled using the clang-tidy as well. This can make it easier to test the alpha checkers with projects which already support the compile_commands.json. It will also allow more people to give feedback and patches about the alpha checkers since they can run it as part of clang tidy checks.

[lebedev.ri]

I just feel like pointing out that you can already do that:

$ clang-tidy -checks=clang-analyzer-alpha* -p <....>

(be wary of * expanding)
clang-tidy --help says:

-checks=<string>             - 
                               <...> This option's value is appended to the
                               value of the 'Checks' option in .clang-tidy
                               file, if any.

[lebedev.ri]

Oh, hm.
I just needed something to view the call graph.
I have almost wrote a clang-tidy check using analysis/CallGraph.h, but then i noticed/remembered that clang static analyzer has that already.
But $ clang-tidy -checks=* -list-checks | grep -i analyzer | grep -i debug does not list it.
Similarly $ clang-tidy -checks=* -list-checks | grep -i analyzer | grep -i alpha, contrarily to my expectations also does not list alpha checks...

So while this change is needed,
I think this is the larger problem that should be resolved.

[lebedev.ri]

In D46159#1080939, @lebedev.ri wrote:

Oh, hm.
I just needed something to view the call graph.
I have almost wrote a clang-tidy check using analysis/CallGraph.h, but then i noticed/remembered that clang static analyzer has that already.
But $ clang-tidy -checks=* -list-checks | grep -i analyzer | grep -i debug does not list it.
Similarly $ clang-tidy -checks=* -list-checks | grep -i analyzer | grep -i alpha, contrarily to my expectations also does not list alpha checks...

So while this change is needed,
I think this is the larger problem that should be resolved.

...
And actually looking a bit more at this, i notice

// RUN: clang-tidy -checks=* -list-checks -enable-alpha-checks | grep 'clang-analyzer-alpha'

So ok, i think this is good. (I'll add debug checks ontop of this differential)
Do we want to be more specific and specify that we are talking about /analyzer/ alpha checks?

[lebedev.ri]

So i suspect the history here is: D26310 added a way to filter-out these clang analyzer checks,
which removed these checks from being available in clang-tidy, but did not add an option
(like in this differential) to get them back, so it was a clear regression.

I'm going to accept this because of the reasoning in my previous comments here,
but please wait for @alexfh or someone else to LGTM this too.

[dcoughlin]

I'm curious about the use case for this, since I don't think it ever makes sense to run all the alpha checks at once. These checks are typically a work in progress (they're not finished yet) and often have false positives that haven't been fixed yet. Often they don't handle all facets of the language and can crash on unhandled constructs. While it makes sense to enable a specific alpha check (or even a set of related alpha checks) when developing these checks or when evaluating whether they are ready to graduate into non-alpha it seems to me that running all of them isn't useful, ever.

There is a real cost to this. People often don't know what 'alpha' means and so when they run all the alpha checks they get an incorrect perspective of the quality of the analyzer. They don't realize that the alpha checks are purely used as a mechanism to support in-tree incremental development.

Without a strong rationale for this flag, I would be very opposed it since (as we have seen in the past) it leads to a poor user experience.

[lebedev.ri]

In D46159#1081366, @dcoughlin wrote:

...

Note that it is completely off by default, and is not listed in documentation, clang-tidy --help,
so one would have to know of the existence of that hidden flag first, and only then when that flag is passed, those alpha checks could be enabled.

[dcoughlin]

In D46159#1081371, @lebedev.ri wrote:

Note that it is completely off by default, and is not listed in documentation, clang-tidy --help,
so one would have to know of the existence of that hidden flag first, and only then when that flag is passed, those alpha checks could be enabled.

All it takes is one stack overflow comment or blog post recommending the flag and then we're back where we started. I'm really worried about well-intentioned ("running more checks means higher-quality code, so let's run ALL the checks") people enabling this option and having a bad experience.

Can you explain what the benefit of this flag is? How do you envision it being used?

[lebedev.ri]

In D46159#1081392, @dcoughlin wrote:

In D46159#1081371, @lebedev.ri wrote:

Note that it is completely off by default, and is not listed in documentation, clang-tidy --help,
so one would have to know of the existence of that hidden flag first, and only then when that flag is passed, those alpha checks could be enabled.

All it takes is one stack overflow comment or blog post recommending the flag and then we're back where we started. I'm really worried about well-intentioned ("running more checks means higher-quality code, so let's run ALL the checks") people enabling this option and having a bad experience.

Can you explain what the benefit of this flag is? How do you envision it being used?

What about scan-build proper?
Those alpha checks are always present in all builds, one always can pass -enable-checker alpha.clone.CloneChecker.
I would understand if there was a cmake-time switch to build those, but there isn't one..

Also, it is *much* easier to use clang-tidy rather than clang-analyzer, because you can run clang-tidy on an existing compilation database,
while in scan-build's case, you need whole new build. It's cumbersome.

[NoQ]

clang-tidy/ClangTidy.cpp

373–376 ↗

(On Diff #144245)

Btw this is already enabled by default.

[NoQ]

Am i understanding correctly that the new -enable-alpha-checks flag doesn't enable alpha checks, but it only allows enabling them with a separate command?

Also wanted to mention that developers who are interested in running analyzer alpha checkers over a compilation database also have an option of trying to use scan-build-py which has support for both generating compilation databases from arbitrary build systems and running over existing compilation databases. This tool is also not recommended for actual users due to a number of regressions from the existing scan-build, but it might get the job done when it comes to developing and testing the analyzer itself.

[lebedev.ri]

In D46159#1081559, @NoQ wrote:

Am i understanding correctly that the new -enable-alpha-checks flag doesn't enable alpha checks, but it only allows enabling them with a separate command?

Yes, absolutely! Same with my followup D46188.
Perhaps something like -allow-enabling-alpha-checks would be a better name, but it is kinda long.

Also wanted to mention that developers who are interested in running analyzer alpha checkers over a compilation database also have an option of trying to use scan-build-py which has support for both generating compilation databases from arbitrary build systems and running over existing compilation databases.

Good to know, i think this is the first time i'm hearing of it.

This tool is also not recommended for actual users due to a number of regressions from the existing scan-build, but it might get the job done when it comes to developing and testing the analyzer itself.

[pfultz2]

I'm curious about the use case for this, since I don't think it ever makes sense to run all the alpha checks at once. These checks are typically a work in progress (they're not finished yet) and often have false positives that haven't been fixed yet.

We want to run several of the alpha checks in our codebase, and as we find bugs and FPs we can then contribute patches to fix them. Since most of our repos use compile_commands.json, it easier to do this in clang tidy instead of scan-build or clang driver.

[pfultz2]

Am i understanding correctly that the new -enable-alpha-checks flag doesn't enable alpha checks, but it only allows enabling them with a separate command?

Yes, it enables them to be selected by clang tidy.

[pfultz2]

Do you want the flag to be renamed?

[lebedev.ri]

In D46159#1085400, @pfultz2 wrote:

Do you want the flag to be renamed?

I personally would like a bit better name, but i'm not sure what would be better, so not a requirement.

Do you have some better choices?

[pfultz2]

Do you have some better choices?

I could do -allow-alpha-checks. What do you think?

[dcoughlin]

In D46159#1085548, @pfultz2 wrote:

Do you have some better choices?

I could do -allow-alpha-checks. What do you think?

That seems reasonable to me. Although I like -allow-enabling-alpha-checks better because it is longer and will therefore discourage its use.

Do you think it would be possible to have a mechanism that prevents people from turning on all alpha checks at once? That is what I'm worried about. People enabling individual alpha checks to test them out is fine, but people enabling all of them is not a good experience.

[lebedev.ri]

In D46159#1085684, @dcoughlin wrote:

In D46159#1085548, @pfultz2 wrote:

Do you have some better choices?

I could do -allow-alpha-checks. What do you think?

That seems reasonable to me.

Although I like -allow-enabling-alpha-checks better because it is longer and will therefore discourage its use.

This sounds good to me too. Conveys the idea that it only *allows* to enable them, not actually enables them.

Do you think it would be possible to have a mechanism that prevents people from turning on all alpha checks at once? That is what I'm worried about. People enabling individual alpha checks to test them out is fine, but people enabling all of them is not a good experience.

While in this particular case it may make sense, it also may open the road for restricting/removing -checks=* support, which would be quite unfortunate :/

[alexfh]

As Devin says (and as we discussed this with Anna Zaks) alpha checkers are still in development, so we don't want to expose them to the users, even very curious ones. For those who want to help with development of the alpha checkers, there's always a possibility to change one boolean literal in clang-tidy sources and build their own clang-tidy binary. Or one can use clang -analyze. Are there any use cases not covered by these two options?

clang-tidy/ClangTidy.cpp

373–376 ↗

(On Diff #144245)

Should we kill the flag completely?

[lebedev.ri]

In D46159#1086322, @alexfh wrote:

How about -enable-alpha-checks=yes-i-know-they-are-broken ?

[alexfh]

clang-tidy/ClangTidy.cpp

373–376 ↗

(On Diff #144245)

I've removed the clang-tidy configuration option in r331456.

[pfultz2]

As Devin says (and as we discussed this with Anna Zaks) alpha checkers are still in development, so we don't want to expose them to the users, even very curious ones.

Then why do we make them available with clang --analyze? If the plan is not to expose them to the users at all, they should be removed from the codebase, as they are just sitting there bit-rotting.

Ideally, I see no problem exposing them to users. This will allow more users to run them on their codebases and submit issues or patches for the problems they find.

[pfultz2]

clang-tidy/ClangTidy.cpp

373–376 ↗

(On Diff #144245)

I didnt modify this line of code. Are you just wanting me to rebase?

[alexfh]

In D46159#1086332, @lebedev.ri wrote:

In D46159#1086322, @alexfh wrote:

How about -enable-alpha-checks=yes-i-know-they-are-broken ?

A hidden flag with a scary name without any way to specify it in .clang-tidy configuration file would be the closest thing to consider.

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

[lebedev.ri]

In D46159#1086463, @pfultz2 wrote:

As Devin says (and as we discussed this with Anna Zaks) alpha checkers are still in development, so we don't want to expose them to the users, even very curious ones.

Then why do we make them available with clang --analyze? If the plan is not to expose them to the users at all, they should be removed from the codebase, as they are just sitting there bit-rotting.

Ideally, I see no problem exposing them to users. This will allow more users to run them on their codebases and submit issues or patches for the problems they find.

Just so i'm perfectly clear, i find this "they are not for common user, so let's not expose them at all" approach very Gnome-like, and not really appropriate for LLVM.
With that approach, there should also not be a https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fexperimental-new-pass-manager and maybe some other flags.

[pfultz2]

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

Because I would like to direct users to try an alpha check on their local codebases without having to tell them to rebuild clang.

[alexfh]

In D46159#1086479, @pfultz2 wrote:

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

Because I would like to direct users to try an alpha check on their local codebases without having to tell them to rebuild clang.

Okay, let's go with a hidden flag (something explicit enough, e.g. -allow-enabling-static-analyzer-alpha-checkers) and remove the clang-tidy configuration option (so that there's no way it can silently sit in the options file, and instead the flag has to be used in each clang-tidy invocation). IIUC, Devin was not completely opposed to this sort of a solution?

clang-tidy/ClangTidy.cpp

373–376 ↗

(On Diff #144245)

I was just answering NoQ's comment. Sorry for the off-topic.

[alexfh]

In D46159#1086487, @alexfh wrote:

In D46159#1086479, @pfultz2 wrote:

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

Because I would like to direct users to try an alpha check on their local codebases without having to tell them to rebuild clang.

Okay, let's go with a hidden flag (something explicit enough, e.g. -allow-enabling-static-analyzer-alpha-checkers)

Or similar to what Roman suggested: -allow-enabling-static-analyzer-alpha-checkers-yes-i-know-they-can-be-broken ;)

[aaron.ballman]

In D46159#1086472, @lebedev.ri wrote:

In D46159#1086463, @pfultz2 wrote:

As Devin says (and as we discussed this with Anna Zaks) alpha checkers are still in development, so we don't want to expose them to the users, even very curious ones.

Then why do we make them available with clang --analyze? If the plan is not to expose them to the users at all, they should be removed from the codebase, as they are just sitting there bit-rotting.

Ideally, I see no problem exposing them to users. This will allow more users to run them on their codebases and submit issues or patches for the problems they find.

Just so i'm perfectly clear, i find this "they are not for common user, so let's not expose them at all" approach very Gnome-like, and not really appropriate for LLVM.

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all. Making it easier to expose does not seem like it serves users because those users expect exposed features to work. Yes, they're already exposed via the static analyzer, but I think that's what should be discussed -- some of these alpha checks have been there in an alpha state for years and I don't think that's a good design for a production tool. I'd rather discuss either improving those to make them production quality or removing them entirely, not making it *easier* to access them.

Making the flag sound scary doesn't suffice -- many users never see the flags because they're hidden away in a build script, but they definitely see the diagnostics and file bug reports.

[xbolva00]

clang-tidy/tool/ClangTidyMain.cpp

195 ↗

(On Diff #144245)

This option enables...

[pfultz2]

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all.

Then why was it merged into clang in the first place? It seems like the whole point of merging it into clang is to get user feedback.

Furthermore, I am trying to update the conversion checker to report more errors, and I would like it to be exposed to users so I can find or fix the FPs.

[xbolva00]

In D46159#1086553, @pfultz2 wrote:

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all.

Then why was it merged into clang in the first place? It seems like the whole point of merging it into clang is to get user feedback.

Furthermore, I am trying to update the conversion checker to report more errors, and I would like it to be exposed to users so I can find or fix the FPs.

If users cannot find / use it, you will not get any feedback. How can they be better checkers if no feedback received?

If nobody should use it, remove this "dead" code

[pfultz2]

Rename flag to allow-enabling-alpha-checks and removed the option from the clang-tidy file.

[aaron.ballman]

In D46159#1086553, @pfultz2 wrote:

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all.

Then why was it merged into clang in the first place? It seems like the whole point of merging it into clang is to get user feedback.

When something is merged into Clang trunk, the expectation is that it will be production quality or will be worked on rapidly to get it to production quality, which is somewhat orthogonal to getting user feedback. I don't know that I have the full history of all of the alpha checks so my generalization may be inaccurate, but it seems like some of these checks were accepted as a WIP and the "progress" stopped for a long time with no one volunteering to remove the features from the analyzer.

Furthermore, I am trying to update the conversion checker to report more errors, and I would like it to be exposed to users so I can find or fix the FPs.

That does not require clang-tidy to surface the alpha checks, correct?

[pfultz2]

When something is merged into Clang trunk, the expectation is that it will be production quality or will be worked on rapidly to get it to production quality, which is somewhat orthogonal to getting user feedback. I don't know that I have the full history of all of the alpha checks so my generalization may be inaccurate, but it seems like some of these checks were accepted as a WIP and the "progress" stopped for a long time with no one volunteering to remove the features from the analyzer.

Some checkers work better than others, while some just need some simple fixes. Of course, no one will volunteer to fix or remove them if they aren't available to run.

That does not require clang-tidy to surface the alpha checks, correct?

A good portion of users are using clang-tidy to run the static analyzer, and I would like it to be exposed to them.

[lebedev.ri]

clang-tidy/tool/ClangTidyMain.cpp

195 ↗

(On Diff #144245)

This option enables...

The current version is correct.

[xbolva00]

clang-tidy/tool/ClangTidyMain.cpp

195 ↗

(On Diff #144245)

There was "This option Enables.." when I commented it.

[alexfh]

In D46159#1086493, @aaron.ballman wrote:

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all. Making it easier to expose does not seem like it serves users because those users expect exposed features to work.

That was also the sentiment static analyzer folks were voicing at some point. I also sympathize to the idea of testing checks and contributing fixes to them, but what the CSA maintainers seem to dislike is a stream of bugs for alpha checkers from users expecting of a certain level of support. So it's basically their decision whether they want to expose alpha checkers via clang frontend and/or via clang-tidy. I can only say whether I like the specific way it is done in clang-tidy.

Making the flag sound scary doesn't suffice -- many users never see the flags because they're hidden away in a build script, but they definitely see the diagnostics and file bug reports.

"We've fixed the glitch" by making everyone wanting a bugzilla account send an email to a human. So only the users who pass this sort of a Turing test will file bugs ;)

clang-tidy/ClangTidyOptions.h

80–82 ↗

(On Diff #145029)

Since this will only be configurable via a flag, this option will be global (i.e. not depend on the location of the file being analyzed). I'd suggest to remove this option altogether and use something else to pass it to ClangTidyASTConsumerFactory. It could be stashed into ClangTidyASTConsumerFactory and passed as a parameter of runClangTidy, or it could live in ClangTidyContext.

clang-tidy/tool/ClangTidyMain.cpp

198 ↗

(On Diff #145029)

s/checks/checkers/ (in the static analyzer's terminology)

I would also make it more explicit that these are static analyzer checkers. -allow-enabling-clang-static-analyzer-alpha-checkers or something like that.

The variable name could be AllowEnablingAnalyzerAlphaCheckers, for example.

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

2 ↗

(On Diff #145029)

grep 'allow-enabling-alpha-checks'

[aaron.ballman]

In D46159#1086627, @alexfh wrote:

In D46159#1086493, @aaron.ballman wrote:

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all. Making it easier to expose does not seem like it serves users because those users expect exposed features to work.

That was also the sentiment static analyzer folks were voicing at some point. I also sympathize to the idea of testing checks and contributing fixes to them, but what the CSA maintainers seem to dislike is a stream of bugs for alpha checkers from users expecting of a certain level of support. So it's basically their decision whether they want to expose alpha checkers via clang frontend and/or via clang-tidy. I can only say whether I like the specific way it is done in clang-tidy.

If the static analyzer people desire this feature, that would sway my position on it, but it sounds like they're hesitant as well. However, I don't think clang-tidy is beholden either -- if we don't think this functionality should be exposed and can justify that position, that should carry weight as well. From a policy perspective, I would be fine with a policy for clang-tidy where we never expose an alpha checker from the static analyzer (or only expose checks on a case by case basis) because I don't mind users having to jump through hoops to get to experimental, unsupported functionality.

As for the way this is surfaced in clang-tidy, I'm also not keen on it but I don't have an improved suggestion to make yet. I primarily don't like the fact that, as a user, I enable checks by name but for some kinds of checks I have to *also* enable them via a secondary mechanism otherwise the name doesn't even exist. This strikes me as being a likely source of confusion where forgetting one flag causes behavioral differences the user doesn't expect.

Making the flag sound scary doesn't suffice -- many users never see the flags because they're hidden away in a build script, but they definitely see the diagnostics and file bug reports.

"We've fixed the glitch" by making everyone wanting a bugzilla account send an email to a human. So only the users who pass this sort of a Turing test will file bugs ;)

Which is an even worse user experience.

[pfultz2]

clang-tidy/ClangTidyOptions.h

80–82 ↗

(On Diff #145029)

But it needs to be passed to getCheckNames as well, which doesn't take a ClangTidyContext.

[pfultz2]

Rename flag to AllowEnablingAnalyzerAlphaCheckers.

[pfultz2]

There doesn't seem to be a simple way to remove it from the ClangTidyOptions class, as a lot more functions need to be changed to support that. I would prefer to leave it there for now, and we can refactor it later. Either way, the check can't be set from a config file.

[alexfh]

In D46159#1086732, @aaron.ballman wrote:

If the static analyzer people desire this feature, that would sway my position on it, but it sounds like they're hesitant as well.
However, I don't think clang-tidy is beholden either -- if we don't think this functionality should be exposed and can justify that position, that should carry weight as well. \
From a policy perspective, I would be fine with a policy for clang-tidy where we never expose an alpha checker from the static analyzer (or only expose checks on a case by case basis) because I don't mind users having to jump through hoops to get to experimental, unsupported functionality.

As folks noted here, some users prefer to use clang-tidy as a frontend for the static analyzer. If this helps them test experimental CSA features and CSA maintainers are willing to accept bug reports and potentially patches from this category of users, I don't want to create obstacles, as long as all experimental features need to be explicitly enabled.

Devin, what's your take on this?

I primarily don't like the fact that, as a user, I enable checks by name but for some kinds of checks I have to *also* enable them via a secondary mechanism otherwise the name doesn't even exist. This strikes me as being a likely source of confusion where forgetting one flag causes behavioral differences the user doesn't expect.

This particular aspect doesn't seem problematic to me. In order to observe these behavioral differences, the user would have to specify this flag at least once, get the results of experimental checkers and see the disclaimer, if we decide to add one (see below). I guess, forgetting about the existence of this flag would be quite unlikely. Forgetting the spelling of the flag should not cause any confusion - it can be looked up in the source code, found out using clang-tidy -help-hidden or asked about.

Making the flag sound scary doesn't suffice -- many users never see the flags because they're hidden away in a build script, but they definitely see the diagnostics and file bug reports.

As an additional way to ensure that this flag doesn't get specified by mistake / doesn't get buried and forgotten inside a script, clang-tidy can print a disclaimer each time it's executed with this flag.

clang-tidy/ClangTidyOptions.h

80–82 ↗

(On Diff #145029)

We can add a boolean parameter to getCheckNames. ClangTidyASTConsumerFactory could get this as a constructor parameter or from ClangTidyContext.

[pfultz2]

Moved flag for alpha checks to the ClangTidyContext

[NoQ]

In D46159#1088050, @alexfh wrote:

As folks noted here, some users prefer to use clang-tidy as a frontend for the static analyzer. If this helps them test experimental CSA features and CSA maintainers are willing to accept bug reports and potentially patches from this category of users, I don't want to create obstacles, as long as all experimental features need to be explicitly enabled.

+my imho thing: i totally don't mind that category of users :)

But just one thing - as of the current (for the last few years) balance between 1.5 – 2 maintainers being blessed with really impressing amount of contributors, we really really prefer maintainance help (eg. someone who would be able to take responsibility of making sure a certain check is complete and moved out of alpha - test it thoroughly, address known issues, keep maintaining it after it is declared stable) versus random improvements and experiments on which time is being spent without any visible movement towards making the check actually available to the users in the nearest future.

In this sense bug reports against abandoned alpha checkers (which are, unfortunately, the majority) aren't very useful. In most cases it's just too easy to see how broken they are. But if you are interested in a particular checker and want to work on it to make sure it's stable, we'd be glad to help, so please contact us on the mailing list.

That's kinda where we are these days.

[pfultz2]

In this sense bug reports against abandoned alpha checkers (which are, unfortunately, the majority) aren't very useful. In most cases it's just too easy to see how broken they are.

Although the majority are like that, not all of them are. Some like the Conversion checker doesn't seem to have any issues(because its fairly limited at this point). Others like StreamChecker probably just needs some simple fixes. Maybe we can move the alpha checks that have lots of issues and no contributions for awhile to another category such as abandoned.

But if you are interested in a particular checker and want to work on it to make sure it's stable, we'd be glad to help, so please contact us on the mailing list.

I am definitely interested in working on getting some of the checkers stable, and have already started some work on expanding the ConversionChecker, but would like user feedback as I move forward.

[NoQ]

Ok, i personally think this patch should go in. I think it makes it clear enough that an unsuspecting user would suspect something by reading the flag, and makes the feature hard enough to discover. And i'm happy to support anybody who's going to work on this stuff.

---

~*sorry for hijacking review with this, we should move this conversation to the list*~

In D46159#1088768, @pfultz2 wrote:

I am definitely interested in working on getting some of the checkers stable, and have already started some work on expanding the ConversionChecker, but would like user feedback as I move forward.

Yay! I would most likely have a chance to help out with final evaluation of checkers before they move out of alpha.

In D46159#1088768, @pfultz2 wrote:

Some like the Conversion checker doesn't seem to have any issues(because its fairly limited at this point).

Last time i tried it, it was very hard to understand positives produced by the checker. It seems that it needs to implement a relatively sophisticated "bug visitor" in order to explain why does it think that a certain conversion would overflow by highlighting the respective spot somewhere in the middle of the path. It is especially important when the value comes from an inlined function because the analyzer wouldn't draw paths through all inlined functions (it would have been overwhelming) unless something interesting happens in there.

Then somebody would need to have a look at intentional overflows and if it's possible to avoid them.

In D46159#1088768, @pfultz2 wrote:

Others like StreamChecker probably just needs some simple fixes.

Hmm, at a glance, it looks almost empty.

1. Again, it needs a bug visitor to highlight where the file was open. Otherwise many reports may become impossible to understand.
2. It is eagerly splitting states upon file open, which doubles remaining analysis time on every file open. This needs to be deferred until an actual branch is found in the code.
3. It needs a pointer escape callback, in order to work with wrappers around open/close functions. This needs to be complemented with a certain knowledge about the standard library, to see what functions should not cause escapes. SimpleStreamChecker has an example.
4. If it's ever to support open()/close(), it would, apart from other things, also need an "integer escape" callback , which will require a bit of digging into the analyzer core.

Pt.4 is mostly about feature work, but Pt.1–3 are must-fix. This isn't an overwhelming amount of work, but it will take some time.

Otherwise, yeah, it's a good simple check to have. It could be expanded to support a variety of weird APIs such as fdopen() but it's also a checker that's hard to get wrong. PthreadLockChecker is also a great candidate with similar problems; i have some unfinished work on that on here on the phabricator.

[alexfh]

Given Artem's answer (and if there are no objections from other CSA maintainers), I have no concerns with this patch going in. A couple of minor nits.

clang-tidy/ClangTidyDiagnosticConsumer.h

190–191 ↗

(On Diff #145229)

Let's call this function more consistently with all other instances of this setting, e.g. canEnableAnalyzerAlphaCheckers() (alternatively, mayEnableAnalyzerAlphaCheckers) and change the comment to be in line with the actual meaning of the function (it doesn't turn on alpha checkers, it merely allows them to be enabled).

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

1 ↗

(On Diff #145229)

nit: Add a trailing period.

[aaron.ballman]

Aside from a minor commenting nit, also LGTM.

clang-tidy/tool/ClangTidyMain.cpp

195 ↗

(On Diff #145229)

I'd reword to: This option allows enabling the experimental alpha checkers from the static analyzer.

[pfultz2]

Some changes based on feedback.

[pfultz2]

Is someone able to merge in my changes?

[alexfh]

In D46159#1098000, @pfultz2 wrote:

Is someone able to merge in my changes?

Will do.

[alexfh]

In D46159#1098018, @alexfh wrote:

In D46159#1098000, @pfultz2 wrote:

Is someone able to merge in my changes?

Will do.

Please rebase the patch, it doesn't apply cleanly.

[pfultz2]

Rebased

clang-tools-extra/trunk/clang-tidy/ClangTidy.h

 
 private:
   ClangTidyContext &Context;
   std::unique_ptr<ClangTidyCheckFactories> CheckFactories;
 };
 
 /// \brief Fills the list of check names that are enabled when the provided
 /// filters are applied.
-std::vector<std::string> getCheckNames(const ClangTidyOptions &Options);
+std::vector<std::string> getCheckNames(const ClangTidyOptions &Options,
+                                       bool AllowEnablingAnalyzerAlphaCheckers);
 
 /// \brief Returns the effective check-specific options.
 ///
 /// The method configures ClangTidy with the specified \p Options and collects
 /// effective options from all created checks. The returned set of options
 /// includes default check-specific options for all keys not overridden by \p
 /// Options.
-ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options);
+ClangTidyOptions::OptionMap
+getCheckOptions(const ClangTidyOptions &Options,
+                bool AllowEnablingAnalyzerAlphaCheckers);
 
 /// \brief Run a set of clang-tidy checks on a set of files.
 ///
 /// \param EnableCheckProfile If provided, it enables check profile collection
 /// in MatchFinder, and will contain the result of the profile.
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
                   const tooling::CompilationDatabase &Compilations,
                   ArrayRef<std::string> InputFiles,

clang-tools-extra/trunk/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(ClangTidyContext &Context) {
+static CheckersList getCheckersControlList(ClangTidyContext &Context,
+                                           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 |= Context.isCheckEnabled(ClangTidyCheckName);
   }
 
   if (!AnalyzerChecksEnabled)
     return List;
for (auto &Check : Checks) {
     Check->registerPPCallbacks(Compiler);
   }
 
   std::vector<std::unique_ptr<ASTConsumer>> Consumers;
   if (!Checks.empty())
     Consumers.push_back(Finder->newASTConsumer());
 
   AnalyzerOptionsRef AnalyzerOptions = Compiler.getAnalyzerOpts();
-  AnalyzerOptions->CheckersControlList = getCheckersControlList(Context);
+  AnalyzerOptions->CheckersControlList =
+      getCheckersControlList(Context, Context.canEnableAnalyzerAlphaCheckers());
   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(Profiling), std::move(Finder),
       std::move(Checks));
 }
 
 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 &AnalyzerCheck : getCheckersControlList(Context))
+  for (const auto &AnalyzerCheck : getCheckersControlList(
+           Context, Context.canEnableAnalyzerAlphaCheckers()))
     CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 
   std::sort(CheckNames.begin(), CheckNames.end());
   return CheckNames;
 }
 
 ClangTidyOptions::OptionMap ClangTidyASTConsumerFactory::getCheckOptions() {
   ClangTidyOptions::OptionMap Options;
void OptionsView::store(ClangTidyOptions::OptionMap &Options,
   Options[NamePrefix + LocalName.str()] = Value;
 }
 
 void OptionsView::store(ClangTidyOptions::OptionMap &Options,
                         StringRef LocalName, int64_t Value) const {
   store(Options, LocalName, llvm::itostr(Value));
 }
 
-std::vector<std::string> getCheckNames(const ClangTidyOptions &Options) {
+std::vector<std::string>
+getCheckNames(const ClangTidyOptions &Options,
+              bool AllowEnablingAnalyzerAlphaCheckers) {
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
-                                                Options));
+                                                Options),
+      AllowEnablingAnalyzerAlphaCheckers);
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckNames();
 }
 
-ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options) {
+ClangTidyOptions::OptionMap
+getCheckOptions(const ClangTidyOptions &Options,
+                bool AllowEnablingAnalyzerAlphaCheckers) {
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
-                                                Options));
+                                                Options),
+      AllowEnablingAnalyzerAlphaCheckers);
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
                   const CompilationDatabase &Compilations,
                   ArrayRef<std::string> InputFiles,
                   llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS,

clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h

 /// warnings like:
 /// \code
 /// Context->Diag(Loc, "Single-argument constructors must be explicit")
 ///     << FixItHint::CreateInsertion(Loc, "explicit ");
 /// \endcode
 class ClangTidyContext {
 public:
   /// \brief Initializes \c ClangTidyContext instance.
-  ClangTidyContext(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider);
+  ClangTidyContext(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
+                   bool AllowEnablingAnalyzerAlphaCheckers = false);
 
   ~ClangTidyContext();
 
   /// \brief Report any errors detected using this method.
   ///
   /// This is still under heavy development and will likely change towards using
   /// tablegen'd diagnostic IDs.
   /// FIXME: Figure out a way to manage ID spaces.
void setCurrentBuildDirectory(StringRef BuildDirectory) {
     CurrentBuildDirectory = BuildDirectory;
   }
 
   /// \brief Returns build directory of the current translation unit.
   const std::string &getCurrentBuildDirectory() {
     return CurrentBuildDirectory;
   }
 
+  /// \brief If the experimental alpha checkers from the static analyzer can be
+  /// enabled.
+  bool canEnableAnalyzerAlphaCheckers() const {
+    return AllowEnablingAnalyzerAlphaCheckers;
+  }
+
 private:
   // Calls setDiagnosticsEngine() and storeError().
   friend class ClangTidyDiagnosticConsumer;
   friend class ClangTidyPluginAction;
 
   /// \brief Sets the \c DiagnosticsEngine so that Diagnostics can be generated
   /// correctly.
   void setDiagnosticsEngine(DiagnosticsEngine *Engine);
private:
 
   ClangTidyStats Stats;
 
   std::string CurrentBuildDirectory;
 
   llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
 
   bool Profile;
+
+  bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
 // FIXME: If we move away from unit-tests, this can be moved to a private
 // implementation file.
 class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {

clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp

 
 private:
   GlobList Globs;
   enum Tristate { None, Yes, No };
   llvm::StringMap<Tristate> Cache;
 };
 
 ClangTidyContext::ClangTidyContext(
-    std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider)
+    std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
+    bool AllowEnablingAnalyzerAlphaCheckers)
     : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
-      Profile(false) {
+      Profile(false),
+      AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
   // Before the first translation unit we can get errors related to command-line
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
 }
 
 ClangTidyContext::~ClangTidyContext() = default;
 
 DiagnosticBuilder ClangTidyContext::diag(

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

 
 static cl::opt<bool> EnableCheckProfile("enable-check-profile", cl::desc(R"(
 Enable per-check timing profiles, and print a
 report to stderr.
 )"),
                                         cl::init(false),
                                         cl::cat(ClangTidyCategory));
 
+/// This option allows enabling the experimental alpha checkers from the static
+/// analyzer. This option is set to false and not visible in help, because it is
+/// highly not recommended for users.
+static cl::opt<bool>
+    AllowEnablingAnalyzerAlphaCheckers("allow-enabling-analyzer-alpha-checkers",
+                                       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 int clangTidyMain(int argc, const char **argv) {
   }
 
   SmallString<256> FilePath(FileName);
   if (std::error_code EC = llvm::sys::fs::make_absolute(FilePath)) {
     llvm::errs() << "Can't make absolute path from " << FileName << ": "
                  << EC.message() << "\n";
   }
   ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
-  std::vector<std::string> EnabledChecks = getCheckNames(EffectiveOptions);
+  std::vector<std::string> EnabledChecks =
+      getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
 
   if (ExplainConfig) {
     // FIXME: Show other ClangTidyOptions' fields, like ExtraArg.
     std::vector<clang::tidy::ClangTidyOptionsProvider::OptionsSource>
         RawOptions = OptionsProvider->getRawOptions(FilePath);
     for (const std::string &Check : EnabledChecks) {
       for (auto It = RawOptions.rbegin(); It != RawOptions.rend(); ++It) {
         if (It->first.Checks && GlobList(*It->first.Checks).contains(Check)) {
if (ListChecks) {
     llvm::outs() << "Enabled checks:";
     for (const auto &CheckName : EnabledChecks)
       llvm::outs() << "\n    " << CheckName;
     llvm::outs() << "\n\n";
     return 0;
   }
 
   if (DumpConfig) {
-    EffectiveOptions.CheckOptions = getCheckOptions(EffectiveOptions);
+    EffectiveOptions.CheckOptions =
+        getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
     llvm::outs() << configurationAsText(
                         ClangTidyOptions::getDefaults().mergeWith(
                             EffectiveOptions))
                  << "\n";
     return 0;
   }
 
   if (EnabledChecks.empty()) {
     llvm::errs() << "Error: no checks enabled.\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return 1;
   }
 
   if (PathList.empty()) {
     llvm::errs() << "Error: no input files specified.\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return 1;
   }
 
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
-  ClangTidyContext Context(std::move(OwningOptionsProvider));
+  ClangTidyContext Context(std::move(OwningOptionsProvider),
+                           AllowEnablingAnalyzerAlphaCheckers);
   runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
                EnableCheckProfile);
   ArrayRef<ClangTidyError> Errors = Context.getErrors();
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
                        return E.DiagLevel == ClangTidyError::Error;
                      }) != Errors.end();
 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;

clang-tools-extra/trunk/test/clang-tidy/enable-alpha-checks.cpp

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