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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a subscriber: whisperity.
steakhal added a comment.

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

> In D123352#3439390 <https://reviews.llvm.org/D123352#3439390>, @steakhal wrote:
>
>> tldr; static-analyzer fixits are not completely implemented.
>
> Where can I learn more about this?

Grep through the code and look for references to the variable. It's not used widely.
In fact, there are no checkers facilitating fixits.

> Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

I don't believe that `clang-tidy` should be involved in this. It would be better to keep it in-house (in the analyzer).

>> When I passed the `apply-fixits`, it modified the input source file - as I expected.
>
> Did you test this diff, or an existing checker? Would you please share the command you used to test?

Sort of. I tried to apply some parts of it by hand, and check the output difference in my command.

I cannot immediately recall, but I cannot remember any major obstacles, which suggests that I had to set the `apply-fixits=true` analyzer config option and that's it.
Since there were no checkers emitting fixit hints, I modified an existing one to emit a dummy fixithint. Nothing fancy there.

>> Then I tried the `-analyzer-output=text` and suddenly it inserted the fixit 2 times xD, which is less than ideal and we should fix this.
>> And I'm expecting many more bugs with this feature.
>
> This is why it's gated. xD.

What do you mean by 'gated'? It seems to be a bug, which we should fix prior to this.

PS: I'm also inviting @whisperity since he is more experienced with fixits and that sort of stuff.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:162-164
+  /// 2. If we are annotating an Objective-C method, and not a function, we
+  ///    want to use the `nullable` form instead of `_Nullable`.
+  ///    When \p SyntaxSugar is true, we handle the second case.
----------------
You could pass the owning Decl to this function and directly figure out if it's an objc method or not.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:675
     OS << " returned from a " << C.getDeclDescription(D) <<
-          " that is expected to return a non-null value";
+          " that is expected to return a non-null value foo";
+
----------------
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352