[clang] [analyzer] Avoid use of `CallEvent`s with obsolete state (PR #160707)

[Artem Dergachev via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/160707 at github.com>


haoNoQ wrote:

By making `getState()` private/protected you effectively prove it at compile time that the code no longer makes the mistake you're describing. I see this as a much better thing to do than making a runtime test. Like, why would we even be here if we didn't believe in the superiority of compile-time bug prevention? 😅

Also we're not even trying to prevent the `CallEvent`'s inner state from going out of sync. We explicitly *allow* it to go out of sync. It doesn't make sense to test that it stays in sync.

What makes sense to test, what we really care about, is that the output of the remaining public accessors that look things up from the state stays in sync. And that's a contract that has been followed from the start, your patch didn't improve it, so it's not exactly in the spirit of TDD. But these tests are at least useful for documenting the contract, even if it's currently impossible to write code that would violate that contract. They'd be able to catch an unexpected future situation in which something really goes horribly wrong, like if the argument values in the Environment actively mutate during the lifetime of the `CallEvent`.

Maybe you could also add a few assertions around the call sites of the `CallEvent`'s access sites in the engine. Like, if you're accessing `getArgSVal()`, confirm that the same access to your current state outside of `CallEvent` yields the same value. These assertions may be less trivial and more on-point. But, again, they won't be new.

Or you could, like, make a SFINAE static assertion to confirm that `T.getState()` leads to a substitution failure when `T` is substituted with `CallEvent`. This would make buildbots red when somebody accidentally removes the access qualifier, so we no longer have to rely on a good-faith agreement that mindlessly widening access qualifiers is similar to mindlessly deleting tests that your patch has broken. But this doesn't prevent people from adding more accessor methods that are incorrect, and I'm not sure how that'd work. Unless you find a way to limit the information available to `CallEvent` itself, eg. don't bundle the entire State with it, but only the Environment. But, again, it's not like you can prevent people from adding more data fields in the future. Or maybe you could still do that with a static assertion or SFINAE? Like, confirm that each of the intended fields is available and is of the right type, and the total size of the object accounts for all these fields? This may be a good guarantee that every time somebody wants to add or access more data or change the type of the existing data they'd make buildbots angry and they'll be forced to read your angry warning comment. But, again, that's not something we usually require for every commit. If that's your best option, I'm actually OK with having no new tests in this patch that were failing before the patch. It sounds like you already plan to do more than enough to make your change difficult to regress.

https://github.com/llvm/llvm-project/pull/160707