[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Tue Aug 13 01:17:05 PDT 2019

Szelethus added a comment.

Too bad we're in different time zones, I can only respond in a giant comment, but I wouldn't want to make anyone feel left out :)

The conclusion to my responses is this:

- I recently sent out a mail to cfe-dev (I hope to have added your correct email adresses!) that proposes what I believe to be a far better, long term solution to the issue brought up in this thread. http://lists.llvm.org/pipermail/cfe-dev/2019-August/063070.html
- While I see that there are some disagreements on whether we should have experimental features as frontend flags, I still feel that this should be an analyzer config (or checker/package options). The solution in my mail makes this patch a temporary bandaid, not a feature.
- I understand that my stance on this is strong, but I'm still happy to go on with the discussion and be proven wrong.

As soon as we convert this into a config, I'll be happy to comment on the nits and accept the patch.

In D66042#1625971 <https://reviews.llvm.org/D66042#1625971>, @NoQ wrote:

> In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:
>
> > if we add this flag, people responsible for developing interafaces for the analyzer might end up using it.
>
>
> And this is fine, that's supported. There's a very limited list of such people and we could talk to all of them easily if we wanted to. On the other hand, an end-user running `clang --analyze -Xclang -flagflagflag` manually on his desktop is not supported.

But even if this is what we wanted to pursue, incrementally fixing up the code and adding this flag as a crown on top wod be the way to go. Frontend flags dont have alpha packages. We've always developed configs for in-development features.

> In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:
> 
>> Whenever we forget about adding a checker tag to a subchecker, this issue will arise again.
> 
> 
> Mmm, if we forget about adding a checker tag to a subchecker, then we already have a problem, regardless of this patch, right? The checker name in the bug report is going to be incorrect in this case, which will, at the very least, hurt clang-tidy users.
> 
> In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:
> 
>> Your patch title, and the things things that you said make me believe that there are only a handful of core checkers that cause headaches, how about you add checker options to those instead? If the entirety of the core is problematic, you might as well add a package option to it.
> 
> 
> My impression is kinda the opposite: the more we put our hacks into the imperative checker code, the harder it gets. `Checkers.td` "just works", but whenever we try to mess with it, we always get something wrong. Which is exactly why i greatly appreciated your work to formalize everything in tablegen: you allowed everybody to re-use the code that is easy to get wrong.

Very true, which is why I proposed a solution on the mailing list that would enforce using the correct checker tag. Until then, lets not cascade one issue into many more I guess? Disabling a checker should only be about getting rid of the diagnostics, so I think we should pursue that direction instead of silencing, and I think that's the expectation users have as well, don't they?

> In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:
> 
>> If you still want to make a new functionality that could silence any checker, create an analyzer config. Those really are meant for development purposes only...
> 
> 
> I have no data to conclude that frontend flags are more discoverable than `-analyzer-config` options. As a matter of our policy of what we actively support vs. what is passively available, those aren't supported.

I dont have data per se, but its only since the last release they became listable. Though I agree that a more aggressive warning message should be accompanied with them, something like "These configurations are meant for development purposes only!".

On another note, my patches that added frontend flags were stalled for very long stemming from this debate. I see the expression "our policy", but it seems like everyone views "our policy" in their own way -- Devin seems to be very concerned to the level where even the source code should be as secretive as possible, I like to think that that's maybe a bit too strict, but the frontend flags are our user interface, while You and Csaba seem to have a more relaxed stance. Maybe its time to formalize this.

I also dislike that we don't have an interface through the driver. I don't see why we shouldn't, and it would naturally create layers where frontend flags would justifiably be called development-only flags.

In D66042#1626086 <https://reviews.llvm.org/D66042#1626086>, @Charusso wrote:

> [...] I think not just scan-build would use that feature I have just implemented, as this is the only comfortable way to disable the core. That is why it is inside the Analyzer, and I believe every other place to implement would be a bad idea, as everyone really needs that feature. [...] Also I cannot really force out to implement that new feature in every scan-build like tooling, and since then this patch would bring up overhead without actual implementations.

Why is this the only why? Why are the previously mentioned solution any worse? Why is this being comfortable important, if its a development-only feature, and supposedly only used by scan-build?

