[PATCH] D71510: [clang][checkers] Added new checker 'error-return-checker'.

Mon Jan 13 17:09:15 PST 2020

NoQ added a comment.

Uh-oh, what happened here?

Please don't post huge patches. 600 lines of code is huge. You could start off by implementing a single check (eg., `NullErrorResultChecker`) with a single library function and then add more checks and more functions in follow-up patches; this would have been much easier to review and would have made the community happy.

-----

Ok, here's how i see this:

1. In the beginning there was `__attribute__((warn_unused_result))`. It came with a compiler warning when the return value was completely discarded. Putting it into a variable or casting it to `(void)` silenced the warning.
2. Then someone decided that putting that attribute on `printf` and `scanf` is not humane because it will infuriate all first-year computer science students who just want to read a number, multiply it by 2 and print the answer. So it was removed from most functions that needed it for security-related reasons, and only remained on, basically, pure functions such as `std::vector::empty()` (so that it wasn't confused with `std::vector::clear()`).
3. Removing the attribute from library headers means that the compiler will be completely unable to diagnose upon discarded return values. After all, the compiler isn't allowed to possess knowledge of APIs.
4. This is where @balazske comes in and decides to put the extra warning into the Static Analyzer. After all, it's allowed to possess knowledge of APIs, and it's also an opt-in tool, which avoids the first-year students problem.
5. But then @balazske becomes obsessed with the power that the Static Analyzer gives him and decides to also make the warning perfectly precise with the help of path-sensitive analysis. This would show it to all the nasty developers who have so far got away with casting the value to `(void)` to suppress the warning. They can no longer trick us. They will have to //actually// check the value.

I want to discuss step 5. Do we really need to go that far? Your solution is mathematically perfect (it probably isn't just by looking at the set of the checker callbacks that you've subscribed so far, but suppose it is). But is it actually good for the humanity to have the perfect solution? Do you really want to uncover all the places where the developer has intentionally suppressed the warning for the unused result? Do you really want to engage in an arms race with a developer who wants to silence the warning? Or would everybody be much happier if you simply re-used the implementation of `__attribute__((warn_unused_result))` and treat its limitations as if it's the intended behavior?

Like, i myself don't have an answer to this question. I kind of understand both sides. Your approach is more costly, as you'll have to re-investigate all the previous suppression sites, but it may uncover a few more bugs, as well as some accidental omissions that were accidentally suppressed (e.g., the return value was intentionally put into a variable, but the value was never read (which is btw also a dead store, so our other checker will find it)). On the other hand, I believe that a simple AST-based approach employed by `warn_unused_result` would be good enough for most practical purposes, it would have low false positive rate (something we really value a lot) and provide an intuitive suppression mechanism, and it will also be much easier to maintain (which really pleases me as a maintainer who'll have to fix bugs in your code once it's committed, but ideally it shouldn't outweigh the benefits of the user).

I'd love to have you collect some data on this subject, and you're in a good position to do so, given that you already have a checker. If you run your checker on a lot of code, how many warnings will be non-trivially path-sensitive (i.e., the value isn't immediately discarded but dragged around for a bit, but still not checked)? Are these warnings valuable? How many of them highlight intentional suppressions?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71510