This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.
ClosedPublic

Authored by NoQ on Sep 10 2019, 4:44 PM.

Details

Summary

The AnalyzerOptions object contains too much information that's completely specific to the Analyzer. It is also being referenced by path diagnostic consumers to tweak their behavior. If we want path diagnostic consumers to function separately from the Analyzer, we'll have to make a smaller options object that only contains relevant options.

@Szelethus: I could have stored PathDiagnosticConsumerOptions in AnalyzerOptions by value and pass a const reference around, but it wasn't pleasant to integrate with AnalyzerOptions.def. I.e., i'd have to implement a new kind of option that doesn't allocate its own field but instead re-uses a field within a sub-object. Do you want me to go for it or is this implementation good enough or do you have other approaches in mind?

Diff Detail

Event Timeline

NoQ created this revision.Sep 10 2019, 4:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 4:44 PM

Are you planning to move users of these options -- like PlistDiagnostics -- to libAnalysis?

clang/include/clang/Analysis/PathDiagnostic.h
81

"but for some consumers is experimental" -- parse error. What is experimental?

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
534

I think we need an equal sign for ClangTidy to pick it up: /*foo=*/

Szelethus added a comment.EditedSep 11 2019, 7:26 AM

@Szelethus: I could have stored PathDiagnosticConsumerOptions in AnalyzerOptions by value and pass a const reference around, but it wasn't pleasant to integrate with AnalyzerOptions.def. I.e., i'd have to implement a new kind of option that doesn't allocate its own field but instead re-uses a field within a sub-object. Do you want me to go for it or is this implementation good enough?

I don't really like this direction :^) Imagine how tedious and non-obvious it'll be to add a new pathdiagnostic option. I don't like we're copying this around. There is also a discussion to be had about whether having to specify -analyzer-config to a no longer analyzer specific feature is any good.

or do you have other approaches in mind?

I do! The high level idea is to completely separate pathdiagnostic configs from the AnalyzerOptions, I believe this is more in line with your goal.

  • Let's create the equivalent of -analyzer-config, -pathdiagnostic-config, and things like -pathdiagnostic-config-help.
  • Avoid getting a parsing error for specifying -analyzer-config macro-expansion instead of -pathdiagnostic-config macro-expansion.
    • Create a field similar to AnalyzerOptions::AnalyzerConfigCmdFlags called AnalyzerOptions::AnalyzerConfigAliases with the 4 configs that we're moving over. In compatibility mode, AnalyzerOptions::isUnknownAnalyzerConfig() shall search in this field as well. In non-compatibility mode, this will result in an error.
  • Make PathDiagnosticConsumerOptions field of CompilerInvocation, similar to AnalyzerOptions.
  • Your idea of implementing a DIAGNOSTIC_OPTION macro sounds nice.
    • Delete these entries from AnalyzerOptions.def.
    • Create clang/include/Analysis/PathDiagnostics.def, list them there.
    • Generate fields to PathDiagnosticConsumerOptions similar to how AnalyzerOptions does it.
  • Parse the options CompilerInvocation.cpp
    • Most of the analyzer config parsing functions can be reused!
    • For these select 4 options that suffer compatibility issues, manually check AnalyzerOptions::ConfigTable.

WDYT? I am happy to help out, I realize this is plenty of code to write, if we go for it.

NoQ added a comment.Sep 11 2019, 11:32 AM

I do!

Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to use the same set of flags to control these options (?) Much like -analyzer-config, these flags will have different "style" compared to other flags in the tool. Which is probably not too bad for hidden frontend flags that control the Analyzer because they're anyway set by a separate GUI checkbox, but the inconsistency with other flags would be super glaring in case of Clang-Tidy CLI. Do we really want to go in that direction? I'll be much more comfortable with letting each tool deal with its flags the way it prefers - this doesn't look like a good place for code reuse to me.

NoQ added a comment.Sep 11 2019, 11:35 AM
  • For these select 4 options that suffer compatibility issues, manually check AnalyzerOptions::ConfigTable.

*3 options. ToolInvocation is not really an option that you're ever supposed to adjust manually. Which is one more reason for me to think of this structure as of an implementation detail - an interface through which diagnostic consumers' behavior can be tweaked, regardless of why the tool wants it to be tweaked (because the human told it to or because the tool itself knows better).

NoQ added a comment.Sep 11 2019, 11:36 AM

Are you planning to move users of these options -- like PlistDiagnostics -- to libAnalysis?

Yup, i was supposed to do that in D67422 :)

NoQ updated this revision to Diff 219821.Sep 11 2019, 3:54 PM

Unforget to do the same for the text consumer. As a side effect, the factory function for the text consumer is no longer special, which will be less confusing when put in libAnalysis.

