[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

*gets hyped for the upcoming patchlanding party*

In D54438#1329425 <https://reviews.llvm.org/D54438#1329425>, @Szelethus wrote:

> Hmmm, I left plenty of room for improvement in the tblgen generation, we could generate compile-time errors on cycles within the dependency graph, try to include the generated file only once, but these clearly are very low-prio issues, so I'll do it later. I'll have to revisit the entire thing soon enough.


Hmm. The dependency cycle check sounds cool as long as we do actually have problems with dependency cycles. I guess we could just enable/disable the whole cycle of checkers all together? This might even be a useful feature.

What do you mean by "include the generated file only once"?



================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:225-229
+    if (!deps) {
+      // If we failed to enable any of the dependencies, don't enable this
+      // checker.
+      continue;
+    }
----------------
Aha, ok, so the current approach to conflict resolution is to only enable the checker if none of its dependencies were disabled on the command line. So if, say, `DependentChecker` depends on `BaseChecker`, once an `-analyzer-disable-checker BaseChecker` flag is passed, it needs to be reverted via `-analyzer-checker BaseChecker` before the `-analyzer-checker DependentChecker` directive would take effect, regardless of in which order it is with respect to the `BaseChecker`-enabling/disabling directives.

So we kinda choose to err on the side of disabling in ambiguous situations. Which kinda makes sense because the disable-argument is more rare and indicates an irritation of the user. But it is also kinda inconsistent with how the options interact in absence of dependencies: "the flag that was passed last overrides all previous flags". And we can kinda fix this inconsistency by introducing a different behavior:

- whenever an `-analyzer-checker` flag is passed, remove the "disabled" marks from all checkers it depends on;
- whenever an `-analyzer-disable-checker` flag is passed, remove the "enabled" marks from all checkers that depend on it.

This approach still ensures that the set of enabled checkers is consistent (i.e., users are still not allowed to shoot themselves in the foot by running a checker without its dependencies), but it also looks respects every flag in an intuitive manner. WDYT?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54438