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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I'm worried that even if the warning is correct, the suggested fix is not necessarily the right solution.

The very nature of path-sensitive warnings requires multiple events to happen on the path in order for the problem to manifest. Eliminating even one of those critical events from the path would eliminate the warning even if all other events are still present. The fix-it hint you're adding targets only one of those events. So it'll eliminate the warning, but so would 2-3 other potential fixes, and there's no way to know which one is actually preferred.

For example, you can eliminate a null-pointer-to-nonnull warning by either changing the "to-nonnull" part (changing the attribute) or by changing the "null-pointer" part (eg., adding a null check before the call). Both fixes make sense depending on the circumstances. Say, if the callee function always crashes on null pointer parameter, annotating the parameter as nullable is a totally wrong thing to do.

So I generally advice against fix-it hints for path-sensitive warnings. I'm curious how you plan to deal with this problem in your specific workflow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352