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

Bal√°zs Benics via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 19 03:13:48 PDT 2022

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";

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list