[PATCH] D123352: [analyzer] Add FixItHint to `nullability.NullReturnedFromNonnull` and `nullability.NullableReturnedFromNonnull`

Tue Apr 26 01:50:56 PDT 2022

whisperity added a comment.

Regarding FixIts... FixIts are implemented in the "Diagnostic" library, which is non-specific to neither Clang-Tidy nor Sema whatsoever, they use the same infrastructure under the hood. Why the apply logic in CSA might do the same FixIt multiple times is beyond me, but I know that both `clang-apply-replacements` and `clang-tidy` go to length to ensure that in case multiple checkers report to the same location with potentially conflicting FixIts, then none gets applied, because applying all of them would result in ridiculously broken source code. They internally become an object in the `clang::tooling` namespace which is implemented as a core Clang library. The relevant entrypoint to this logic, at least in Clang-Tidy, should be this one: http://github.com/llvm/llvm-project/blob/8f9dd5e608c0ac201ab682ccc89ac3be2dfd0d29/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L115-L134

FixIts are very rudimentary under the hood. Essentially, they allow you to, in a file at a given position, delete `N` characters and insert `M` characters. Removal fixits only delete, insertion fixits only insert (duh), and just like in version control, a //modification// is a "removal" and an "insertion" grouped into one action...

If FixIts are so broken in CSA's infrastructure as @steakhal observed then it might very well be a good thing to hold off on them and fix the infrastructure first. Or at least remove the ability to auto-apply them. You don't **have to** apply the fixes, but you can always get the neat little green-coloured notes as to what could the user should insert.

In D123352#3463063 <https://reviews.llvm.org/D123352#3463063>, @MosheBerman wrote:

> There's a DeadStore checker that does have some fixit code, which is where I found the `FixItHint` class. I did notice it isn't used in too many other places.

In D123352#3439390 <https://reviews.llvm.org/D123352#3439390>, @steakhal wrote:

> For fixits, we should be confident that some property holds, but the static analyzer might conclude erroneously that a given pointer is null resulting in a **bad** fixit.

Dead stores and dead code are, for the most part, rather easy to reason about and not make dangerous fixits by their removal. (I'd go far enough to say the DeadStore checker should be a run-of-the-mill warning in Sema, and not some extra fluff only available in CSA...)

In D123352#3439649 <https://reviews.llvm.org/D123352#3439649>, @MosheBerman wrote:

> Where can I learn more about this?  Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

Very unlikely. While Clang-Tidy can execute CSA checkers (because CSA is a library within Clang, but AFAIK calling CSA from Tidy doesn't handle things like CTU), Clang-Tidy does not contain **any** notion of dependency between checks and they are meant to be independent. Moreover, you would need to enforce a sequential execution by the user //across// binaries (not to mention the fact that the entire project needs to be parsed twice, which might not be a trivial cost!), which seems error-prone...

-----

In general, why you are trying to achieve this with the static analyser? It looks to me as if what you want is to mechanically add an annotation to some types if they are in between specific macros / annotations. More specifically, the table that is in your original post doesn't seem to involve path-sensitive information. So could this be a purely (platform-specific?) Tidy check? Your test code also doesn't contain anything that seems to depend on path-sensitive information.

1. Where is this `NS_` annotation coming from? Is this a thing of a specific platform, or specific to your company's products?
2. If you are sure you will not depend on any path-sensitive symbolic execution information (which you currently appear you do not), and the answer to `1.` is meaningful enough to be useful for a wider community (that is, not just you or your company?), then I would suggest rewriting the whole thing within Clang-Tidy. Clang-Tidy works off of AST matchers directly. The nullability annotations are attributes, which are also AST nodes, so you can handle them similarly to matching types or Decls.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352