[PATCH] D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D83660#2149470 <https://reviews.llvm.org/D83660#2149470>, @OikawaKirie wrote:

> It seems to be better to write a checker to report assertion failures.


No no no. No no no no no. Assertions don't say "the program crashes if this happens", assertions say "this doesn't happen". We should trust them, not argue against them. They're god-sent for any static analysis and also they themselves constitute a form of dynamic analysis. Like, even if there's `assert(false)` in the beginning of a function, we shouldn't flag it; it's merely an indication from the user that this function is never called. There are some cases where the assertion indeed doesn't make sense but they're extremely rare. It becomes even more confusing when people occasionally have code to handle a would-be assertion failure gracefully in release builds (i.e., when assertions are disabled).

What we should warn about, though, is //precondition// failures. And the assertion that we've stepped on here is, in fact, a precondition: "if you call this method on an optional, //first// make sure the optional isn't empty". I.e., the assertion is put inside the method, but in fact it should be on the method boundary, and that's where it makes sense to warn ("method is misused"). But as of now C/C++ doesn't have a way of formally differentiating assertions from preconditions or postconditions; such feature is known as "contracts" and it was recently delayed until C++23. So manually teaching the static analyzer about preconditions of `Optional`'s methods is the right solution until we have contracts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83660