Making FrontendActions composable (or not)

Heya,

while trying to integrate the static analyzer with clang-tidy I ran into the problem of combining the static analyzer’s FrontendAction with clang-tidy’s. As most of FrontendAction’s methods are protected it is not straight forward to write a combiner/forwarder.

I see multiple ways to solve this problem, but don’t really know what the best way to go forward is:
a) create a CombinedFrontendAction (either by befriending FrontendAction, or lowering its access boundaries); I assume that the protectedness in FrontendAction exists for a reason, so there might be downsides to this I’m not aware of
b) modularize the Analyzer enough to be able to run it as a PPCallback / ASTConsumer which is registered by a one-off FrontendAction; the problem with this approach is that when we want to build higher-level tools, we always need to keep that split

Thoughts?
/Manuel

It seems weird to be using FrontendAction for clang-tidy (and the analyzer too). clang-tidy isn’t a compiler frontend (I mean look at the methods: hasCodeCompletionSupport? getCurrentFileKind? shouldEraseOutputFiles?). AFAICT All you are really need is a vector of PPCallbacks, and a vector of what is effectively std::function<TidyResult(ASTContext &)>. It seems like there should be at most a single FrontendAction which just sets things up so that it can forward everything down into those vectors.

– Sean Silva

It seems weird to be using FrontendAction for clang-tidy (and the analyzer
too). clang-tidy isn't a compiler frontend (I mean look at the methods:
hasCodeCompletionSupport? getCurrentFileKind? shouldEraseOutputFiles?).
AFAICT All you are really need is a vector of PPCallbacks, and a vector of
what is effectively std::function<TidyResult(ASTContext &)>. It seems like
there should be at most a single FrontendAction which just sets things up
so that it can forward everything down into those vectors.

That is basically what I proposed as "b". The problem with that is that
this is not how the static analyzer is currently factored - if I want to
pull it apart into a PPCallbacks and an ASTConsumer, I have to duplicate
significant amounts of code. Thus, if we want to go that way, the next step
is to make the static analyzer more modular: basically pull out a class
that can give you both the PPCallbacks and the ASTConsumer. But on the
other hand, such a class already exists - it is the static analyzer's
FrontendAction; it is exactly what we want, namely a "factory" class (well,
in the broader sense) of a PPCallbacks and ASTConsumer instance, which
happens to also be able to put the compiler into the state it needs it in.

Perhaps when you argue on the interface level, you mean "there should
really be something in between a FrontendAction and an ASTConsumer or
PPCallbacks interface", and perhaps you're right, but that sounds like a
much bigger change...

Cheers,
/Manuel

It seems weird to be using FrontendAction for clang-tidy (and the
analyzer too). clang-tidy isn't a compiler frontend (I mean look at the
methods: hasCodeCompletionSupport? getCurrentFileKind?
shouldEraseOutputFiles?). AFAICT All you are really need is a vector of
PPCallbacks, and a vector of what is effectively
std::function<TidyResult(ASTContext &)>. It seems like there should be at
most a single FrontendAction which just sets things up so that it can
forward everything down into those vectors.

That is basically what I proposed as "b". The problem with that is that
this is not how the static analyzer is currently factored - if I want to
pull it apart into a PPCallbacks and an ASTConsumer, I have to duplicate
significant amounts of code. Thus, if we want to go that way, the next step
is to make the static analyzer more modular: basically pull out a class
that can give you both the PPCallbacks and the ASTConsumer. But on the
other hand, such a class already exists - it is the static analyzer's
FrontendAction; it is exactly what we want, namely a "factory" class (well,
in the broader sense) of a PPCallbacks and ASTConsumer instance, which
happens to also be able to put the compiler into the state it needs it in.

I'm not sure I understand what you mean by "put the compiler into the state
it needs it in". That seems like it completely violates the encapsulation
(I'm imagining something like it twiddling LangOptions; that would wreak
havoc on everything else...).

-- Sean Silva

It seems weird to be using FrontendAction for clang-tidy (and the
analyzer too). clang-tidy isn't a compiler frontend (I mean look at the
methods: hasCodeCompletionSupport? getCurrentFileKind?
shouldEraseOutputFiles?). AFAICT All you are really need is a vector of
PPCallbacks, and a vector of what is effectively
std::function<TidyResult(ASTContext &)>. It seems like there should be at
most a single FrontendAction which just sets things up so that it can
forward everything down into those vectors.

That is basically what I proposed as "b". The problem with that is that
this is not how the static analyzer is currently factored - if I want to
pull it apart into a PPCallbacks and an ASTConsumer, I have to duplicate
significant amounts of code. Thus, if we want to go that way, the next step
is to make the static analyzer more modular: basically pull out a class
that can give you both the PPCallbacks and the ASTConsumer. But on the
other hand, such a class already exists - it is the static analyzer's
FrontendAction; it is exactly what we want, namely a "factory" class (well,
in the broader sense) of a PPCallbacks and ASTConsumer instance, which
happens to also be able to put the compiler into the state it needs it in.

I'm not sure I understand what you mean by "put the compiler into the
state it needs it in". That seems like it completely violates the
encapsulation (I'm imagining something like it twiddling LangOptions; that
would wreak havoc on everything else...).

Sorry, I worded that incorrectly - it pulls various parts of configuration
information out of the compiler.

It’s been a while since I’ve looked at the frontend-ish parts of the analyzer, but AFAICT it’s not actually using PPCallbacks in any way. I don’t think there’s any problem with jumping directly to the AnalysisConsumer.

That said, if we ever want to add preprocessing-time checks, they wouldn’t be included in clang-tidy without a separate mechanism, but I think I’m okay with that. The current model is certainly intended to behave as an idempotent ASTConsumer.

Jordan

Ah, cool - then I'll try that and see how it goes... Thanks!

This is correct. The analyzer waits until the entire translation unit is available before doing anything.