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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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

> In D66042#1624320 <https://reviews.llvm.org/D66042#1624320>, @Szelethus wrote:
>
> > I think it would make a lot more sense to create a separate (and hidden!) coreModeling package that would do all the modeling, and regard core as a highly recommended, but not mandatory set of checkers. Wouldn't this create a cleaner interface?
>
>
> Sadly separating a modeling package is impossible. The checkers subscription-based design is made for that purpose to make them both model the analysis, and both emit reports. None of the checkers has separated modeling and separated business logic, because they are so tied together. The reporting part fires on given modeling parts, like that is why they could report errors during modeling and stop the modeling. May if we redesign all the core checkers to separated business/modeling logic it would be helpful.


I didn't mean to do too invasive changes, only something like this, which would be tedious but not difficult:

  struct ModelingChecker {
    bool AreDiagnosticsEnabled = false;
    void model() const;
  };
  
  void ModelingChecker::model() const {
    // whatever
  
    if (!AreDiagnosticsEnabled) {
      C.generateSink(State, C.getPredecessor());
      return;
    }
  
    static CheckerProgramPointTag DiagnosticCheckerTag(this, "DiagnosticChecker");
    const ExplodedNode *ErrorNode = C.generateErrorNode(State, &DiagnosticCheckerTag);
    // etc etc
  }
  
  void registerDiagnosticChecker(CheckerManager &Mgr) {
    Mgr.getChecker<ModelingChecker>()->AreDiagnosticsEnabled = true;
  }

A solution like this would preserve the current checker structures while neatly hiding the implementation part.

> The problem with that it requires huge overhead for no reason. My idea here is to create a single CoreChecker which does only the core modeling and existing core checkers has the reporting logic so they message with the CoreChecker. I believe it could not be scalable, as we already have 3k LOC checkers and apiModeling would merge into that at some point.

I think these aren't anything to worry about if we use the above solution :^)

> Also this patch aims to hide 600 `cast<>` related reports, and try to fix that ambiguity to "disable a core checker" as we really mean that to do not emit uninteresting reports. Personally I am really against the idea to make the core modeling disable-able, this scan-build addition fix that, you cannot disable it. If crash occurs with the core package then we should give it a 9000 priority level on Bugzilla and `fix-it`.

My wording may have been poor, apologies for the misunderstanding -- of course I do not intend to get rid of the modeling, just achieving the same goal from a different angle. `coreModeling` would be a dependency of all path sensitive checkers (we did talk about this in one of my patches), and the user would no longer be exposed to the option of disabling them, only their diagnostics. This is similar to the way how the checker dependency system was reimplemented as well, and I like to think that the analyzer's interface really benefited from it.

What do you think?


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

https://reviews.llvm.org/D66042