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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

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

> 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.

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.

>> 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.

The `-analyzer-output=text` comes from a frontend. (I misplaced the source right now, but I did see it recently on the LLVM github.) The reasons I'm not too concerned are:

1. It looks like a pre-existing behavior. (The text frontend consumer appeared to be printing fixits independently of whatever other diagnostic flags)
2. When I say gated, I mean that the default behavior of the checker hasn't changed. These fixits are opt-in. If you're opting in, you can also choose to not use the text output.

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

Thanks! More reviewers means I learn more and can be more accurate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352