Address minor comments, don't address major comments.

NoQ updated this revision to Diff 219826.Sep 11 2019, 4:14 PM

Clean up a tiny bit of dead code.

NoQ marked 2 inline comments as done.Sep 11 2019, 4:15 PM

PathDiagnosticConsumerOptions is pretty lightweight, and is not passed around in hot paths if I understand correctly. What do you think about passing it by value everywhere?

In D67420#1666578, @NoQ wrote:

I do!

Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to use the same set of flags to control these options (?) Much like -analyzer-config, these flags will have different "style" compared to other flags in the tool. Which is probably not too bad for hidden frontend flags that control the Analyzer because they're anyway set by a separate GUI checkbox, but the inconsistency with other flags would be super glaring in case of Clang-Tidy CLI. Do we really want to go in that direction? I'll be much more comfortable with letting each tool deal with its flags the way it prefers - this doesn't look like a good place for code reuse to me.

There are two things I wanted to touch on, but kinda failed to emphasize it. First is the topic of whether -analyzer-config flags are fitting for a feature no longer specific to the analyzer, and the second is similar in nature, but in the actual code -- it doesn't doesn't feel natural to me that AnalyzerOptions is required to construct this, while at the same time we're trying to make diagnostics construction independent of the analyzer. But I really wouldn't like to overengineer this just for the sake of it :^)

On the first, I'm kinda meh, even if we went for it, it would be a separate revision I guess, but the second has me concerned, unless I'm not following something right.

NoQ added a comment.Sep 12 2019, 2:08 PM

...and the second is similar in nature, but in the actual code -- it doesn't doesn't feel natural to me that AnalyzerOptions is required to construct this, while at the same time we're trying to make diagnostics construction independent of the analyzer.

It's not that AnalyzerOptions is necessary to construct this, it's more like sufficient. PathDiagnosticConsumerOptions is a plain de-encapsulated aggregate and anybody can aggregate-initialize or mutate it. But in the realm of the Analyzer it has "accidentally" turned out that AnalyzerOptions has just the right amount of information to fill it in.

In D67420#1668474, @NoQ wrote:

It's not that AnalyzerOptions is necessary to construct this, it's more like sufficient.

*sudden enlightenment*

Ah, okay, right, I'm a dummie, I think I got what's happening here! :) In that case, the idea sounds great. I'll take a second look on the code later (I have a feeling that we should delete the default ctor of PathDiagnosticConsumerOptions, etc), but I'm just a tad too tired atm :)

Aha, so every user gets to create their own PathDiagnosticConsumerOptions object, makes sense! There is no interface misconception, because -analyzer-config will only configure what the analyzer would tinket with. I like this patch! If you dont mind, I'd prefer to have one last round with this, but otherwise LGTM.

clang/include/clang/Analysis/PathDiagnostic.h
63

Lets delete the default constructor. It would be an option to generate this with the analyzer secific .def file, but since we have so few users, lets force them to create this object manually :)

Szelethus added inline comments.Sep 15 2019, 7:56 AM
clang/include/clang/Analysis/PathDiagnostic.h
81

I suspect you copied these from the .def file, did you change the descriptions there too?

NoQ updated this revision to Diff 277652.Jul 13 2020, 9:16 PM
NoQ marked 2 inline comments as done.

Rebase!! This work was on hiatus but i plan to continue.

Address a review comment.

There are two new path diagnostic options since the last patch that correspond to the optional behaviors of the text consumer that were introduced since last revision:

  • ShouldApplyFixIts;
  • ShouldDisplayDiagnosticName.
NoQ added inline comments.
clang/include/clang/Analysis/PathDiagnostic.h
81

Nope, these were hand-written. I tried to make sure they don't mention any static analyzer specific things.

I do like this change even apart from the clang-tidy integration epic. I strongly believe that decoupling (when done right) makes things easier and this change seems like a good first step.

I've seen you resurrecting the thread, I'll just take a bit of time to remember what happened in the previous episodes!

Szelethus accepted this revision.Jul 14 2020, 5:41 AM
Szelethus marked 2 inline comments as done.

LGTM! Very well done!

I strongly believe that decoupling (when done right) makes things easier and this change seems like a good first step.

This patch I think categorizes, rather. We could do a much better job at that with these options, I agree, if we switched over to TableGen (D83475#inline-768468). The problem is, its hard to justify the engineering hours for an arguably negligible gain.

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
47–48

Nice.

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
39–42 ↗(On Diff #277652)

Really nice!

This revision is now accepted and ready to land.Jul 14 2020, 5:41 AM
NoQ updated this revision to Diff 277861.Jul 14 2020, 9:10 AM

Remove the ShouldDisplayPathNotes option as it never was an AnalyzerOption to begin with and it only makes things worse.