You yourself stated that the construction of the trimmed graph, exploded graph and whatever else is of no concern to you, so why do you worry about some overhead? To be clear, what do you refer to when you mention "overhead"?

> In my mind every new API/feature like the NoteTag or the CallDescriptionMap should arrive in the code base with removing the old API. So we will have better code and no-one would use the old API and instead would improve the new much better API without continuing to create more and more deprecated code (that is happening with NoteTags ATM). Because we are working with tons of experimental features, like that two new improvement, we do not have such policy to require to deprecate the old API. As we have no policies according to experimental features and that patch is an experimental feature, I see two directions: publish that as it is, or abandon it, and use it locally.

There is no need to be this drastic! We implement in-development checkers in the alpha package, and in-development other features as off-by-default analyzer configs, this has been a tradition  long before us. Since this patch is not only experimental but known to be faulty, there it should reside. Its not a drastic change.

In D66042#1626280 <https://reviews.llvm.org/D66042#1626280>, @Charusso wrote:

> So, as I am the owner of the patch, I have to take responsibilities for my experimental stuff. Every experimental flag we provide we provide for peoples who creates interfaces. Like my `PathDiagnosticPopUpPiece` is an experimental feature, and has not been arrived to CodeChecker yet. It is upstreamed and if someone wants to invoke it, it is possible since it is upstreamed under the estimation it is experimental and non-required to implement. It was broken for two months and no-one cared because it is an experimental feature. Experimental features could arrive without a perfect shape and only made for people who could deal with them so that if someone does not know how to use them, we assume that that user would not use that.
>  That experimental-feature-chain invented by @NoQ in his previous quoted comment explains very well how bad is the situation since the beginning for an advanced user. They are advanced users so they could handle any experimental feature pretty easily. Also please note that, **it is an experimental feature**.

This feature is faulty, and its the responsibility of the reviewers not to let it through if it is so, at least not as-is. This is a bandaid to a problem we should fix, and shouldn't be a feature. I don't agree with this feature for the long term. While I have a firm stance on this, I might be wrong, this is why we need a discussion, and the problem of this code already being written could've been avoided if we had a round on the mailing list.

In D66042#1626460 <https://reviews.llvm.org/D66042#1626460>, @NoQ wrote:

> I'd like to hear @Szelethus's opinion one more time on this patch. I'm perfectly fine with abandoning the idea and going for in-checker suppressions, simply because there's at least one strong opinion against it and i don't want to push this further, but i just honestly think this patch is a good idea. This discussion has so far been very useful regardless, at least to me personally.

Please find my mail in which I propose how to solve this problem. While I read all of your responses and I also greatly appreciate the discussion (we've mentioned plenty of other things not related to this patch that we should talk about even more), and did so with an open mind, my stance is largely unchanged.

In D66042#1626513 <https://reviews.llvm.org/D66042#1626513>, @Charusso wrote:

> I really appreacite your ideas. It is unbelievable you guys bring up 20 different ideas for 5 LOC. I cannot really argue about any idea, as every of them could be a possible solution. I have to pick the right solution, and drop the other 19. I believe with that in mind what is an experimental feature and how we support to use the Analyzer, none of the 19 ideas would born. I did not want to refuse that many ideas, but I have to, because we could pick at most 1 to implement per patch. That is why I really try to emphasize it is under that experimental feature umbrella and we have to think no more about that patch from that point: since the beginning.

Given our discussion, we've thrown out all but 1 of the 4, by the way (fixing the actual problem, making this a config, creating checker/package options, solving this in scan-build only), ideas. Make this a config. You're correct, thats about 5 LOC change in this patch, at which point I'd be happy to accept :)

> I am so sorry I have to be a dictator here, but someone - probably me or the code owner - has to decide to move forward.

I feel very uncomfortable with this statement. I find my concerns genuine, yet you chose to not to answer them at all. You still didn't say a word about my ultimate, 5 LOC change of making this a config. I have worked plenty on this part of the project, and while it doesn't make me a code owner, and doesn't even make my opinion necessarily correct, these things shouldn't be dismissed.

While I understand your frustration, especially since the deadline is nearing, having a friendly, constructive discussion where we respond to each other points would be more beneficial I think. My ideas have been proven wrong or impractical countless times, but there only is a chance for that if people respond to my points.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66042/new/

https://reviews.llvm.org/D66042