This patch converts the ObjCDeallocChecker to be path sensitive. The primary motivation for this change is to prevent false positives when -dealloc calls helper invalidation methods to release instance variables, but it additionally improves precision when -dealloc contains control flow. It also reduces the need for pattern matching. The check for missing -dealloc methods remains AST-based.
|89 ↗||(On Diff #48693)|
Let's not use these comments blocks. We should use oxygen style comments.
|161 ↗||(On Diff #48693)|
Use doxygen throughout. Please, mention that this is an AST check.
|184 ↗||(On Diff #48693)|
Looks like we do not use this for the other checker. Is that the correct behavior? Seems like it.
Maybe we should reflect this in the method name.
|198 ↗||(On Diff #48693)|
We usually capitalize this.
|249 ↗||(On Diff #48693)|
I do not understand this.
|261 ↗||(On Diff #48693)|
I think it's worth adding "If we are in -dealloc or" ...
|298 ↗||(On Diff #48693)|
Add: "We have to release before a call to [super dealloc]."
|383 ↗||(On Diff #48693)|
Hopefully, this ensures that all symbols added in the function begin call are removed! The logic is slightly different than where we were adding these:
|418 ↗||(On Diff #48693)|
This might be too long.. Maybe we could remove "instance variable"?
|526 ↗||(On Diff #48693)|
BugType names should be capitalized.
|721 ↗||(On Diff #48693)|
We might need an annotation for this as well.
This update addresses Anna's review comments. The big change is that the program state now maps instance symbols to sets of initial ivar symbols that must be released. (Rather than just storing the set, as before). With this approach we don't need to play the trick of explicitly binding the initial field symbols in the store to get notified that they escape when the instance escapes. Instead, we can look up the instance in the map when it escapes and remove the entire set.
I've also fixed a leak where we weren't removing values properly from the set that must be released and fixed a false positive so we no longer warn about a missing release when the field is known to be nil.
|184 ↗||(On Diff #48750)|
I've changed the method name to 'classHasSeparateTeardown()'.
|249 ↗||(On Diff #48750)|
I've removed this. It is not needed now that the program state is a map from instance symbols to sets of initial field values that must be released.
|383 ↗||(On Diff #48750)|
I've added an assertion to catch when these aren't being removed. (You were right, they weren't always.)