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

Donát Nagy via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 7 05:33:32 PDT 2025


NagyDonat wrote:

I realized that there is an unfortunately incompatibility within my plans:
>  * I'll add tests (probably a new debug checker + a simple lit test that invokes it – I think this would be easier to implement than unit tests).
>    [...]
>  * After that I will probably create a PR that makes `CallEvent::getState()` ~private~ protected and exposes a `CallEvent::getASTContext()` utility method (because it's harmless and would be useful for the checker code that currently calls `getState()`).

When `CallEvent::getState()` becomes protected (which is undoubtedly a step forward) then it will be impossible to have tests which validate that the checker callback can access the same state through the `CallEvent` and the `CheckerContext` (because it cannot look at the state of the `CallEvent` directly). This means that I don't see a good way for testing the effects of this change:
- I don't want to expose a public `CallEvent::getStateForTesting()` -- we shouldn't pollute the codebase with this kind of thing.
- Causing tricky state changes that are (barely) observable through the public `CallEvent` API is even worse -- that is basically exploiting bugs for testing purposes.
- I don't think that unit tests offer a simple solution (visibility also limits them) and I'm not willing to write some complex mocks / workarounds that solve it somehow.

Based on this I would like to merge this PR without adding tests:
- I am confident that this patch fixes logically incorrect internal behavior (some checkers receive `CallEvent`s with obsolete `State` objects), although I don't know whether this can cause buggy output. (As a rough guess, I would assign 60% chance to the existence of corner cases where this bad logic causes buggy output, but it would be very difficult to find such cases.)
- Merging this PR wouldn't reduce the test coverage -- the `State` attached to the `CallEvent` wasn't tested before this change either.
- I can evaluate the effects of this change on our usual collection of open source projects to validate that it doesn't break anything.
@steakhal @haoNoQ What do you think about this?

(If you don't accept this PR without tests and can't suggest a simple way for testing it, I'll abandon it, because I already feel that I spent too much time on it.)

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


More information about the cfe-commits mailing list